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>

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
 

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 @@ class LinkNode {
     }
 
   private:
+    friend class LinkedList<T>;
     LinkNode<T>* previous_;
     LinkNode<T>* 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<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();
     }

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 @@ 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<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 @@ 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<std::mutex> lock(objectList.mutex);
+        object->InsertBefore(objectList.objects.head());
+    }
+
     std::mutex* DeviceBase::GetObjectListMutex(ObjectType type) {
         return &mObjectLists[type].mutex;
     }

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<const char*> 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

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<std::mutex> lock(*GetDevice()->GetObjectListMutex(GetType()));
+        if (!RemoveFromList()) {
+            return false;
+        }
+        DestroyApiObjectImpl();
+        return true;
+    }
+
+    void ApiObjectBase::DestroyApiObjectImpl() {
+    }
+
 }  // namespace dawn_native

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();
 

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

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<Device*> 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();

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<Device*> Create(AdapterBase* adapter,
                                              NSPRef<id<MTLDevice>> 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<ExecutionSerial> CheckAndUpdateCompletedSerials() override;
 

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.

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

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<Device*> Create(Adapter* adapter, const DeviceDescriptor* descriptor);
         ~Device() override;
@@ -156,7 +156,7 @@ namespace dawn_native { namespace null {
 
         ResultOrError<ExecutionSerial> CheckAndUpdateCompletedSerials() override;
 
-        void ShutDownImpl() override;
+        void DestroyImpl() override;
         MaybeError WaitForIdleForDestruction() override;
 
         std::vector<std::unique_ptr<PendingOperation>> mPendingOperations;

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);
     }
 

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<Device*> Create(AdapterBase* adapter,
                                              const DeviceDescriptor* descriptor,
@@ -118,7 +118,7 @@ namespace dawn_native { namespace opengl {
 
         void InitTogglesFromDriver();
         ResultOrError<ExecutionSerial> CheckAndUpdateCompletedSerials() override;
-        void ShutDownImpl() override;
+        void DestroyImpl() override;
         MaybeError WaitForIdleForDestruction() override;
 
         std::queue<std::pair<GLsync, ExecutionSerial>> mFencesInFlight;

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<Ref<BindGroupBase>> 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

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<Device*> 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

src/tests/unittests/LinkedListTests.cpp

@@ -369,6 +369,51 @@ TEST(LinkedList, IsInList) {
     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;