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;