Correct the big shortcomings of the initial IQueue GC design

Author: devshgraphicsprogramming

examples_tests

@@ -151,8 +151,9 @@ class NBL_API2 ILogicalDevice : public core::IReferenceCounted, public IDeviceMe
         bool validateMemoryBarrier(const uint32_t queueFamilyIndex, const IGPUCommandBuffer::SImageMemoryBarrier<ResourceBarrier>& barrier) const;
 
 
-        //! Sync
-        virtual IQueue::RESULT waitIdle() const = 0;
+        //! Very important function, without it being called at the end of when you use a device, it will leak due to circular reference from resources used in the very last submit to a queue
+        //! Alternatively to get rid of circular refs, you can call `waitIdle` individually on every queue you've ever used
+        IQueue::RESULT waitIdle();
 
         //! Semaphore Stuff
         virtual core::smart_refctd_ptr<ISemaphore> createSemaphore(const uint64_t initialValue) = 0;
@@ -767,10 +768,16 @@ class NBL_API2 ILogicalDevice : public core::IReferenceCounted, public IDeviceMe
         ILogicalDevice(core::smart_refctd_ptr<const IAPIConnection>&& api, const IPhysicalDevice* const physicalDevice, const SCreationParams& params, const bool runningInRenderdoc);
         inline virtual ~ILogicalDevice()
         {
+            // There's no point calling `waitIdle` here for two reasons:
+            // - vtable already destroyed, you'll then call undefined function pointer for `waitIdle_impl`
+            // - `waitIdle` must have been called already or a similar operation performed, otherwise you'll have circular references from the per-queue GC and you'll never enter this destructor
             if (m_queues)
             for (uint32_t i=0u; i<m_queues->size(); ++i)
                 delete (*m_queues)[i];
         }
+
+        virtual IQueue::RESULT waitIdle_impl() const = 0;
+
         virtual bool flushMappedMemoryRanges_impl(const std::span<const MappedMemoryRange> ranges) = 0;
         virtual bool invalidateMappedMemoryRanges_impl(const std::span<const MappedMemoryRange> ranges) = 0;
 

include/nbl/video/ILogicalDevice.h

@@ -147,7 +147,7 @@ class IQueue : public core::Interface, public core::Unmovable
 				inline DeferredSubmitResourceDrop& operator=(DeferredSubmitResourceDrop&& other)
 				{
                     m_resources = std::move(other.m_resources);
-                    m_resources = nullptr;
+                    other.m_resources = nullptr;
 					return *this;
 				}
 
@@ -160,8 +160,11 @@ class IQueue : public core::Interface, public core::Unmovable
 
     protected:
         NBL_API2 IQueue(ILogicalDevice* originDevice, const uint32_t _famIx, const core::bitflag<CREATE_FLAGS> _flags, const float _priority);
-        // WARNING: Due to ordering of destructors, its the Base Class that implements `waitIdle_impl` that needs to call `waitIdle`!
-        NBL_API2 ~IQueue();
+        // As IQueue is logically an integral part of the `ILogicalDevice` the destructor will only run during `~ILogicalDevice` which means `waitIdle` already been called
+        inline ~IQueue()
+        {
+            //while (cullResources()) {} // deleter of `m_submittedResources` calls dtor which will do this
+        }
 
         friend class CThreadSafeQueueAdapter;
         virtual RESULT submit_impl(const std::span<const SSubmitInfo> _submits) = 0;

include/nbl/video/IQueue.h

@@ -46,12 +46,7 @@ class CVulkanLogicalDevice final : public ILogicalDevice
 
         CVulkanLogicalDevice(core::smart_refctd_ptr<const IAPIConnection>&& api, renderdoc_api_t* const rdoc, const IPhysicalDevice* const physicalDevice, const VkDevice vkdev, const SCreationParams& params);
 
-        // sync sutff
-        inline IQueue::RESULT waitIdle() const override
-        {
-            return CVulkanQueue::getResultFrom(m_devf.vk.vkDeviceWaitIdle(m_vkdev));
-        }
-            
+        // sync stuff
         core::smart_refctd_ptr<ISemaphore> createSemaphore(const uint64_t initialValue) override;
         ISemaphore::WAIT_RESULT waitForSemaphores(const std::span<const ISemaphore::SWaitInfo> infos, const bool waitAll, const uint64_t timeout) override;
 
@@ -94,6 +89,12 @@ class CVulkanLogicalDevice final : public ILogicalDevice
             m_devf.vk.vkDestroyDescriptorSetLayout(m_vkdev,m_dummyDSLayout,nullptr);
             m_devf.vk.vkDestroyDevice(m_vkdev,nullptr);
         }
+
+        // sync stuff
+        inline IQueue::RESULT waitIdle_impl() const override
+        {
+            return CVulkanQueue::getResultFrom(m_devf.vk.vkDeviceWaitIdle(m_vkdev));
+        }
 
         // memory  stuff
         bool flushMappedMemoryRanges_impl(const std::span<const MappedMemoryRange> ranges) override;

src/nbl/video/CVulkanLogicalDevice.h

@@ -44,12 +44,6 @@ class CVulkanQueue final : public IQueue
         inline VkQueue getInternalObject() const {return m_vkQueue;}
 
     private:
-        inline ~CVulkanQueue()
-        {
-            // need to ensure all submits end to stop spinning in the Base Class destructor
-            waitIdle_impl();
-        }
-
         RESULT submit_impl(const std::span<const SSubmitInfo> _submits) override;
         RESULT waitIdle_impl() const override;
 

src/nbl/video/CVulkanQueue.h

@@ -237,6 +237,17 @@ bool ILogicalDevice::validateMemoryBarrier(const uint32_t queueFamilyIndex, asse
     return true;
 }
 
+
+IQueue::RESULT ILogicalDevice::waitIdle()
+{
+    const auto retval = waitIdle_impl();
+    // Since this is equivalent to calling waitIdle on all queues, just proceed to releasing tracked resources
+    for (auto queue : *m_queues)
+        queue->cullResources();
+    return retval;
+}
+
+
 core::smart_refctd_ptr<IGPUBufferView> ILogicalDevice::createBufferView(const asset::SBufferRange<const IGPUBuffer>& underlying, const asset::E_FORMAT _fmt)
 {
     if (!underlying.isValid() || !underlying.buffer->wasCreatedBy(this))

src/nbl/video/ILogicalDevice.cpp

@@ -115,10 +115,4 @@ uint32_t IQueue::cullResources()
     return m_submittedResources->poll().eventsLeft;
 }
 
-IQueue::~IQueue()
-{
-    // if you find yourself spinning a lot here, its because the base class hasn't called waitIdle_impl()
-    //while (cullResources()) {} // deleter of `m_submittedResources` calls dtor which will do this
-}
-
 }