From fc5a7d414fbbb262c77e41680a230b72bebd7a2e Mon Sep 17 00:00:00 2001 From: Loko Kung Date: Tue, 12 Oct 2021 17:46:26 +0000 Subject: [PATCH] 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 Reviewed-by: Austin Eng --- docs/device_facilities.md | 2 +- src/common/LinkedList.h | 22 ++++++++++++ src/dawn_native/Device.cpp | 45 +++++++++++++++++++++---- src/dawn_native/Device.h | 8 +++-- src/dawn_native/ObjectBase.cpp | 33 +++++++++++++----- src/dawn_native/ObjectBase.h | 21 +++++++++--- src/dawn_native/d3d12/DeviceD3D12.cpp | 4 +-- src/dawn_native/d3d12/DeviceD3D12.h | 4 +-- src/dawn_native/metal/DeviceMTL.h | 4 +-- src/dawn_native/metal/DeviceMTL.mm | 4 +-- src/dawn_native/null/DeviceNull.cpp | 4 +-- src/dawn_native/null/DeviceNull.h | 4 +-- src/dawn_native/opengl/DeviceGL.cpp | 4 +-- src/dawn_native/opengl/DeviceGL.h | 4 +-- src/dawn_native/vulkan/DeviceVk.cpp | 6 ++-- src/dawn_native/vulkan/DeviceVk.h | 4 +-- src/tests/unittests/LinkedListTests.cpp | 45 +++++++++++++++++++++++++ 17 files changed, 174 insertions(+), 44 deletions(-) diff --git a/docs/device_facilities.md b/docs/device_facilities.md index 3a2384c8a5..78b8ed5abd 100644 --- a/docs/device_facilities.md +++ b/docs/device_facilities.md @@ -43,7 +43,7 @@ After this the device is set in the `Disconnected` state. 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 9b89ad579d..881aa82c70 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 +class LinkNode; +template +class LinkedList; + template class LinkNode { public: @@ -165,6 +172,7 @@ class LinkNode { } private: + friend class LinkedList; LinkNode* previous_; LinkNode* next_; }; @@ -190,6 +198,20 @@ class LinkedList { e->InsertBefore(&root_); } + // Moves all elements (in order) of the list and appends them into |l| leaving the list empty. + void MoveInto(LinkedList* 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* head() const { return root_.next(); } diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 59036ad347..bc6f6a8a94 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 #include #include @@ -262,7 +263,30 @@ namespace dawn_native { 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 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 objects; + for (ObjectType type : kObjectTypeDependencyOrder) { + ApiObjectList& objList = mObjectLists[type]; + const std::lock_guard lock(objList.mutex); + objList.objects.MoveInto(&objects); + } + for (LinkNode* 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 @@ namespace dawn_native { 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 @@ namespace dawn_native { 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 @@ namespace dawn_native { return mState != State::Alive; } + void DeviceBase::TrackObject(ApiObjectBase* object) { + ApiObjectList& objectList = mObjectLists[object->GetType()]; + std::lock_guard 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 e0cb9fa2c0..5d51096f0e 100644 --- a/src/dawn_native/Device.h +++ b/src/dawn_native/Device.h @@ -298,6 +298,7 @@ namespace dawn_native { }; State GetState() const; bool IsLost() const; + void TrackObject(ApiObjectBase* object); std::mutex* GetObjectListMutex(ObjectType type); std::vector GetEnabledFeatures() const; @@ -357,7 +358,8 @@ namespace dawn_native { 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 @@ namespace dawn_native { 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 e33722fd73..d458334b51 100644 --- a/src/dawn_native/ObjectBase.cpp +++ b/src/dawn_native/ObjectBase.cpp @@ -37,14 +37,6 @@ namespace dawn_native { 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 @@ namespace dawn_native { : ObjectBase(device) { } + ApiObjectBase::~ApiObjectBase() { + ASSERT(!IsAlive()); + } + void ApiObjectBase::APISetLabel(const char* label) { mLabel = label; SetLabelImpl(); @@ -70,4 +66,25 @@ namespace dawn_native { 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 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 17d32f8ff4..8b14b777ae 100644 --- a/src/dawn_native/ObjectBase.h +++ b/src/dawn_native/ObjectBase.h @@ -35,12 +35,9 @@ namespace dawn_native { 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 @@ namespace dawn_native { 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 3b96092efd..4e019c257f 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.cpp +++ b/src/dawn_native/d3d12/DeviceD3D12.cpp @@ -169,7 +169,7 @@ namespace dawn_native { namespace d3d12 { } Device::~Device() { - ShutDownBase(); + Destroy(); } ID3D12Device* Device::GetD3D12Device() const { @@ -601,7 +601,7 @@ namespace dawn_native { namespace d3d12 { 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 186e29ee70..03856be6af 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.h +++ b/src/dawn_native/d3d12/DeviceD3D12.h @@ -39,7 +39,7 @@ namespace dawn_native { namespace d3d12 { } while (0) // Definition of backend types - class Device : public DeviceBase { + class Device final : public DeviceBase { public: static ResultOrError Create(Adapter* adapter, const DeviceDescriptor* descriptor); ~Device() override; @@ -181,7 +181,7 @@ namespace dawn_native { namespace d3d12 { 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 e5232cbbfa..5d16d8ed01 100644 --- a/src/dawn_native/metal/DeviceMTL.h +++ b/src/dawn_native/metal/DeviceMTL.h @@ -36,7 +36,7 @@ namespace dawn_native { namespace metal { struct KalmanInfo; } - class Device : public DeviceBase { + class Device final : public DeviceBase { public: static ResultOrError Create(AdapterBase* adapter, NSPRef> mtlDevice, @@ -122,7 +122,7 @@ namespace dawn_native { namespace metal { void* userdata) override; void InitTogglesFromDriver(); - void ShutDownImpl() override; + void DestroyImpl() override; MaybeError WaitForIdleForDestruction() override; ResultOrError CheckAndUpdateCompletedSerials() override; diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm index 67ce44bf85..acc351f5d5 100644 --- a/src/dawn_native/metal/DeviceMTL.mm +++ b/src/dawn_native/metal/DeviceMTL.mm @@ -121,7 +121,7 @@ namespace dawn_native { namespace metal { } Device::~Device() { - ShutDownBase(); + Destroy(); } MaybeError Device::Initialize() { @@ -483,7 +483,7 @@ namespace dawn_native { namespace metal { 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 ab6ec331f3..4f08b1bd33 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -87,7 +87,7 @@ namespace dawn_native { namespace null { } Device::~Device() { - ShutDownBase(); + Destroy(); } MaybeError Device::Initialize() { @@ -164,7 +164,7 @@ namespace dawn_native { namespace null { 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 c51152dd69..be1c6135b4 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -84,7 +84,7 @@ namespace dawn_native { namespace null { virtual void Execute() = 0; }; - class Device : public DeviceBase { + class Device final : public DeviceBase { public: static ResultOrError Create(Adapter* adapter, const DeviceDescriptor* descriptor); ~Device() override; @@ -156,7 +156,7 @@ namespace dawn_native { namespace null { ResultOrError CheckAndUpdateCompletedSerials() override; - void ShutDownImpl() override; + void DestroyImpl() override; MaybeError WaitForIdleForDestruction() override; std::vector> mPendingOperations; diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp index ac8be45ba2..aa2fa18592 100644 --- a/src/dawn_native/opengl/DeviceGL.cpp +++ b/src/dawn_native/opengl/DeviceGL.cpp @@ -50,7 +50,7 @@ namespace dawn_native { namespace opengl { } Device::~Device() { - ShutDownBase(); + Destroy(); } MaybeError Device::Initialize() { @@ -291,7 +291,7 @@ namespace dawn_native { namespace opengl { 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 b259647499..ee2ef46466 100644 --- a/src/dawn_native/opengl/DeviceGL.h +++ b/src/dawn_native/opengl/DeviceGL.h @@ -35,7 +35,7 @@ typedef void* EGLImage; namespace dawn_native { namespace opengl { - class Device : public DeviceBase { + class Device final : public DeviceBase { public: static ResultOrError Create(AdapterBase* adapter, const DeviceDescriptor* descriptor, @@ -118,7 +118,7 @@ namespace dawn_native { namespace opengl { void InitTogglesFromDriver(); ResultOrError CheckAndUpdateCompletedSerials() override; - void ShutDownImpl() override; + void DestroyImpl() override; MaybeError WaitForIdleForDestruction() override; std::queue> mFencesInFlight; diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp index 1834ae637b..a30a20eaf4 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -67,7 +67,7 @@ namespace dawn_native { namespace vulkan { // 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 @@ namespace dawn_native { namespace vulkan { } Device::~Device() { - ShutDownBase(); + Destroy(); } ResultOrError> Device::CreateBindGroupImpl( @@ -912,7 +912,7 @@ namespace dawn_native { namespace vulkan { 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 447023f180..3378bc88b5 100644 --- a/src/dawn_native/vulkan/DeviceVk.h +++ b/src/dawn_native/vulkan/DeviceVk.h @@ -40,7 +40,7 @@ namespace dawn_native { namespace vulkan { class RenderPassCache; class ResourceMemoryAllocator; - class Device : public DeviceBase { + class Device final : public DeviceBase { public: static ResultOrError Create(Adapter* adapter, const DeviceDescriptor* descriptor); ~Device() override; @@ -152,7 +152,7 @@ namespace dawn_native { namespace vulkan { 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 1a06d8cc94..72dd411d3e 100644 --- a/src/tests/unittests/LinkedListTests.cpp +++ b/src/tests/unittests/LinkedListTests.cpp @@ -369,6 +369,51 @@ TEST(LinkedList, IsInList) { EXPECT_FALSE(n.RemoveFromList()); } +TEST(LinkedList, MoveInto) { + LinkedList l1; + LinkedList 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 l1; + LinkedList 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 l1; + LinkedList 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 list;