Adds remaining setup logic to implement destroy in Device and ObjectBase.
- Renames some of the Device functions to be consistent with documentation
- Reverts change in https://dawn-review.googlesource.com/c/dawn/+/64820 for overloading mDevice == nullptr to determine if objects are alive because device is needed for error propagation. Instead, use list existence to determine if objects are alive
- Updates destroy api to return bool upwards in case we need to further process the extending objects
- Adds tracking functions in ObjectBase
- Adds final tag to all backend Device implementations
- Adds MoveInto LinkedList support to move list elements in O(1)
Bug: dawn:628
Change-Id: Iff70f4f7d55f46ee52d1bd0e02e3671819f2eed4
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/65861
Commit-Queue: Loko Kung <lokokung@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
diff --git a/docs/device_facilities.md b/docs/device_facilities.md
index 3a2384c..78b8ed5 100644
--- a/docs/device_facilities.md
+++ b/docs/device_facilities.md
@@ -43,7 +43,7 @@
If an `Alive` device is destroyed, then a similar flow to `LoseForTesting happens`.
All this ensures that during destruction or forceful disconnect of the device, it properly gets to the `Disconnected` state with no commands executing on the GPU.
-After disconnecting, frontend will call `backend::Device::ShutDownImpl` so that it can properly free driver objects.
+After disconnecting, frontend will call `backend::Device::DestroyImpl` so that it can properly free driver objects.
### Toggles
diff --git a/src/common/LinkedList.h b/src/common/LinkedList.h
index 9b89ad5..881aa82 100644
--- a/src/common/LinkedList.h
+++ b/src/common/LinkedList.h
@@ -6,6 +6,7 @@
// modifications:
// - Added iterators for ranged based iterations
// - Added in list check before removing node to prevent segfault, now returns true iff removed
+// - Added MoveInto functionality for moving list elements to another list
#ifndef COMMON_LINKED_LIST_H
#define COMMON_LINKED_LIST_H
@@ -89,6 +90,12 @@
// needs to glue on the "next" and "previous" pointers using
// some internal node type.
+// Forward declarations of the types in order for recursive referencing and friending.
+template <typename T>
+class LinkNode;
+template <typename T>
+class LinkedList;
+
template <typename T>
class LinkNode {
public:
@@ -165,6 +172,7 @@
}
private:
+ friend class LinkedList<T>;
LinkNode<T>* previous_;
LinkNode<T>* next_;
};
@@ -190,6 +198,20 @@
e->InsertBefore(&root_);
}
+ // Moves all elements (in order) of the list and appends them into |l| leaving the list empty.
+ void MoveInto(LinkedList<T>* l) {
+ if (empty()) {
+ return;
+ }
+ l->root_.previous_->next_ = root_.next_;
+ root_.next_->previous_ = l->root_.previous_;
+ l->root_.previous_ = root_.previous_;
+ root_.previous_->next_ = &l->root_;
+
+ root_.next_ = &root_;
+ root_.previous_ = &root_;
+ }
+
LinkNode<T>* head() const {
return root_.next();
}
diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp
index 59036ad..bc6f6a8 100644
--- a/src/dawn_native/Device.cpp
+++ b/src/dawn_native/Device.cpp
@@ -48,6 +48,7 @@
#include "dawn_native/ValidationUtils_autogen.h"
#include "dawn_platform/DawnPlatform.h"
+#include <array>
#include <mutex>
#include <unordered_set>
@@ -262,7 +263,30 @@
return {};
}
- void DeviceBase::ShutDownBase() {
+ void DeviceBase::DestroyObjects() {
+ // List of object types in reverse "dependency" order so we can iterate and delete the
+ // objects safely starting at leaf objects. We define dependent here such that if B has
+ // a ref to A, then B depends on A. We therefore try to destroy B before destroying A. Note
+ // that this only considers the immediate frontend dependencies, while backend objects could
+ // add complications and extra dependencies.
+ // TODO(dawn/628) Add types into the array as they are implemented.
+ static constexpr std::array<ObjectType, 0> kObjectTypeDependencyOrder = {};
+
+ // We first move all objects out from the tracking list into a separate list so that we can
+ // avoid locking the same mutex twice. We can then iterate across the separate list to call
+ // the actual destroy function.
+ LinkedList<ApiObjectBase> objects;
+ for (ObjectType type : kObjectTypeDependencyOrder) {
+ ApiObjectList& objList = mObjectLists[type];
+ const std::lock_guard<std::mutex> lock(objList.mutex);
+ objList.objects.MoveInto(&objects);
+ }
+ for (LinkNode<ApiObjectBase>* node : objects) {
+ node->value()->DestroyApiObject();
+ }
+ }
+
+ void DeviceBase::Destroy() {
// Skip handling device facilities if they haven't even been created (or failed doing so)
if (mState != State::BeingCreated) {
// Call all the callbacks immediately as the device is about to shut down.
@@ -304,8 +328,8 @@
if (mState != State::BeingCreated) {
// The GPU timeline is finished.
// Tick the queue-related tasks since they should be complete. This must be done before
- // ShutDownImpl() it may relinquish resources that will be freed by backends in the
- // ShutDownImpl() call.
+ // DestroyImpl() it may relinquish resources that will be freed by backends in the
+ // DestroyImpl() call.
mQueue->Tick(GetCompletedCommandSerial());
// Call TickImpl once last time to clean up resources
// Ignore errors so that we can continue with destruction
@@ -319,14 +343,15 @@
mCallbackTaskManager = nullptr;
mAsyncTaskManager = nullptr;
mPersistentCache = nullptr;
-
mEmptyBindGroupLayout = nullptr;
-
mInternalPipelineStore = nullptr;
AssumeCommandsComplete();
- // Tell the backend that it can free all the objects now that the GPU timeline is empty.
- ShutDownImpl();
+
+ // Now that the GPU timeline is empty, destroy all objects owned by the device, and then the
+ // backend device.
+ DestroyObjects();
+ DestroyImpl();
mCaches = nullptr;
}
@@ -499,6 +524,12 @@
return mState != State::Alive;
}
+ void DeviceBase::TrackObject(ApiObjectBase* object) {
+ ApiObjectList& objectList = mObjectLists[object->GetType()];
+ std::lock_guard<std::mutex> lock(objectList.mutex);
+ object->InsertBefore(objectList.objects.head());
+ }
+
std::mutex* DeviceBase::GetObjectListMutex(ObjectType type) {
return &mObjectLists[type].mutex;
}
diff --git a/src/dawn_native/Device.h b/src/dawn_native/Device.h
index e0cb9fa..5d51096 100644
--- a/src/dawn_native/Device.h
+++ b/src/dawn_native/Device.h
@@ -298,6 +298,7 @@
};
State GetState() const;
bool IsLost() const;
+ void TrackObject(ApiObjectBase* object);
std::mutex* GetObjectListMutex(ObjectType type);
std::vector<const char*> GetEnabledFeatures() const;
@@ -357,7 +358,8 @@
void ForceSetToggle(Toggle toggle, bool isEnabled);
MaybeError Initialize(QueueBase* defaultQueue);
- void ShutDownBase();
+ void DestroyObjects();
+ void Destroy();
// Incrememt mLastSubmittedSerial when we submit the next serial
void IncrementLastSubmittedCommandSerial();
@@ -450,9 +452,9 @@
ExecutionSerial mLastSubmittedSerial = ExecutionSerial(0);
ExecutionSerial mFutureSerial = ExecutionSerial(0);
- // ShutDownImpl is used to clean up and release resources used by device, does not wait for
+ // DestroyImpl is used to clean up and release resources used by device, does not wait for
// GPU or check errors.
- virtual void ShutDownImpl() = 0;
+ virtual void DestroyImpl() = 0;
// WaitForIdleForDestruction waits for GPU to finish, checks errors and gets ready for
// destruction. This is only used when properly destructing the device. For a real
diff --git a/src/dawn_native/ObjectBase.cpp b/src/dawn_native/ObjectBase.cpp
index e33722f..d458334 100644
--- a/src/dawn_native/ObjectBase.cpp
+++ b/src/dawn_native/ObjectBase.cpp
@@ -37,14 +37,6 @@
return GetRefCountPayload() == kErrorPayload;
}
- bool ObjectBase::IsAlive() const {
- return mDevice != nullptr;
- }
-
- void ObjectBase::DestroyObject() {
- mDevice = nullptr;
- }
-
ApiObjectBase::ApiObjectBase(DeviceBase* device, const char* label) : ObjectBase(device) {
if (label) {
mLabel = label;
@@ -58,6 +50,10 @@
: ObjectBase(device) {
}
+ ApiObjectBase::~ApiObjectBase() {
+ ASSERT(!IsAlive());
+ }
+
void ApiObjectBase::APISetLabel(const char* label) {
mLabel = label;
SetLabelImpl();
@@ -70,4 +66,25 @@
void ApiObjectBase::SetLabelImpl() {
}
+ bool ApiObjectBase::IsAlive() const {
+ return IsInList();
+ }
+
+ void ApiObjectBase::TrackInDevice() {
+ ASSERT(GetDevice() != nullptr);
+ GetDevice()->TrackObject(this);
+ }
+
+ bool ApiObjectBase::DestroyApiObject() {
+ const std::lock_guard<std::mutex> lock(*GetDevice()->GetObjectListMutex(GetType()));
+ if (!RemoveFromList()) {
+ return false;
+ }
+ DestroyApiObjectImpl();
+ return true;
+ }
+
+ void ApiObjectBase::DestroyApiObjectImpl() {
+ }
+
} // namespace dawn_native
diff --git a/src/dawn_native/ObjectBase.h b/src/dawn_native/ObjectBase.h
index 17d32f8..8b14b77 100644
--- a/src/dawn_native/ObjectBase.h
+++ b/src/dawn_native/ObjectBase.h
@@ -35,12 +35,9 @@
DeviceBase* GetDevice() const;
bool IsError() const;
- bool IsAlive() const;
- void DestroyObject();
private:
- // Pointer to owning device, if nullptr, that means that the object is no longer alive or
- // valid.
+ // Pointer to owning device.
DeviceBase* mDevice;
};
@@ -52,13 +49,29 @@
ApiObjectBase(DeviceBase* device, LabelNotImplementedTag tag);
ApiObjectBase(DeviceBase* device, const char* label);
ApiObjectBase(DeviceBase* device, ErrorTag tag);
+ virtual ~ApiObjectBase() override;
virtual ObjectType GetType() const = 0;
const std::string& GetLabel() const;
+ // The ApiObjectBase is considered alive if it is tracked in a respective linked list owned
+ // by the owning device.
+ bool IsAlive() const;
+
+ // Allow virtual overriding of actual destroy call in order to allow for re-using of base
+ // destruction oerations. Classes that override this function should almost always call this
+ // class's implementation in the override. This needs to be public because it can be called
+ // from the device owning the object. Returns true iff destruction occurs. Upon any re-calls
+ // of the function it will return false to indicate no further operations should be taken.
+ virtual bool DestroyApiObject();
+
// Dawn API
void APISetLabel(const char* label);
+ protected:
+ void TrackInDevice();
+ virtual void DestroyApiObjectImpl();
+
private:
virtual void SetLabelImpl();
diff --git a/src/dawn_native/d3d12/DeviceD3D12.cpp b/src/dawn_native/d3d12/DeviceD3D12.cpp
index 3b96092..4e019c2 100644
--- a/src/dawn_native/d3d12/DeviceD3D12.cpp
+++ b/src/dawn_native/d3d12/DeviceD3D12.cpp
@@ -169,7 +169,7 @@
}
Device::~Device() {
- ShutDownBase();
+ Destroy();
}
ID3D12Device* Device::GetD3D12Device() const {
@@ -601,7 +601,7 @@
return DAWN_INTERNAL_ERROR(messages.str());
}
- void Device::ShutDownImpl() {
+ void Device::DestroyImpl() {
ASSERT(GetState() == State::Disconnected);
// Immediately forget about all pending commands for the case where device is lost on its
diff --git a/src/dawn_native/d3d12/DeviceD3D12.h b/src/dawn_native/d3d12/DeviceD3D12.h
index 186e29e..03856be 100644
--- a/src/dawn_native/d3d12/DeviceD3D12.h
+++ b/src/dawn_native/d3d12/DeviceD3D12.h
@@ -39,7 +39,7 @@
} while (0)
// Definition of backend types
- class Device : public DeviceBase {
+ class Device final : public DeviceBase {
public:
static ResultOrError<Device*> Create(Adapter* adapter, const DeviceDescriptor* descriptor);
~Device() override;
@@ -181,7 +181,7 @@
WGPUCreateRenderPipelineAsyncCallback callback,
void* userdata) override;
- void ShutDownImpl() override;
+ void DestroyImpl() override;
MaybeError WaitForIdleForDestruction() override;
MaybeError CheckDebugLayerAndGenerateErrors();
diff --git a/src/dawn_native/metal/DeviceMTL.h b/src/dawn_native/metal/DeviceMTL.h
index e5232cb..5d16d8e 100644
--- a/src/dawn_native/metal/DeviceMTL.h
+++ b/src/dawn_native/metal/DeviceMTL.h
@@ -36,7 +36,7 @@
struct KalmanInfo;
}
- class Device : public DeviceBase {
+ class Device final : public DeviceBase {
public:
static ResultOrError<Device*> Create(AdapterBase* adapter,
NSPRef<id<MTLDevice>> mtlDevice,
@@ -122,7 +122,7 @@
void* userdata) override;
void InitTogglesFromDriver();
- void ShutDownImpl() override;
+ void DestroyImpl() override;
MaybeError WaitForIdleForDestruction() override;
ResultOrError<ExecutionSerial> CheckAndUpdateCompletedSerials() override;
diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm
index 67ce44b..acc351f 100644
--- a/src/dawn_native/metal/DeviceMTL.mm
+++ b/src/dawn_native/metal/DeviceMTL.mm
@@ -121,7 +121,7 @@
}
Device::~Device() {
- ShutDownBase();
+ Destroy();
}
MaybeError Device::Initialize() {
@@ -483,7 +483,7 @@
return {};
}
- void Device::ShutDownImpl() {
+ void Device::DestroyImpl() {
ASSERT(GetState() == State::Disconnected);
// Forget all pending commands.
diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp
index ab6ec33..4f08b1b 100644
--- a/src/dawn_native/null/DeviceNull.cpp
+++ b/src/dawn_native/null/DeviceNull.cpp
@@ -87,7 +87,7 @@
}
Device::~Device() {
- ShutDownBase();
+ Destroy();
}
MaybeError Device::Initialize() {
@@ -164,7 +164,7 @@
return std::move(stagingBuffer);
}
- void Device::ShutDownImpl() {
+ void Device::DestroyImpl() {
ASSERT(GetState() == State::Disconnected);
// Clear pending operations before checking mMemoryUsage because some operations keep a
diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h
index c51152d..be1c613 100644
--- a/src/dawn_native/null/DeviceNull.h
+++ b/src/dawn_native/null/DeviceNull.h
@@ -84,7 +84,7 @@
virtual void Execute() = 0;
};
- class Device : public DeviceBase {
+ class Device final : public DeviceBase {
public:
static ResultOrError<Device*> Create(Adapter* adapter, const DeviceDescriptor* descriptor);
~Device() override;
@@ -156,7 +156,7 @@
ResultOrError<ExecutionSerial> CheckAndUpdateCompletedSerials() override;
- void ShutDownImpl() override;
+ void DestroyImpl() override;
MaybeError WaitForIdleForDestruction() override;
std::vector<std::unique_ptr<PendingOperation>> mPendingOperations;
diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp
index ac8be45..aa2fa18 100644
--- a/src/dawn_native/opengl/DeviceGL.cpp
+++ b/src/dawn_native/opengl/DeviceGL.cpp
@@ -50,7 +50,7 @@
}
Device::~Device() {
- ShutDownBase();
+ Destroy();
}
MaybeError Device::Initialize() {
@@ -291,7 +291,7 @@
return DAWN_UNIMPLEMENTED_ERROR("Device unable to copy from staging buffer to texture.");
}
- void Device::ShutDownImpl() {
+ void Device::DestroyImpl() {
ASSERT(GetState() == State::Disconnected);
}
diff --git a/src/dawn_native/opengl/DeviceGL.h b/src/dawn_native/opengl/DeviceGL.h
index b259647..ee2ef46 100644
--- a/src/dawn_native/opengl/DeviceGL.h
+++ b/src/dawn_native/opengl/DeviceGL.h
@@ -35,7 +35,7 @@
namespace dawn_native { namespace opengl {
- class Device : public DeviceBase {
+ class Device final : public DeviceBase {
public:
static ResultOrError<Device*> Create(AdapterBase* adapter,
const DeviceDescriptor* descriptor,
@@ -118,7 +118,7 @@
void InitTogglesFromDriver();
ResultOrError<ExecutionSerial> CheckAndUpdateCompletedSerials() override;
- void ShutDownImpl() override;
+ void DestroyImpl() override;
MaybeError WaitForIdleForDestruction() override;
std::queue<std::pair<GLsync, ExecutionSerial>> mFencesInFlight;
diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp
index 1834ae6..a30a20e 100644
--- a/src/dawn_native/vulkan/DeviceVk.cpp
+++ b/src/dawn_native/vulkan/DeviceVk.cpp
@@ -67,7 +67,7 @@
// Two things are crucial if device initialization fails: the function pointers to destroy
// objects, and the fence deleter that calls these functions. Do not do anything before
// these two are set up, so that a failed initialization doesn't cause a crash in
- // ShutDownImpl()
+ // DestroyImpl()
{
VkPhysicalDevice physicalDevice = ToBackend(GetAdapter())->GetPhysicalDevice();
@@ -100,7 +100,7 @@
}
Device::~Device() {
- ShutDownBase();
+ Destroy();
}
ResultOrError<Ref<BindGroupBase>> Device::CreateBindGroupImpl(
@@ -912,7 +912,7 @@
return {};
}
- void Device::ShutDownImpl() {
+ void Device::DestroyImpl() {
ASSERT(GetState() == State::Disconnected);
// We failed during initialization so early that we don't even have a VkDevice. There is
diff --git a/src/dawn_native/vulkan/DeviceVk.h b/src/dawn_native/vulkan/DeviceVk.h
index 447023f..3378bc8 100644
--- a/src/dawn_native/vulkan/DeviceVk.h
+++ b/src/dawn_native/vulkan/DeviceVk.h
@@ -40,7 +40,7 @@
class RenderPassCache;
class ResourceMemoryAllocator;
- class Device : public DeviceBase {
+ class Device final : public DeviceBase {
public:
static ResultOrError<Device*> Create(Adapter* adapter, const DeviceDescriptor* descriptor);
~Device() override;
@@ -152,7 +152,7 @@
void InitTogglesFromDriver();
void ApplyDepth24PlusS8Toggle();
- void ShutDownImpl() override;
+ void DestroyImpl() override;
MaybeError WaitForIdleForDestruction() override;
// To make it easier to use fn it is a public const member. However
diff --git a/src/tests/unittests/LinkedListTests.cpp b/src/tests/unittests/LinkedListTests.cpp
index 1a06d8c..72dd411 100644
--- a/src/tests/unittests/LinkedListTests.cpp
+++ b/src/tests/unittests/LinkedListTests.cpp
@@ -369,6 +369,51 @@
EXPECT_FALSE(n.RemoveFromList());
}
+TEST(LinkedList, MoveInto) {
+ LinkedList<Node> l1;
+ LinkedList<Node> l2;
+
+ Node n1(1);
+ Node n2(2);
+ l1.Append(&n1);
+ l2.Append(&n2);
+
+ l2.MoveInto(&l1);
+ const int expected[] = {1, 2};
+ ExpectListContents(l1, 2, expected);
+ EXPECT_TRUE(l2.empty());
+}
+
+TEST(LinkedList, MoveEmptyListInto) {
+ LinkedList<Node> l1;
+ LinkedList<Node> l2;
+
+ Node n1(1);
+ Node n2(2);
+ l1.Append(&n1);
+ l1.Append(&n2);
+
+ l2.MoveInto(&l1);
+ const int expected[] = {1, 2};
+ ExpectListContents(l1, 2, expected);
+ EXPECT_TRUE(l2.empty());
+}
+
+TEST(LinkedList, MoveIntoEmpty) {
+ LinkedList<Node> l1;
+ LinkedList<Node> l2;
+
+ Node n1(1);
+ Node n2(2);
+ l2.Append(&n1);
+ l2.Append(&n2);
+
+ l2.MoveInto(&l1);
+ const int expected[] = {1, 2};
+ ExpectListContents(l1, 2, expected);
+ EXPECT_TRUE(l2.empty());
+}
+
TEST(LinkedList, RangeBasedModify) {
LinkedList<Node> list;
