make the queue reference all resources used during previous submits and use the MultiTimelineEventHandlerST to let go of them after they're no longer pending

devshgraphicsprogramming · devshgraphicsprogramming · commit ebf444e0dfe3 · 2024-03-08T21:18:00.000+01:00
diff --git a/include/nbl/video/CThreadSafeQueueAdapter.h b/include/nbl/video/CThreadSafeQueueAdapter.h
@@ -12,34 +12,14 @@ namespace nbl::video
 */
 class CThreadSafeQueueAdapter final : public IQueue
 {
+        friend class ILogicalDevice; // to access the destructor
         friend class ISwapchain;
-    protected:
-        std::unique_ptr<IQueue> originalQueue = nullptr;
-        mutable std::mutex m;
-
-        inline RESULT submit_impl(const std::span<const SSubmitInfo> _submits) override
-        {
-            IQueue* msvcIsDumb = originalQueue.get();
-            return msvcIsDumb->submit_impl(_submits);
-        }
 
     public:
-        inline CThreadSafeQueueAdapter(ILogicalDevice* originDevice, std::unique_ptr<IQueue>&& original)
-            : IQueue(originDevice, original->getFamilyIndex(),original->getFlags(),original->getPriority()), originalQueue(std::move(original)) {}        
+        inline CThreadSafeQueueAdapter(ILogicalDevice* originDevice, IQueue* const original)
+            : IQueue(originDevice,original->getFamilyIndex(),original->getFlags(),original->getPriority()), originalQueue(original) {}        
         inline CThreadSafeQueueAdapter() : IQueue(nullptr, 0, CREATE_FLAGS::PROTECTED_BIT, 0.f) {}
 
-        inline RESULT waitIdle() const override
-        {
-            std::lock_guard g(m);
-            return originalQueue->waitIdle();
-        }
-
-        inline RESULT submit(const std::span<const SSubmitInfo> _submits) override
-        {
-            std::lock_guard g(m);
-            return originalQueue->submit(_submits);
-        }
-
         inline bool startCapture() override
         { 
             std::lock_guard g(m);
@@ -67,15 +47,56 @@ class CThreadSafeQueueAdapter final : public IQueue
             return originalQueue->endDebugMarker();
         }
 
+        inline RESULT submit(const std::span<const SSubmitInfo> _submits) override
+        {
+            std::lock_guard g(m);
+            return originalQueue->submit(_submits);
+        }
+
+        inline RESULT waitIdle() override
+        {
+            std::lock_guard g(m);
+            return originalQueue->waitIdle();
+        }
+
+        inline uint32_t cullResources() override
+        {
+            std::lock_guard g(m);
+            return originalQueue->cullResources();
+        }
+
         inline IQueue* getUnderlyingQueue() const
         {
-            return originalQueue.get();
+            return originalQueue;
         }
 
         inline const void* getNativeHandle() const
         {
             return originalQueue->getNativeHandle();
         }
+
+    protected:
+        inline ~CThreadSafeQueueAdapter()
+        {
+            delete originalQueue;
+        }
+
+        // These shall never be called, they're here just to stop the class being pure virtual
+        inline RESULT submit_impl(const std::span<const SSubmitInfo> _submits) override
+        {
+            assert(false);
+            return originalQueue->submit_impl(_submits);
+        }
+        inline RESULT waitIdle_impl() const override
+        {
+            assert(false);
+            return originalQueue->waitIdle_impl();
+        }
+
+
+        // used to use unique_ptr here, but it needed `~IQueue` to be public, which requires a custom deleter, etc.
+        IQueue* const originalQueue = nullptr;
+        mutable std::mutex m;
 };
 
 }
diff --git a/include/nbl/video/IQueue.h b/include/nbl/video/IQueue.h
@@ -8,6 +8,9 @@ namespace nbl::video
 
 class IGPUCommandBuffer;
 
+template<typename Functor,bool RefcountTheDevice>
+class MultiTimelineEventHandlerST;
+
 class IQueue : public core::Interface, public core::Unmovable
 {
     public:
@@ -70,7 +73,7 @@ class IQueue : public core::Interface, public core::Unmovable
             DEVICE_LOST,
             OTHER_ERROR
         };
-        //
+        // We actually make our Queue Abstraction keep track of Commandbuffers and Semaphores used in a submit until its done
         struct SSubmitInfo
         {
             struct SSemaphoreInfo
@@ -94,34 +97,78 @@ class IQueue : public core::Interface, public core::Unmovable
 
             inline bool valid() const
             {
-                // any two being empty is wrong
-                if (commandBuffers.empty() && signalSemaphores.empty()) // wait and do nothing
-                    return false;
-                if (waitSemaphores.empty() && signalSemaphores.empty()) // work without sync
-                    return false;
-                if (waitSemaphores.empty() && commandBuffers.empty()) // signal without doing work first
+                // need at least one semaphore to keep the submit resources alive
+                if (signalSemaphores.empty())
                     return false;
                 return true;
             }
         };
-        virtual RESULT submit(const std::span<const SSubmitInfo> _submits);
-        //
-        virtual RESULT waitIdle() const = 0;
+        NBL_API2 virtual RESULT submit(const std::span<const SSubmitInfo> _submits);
+        // Wait idle also makes sure all past submits drop their resources
+        NBL_API2 virtual RESULT waitIdle();
+        // You can call this to force an early check on whether past submits have completed and hasten when the refcount gets dropped.
+        // Normally its the next call to `submit` that polls the event timeline for completions.
+        NBL_API2 virtual uint32_t cullResources();
 
         // we cannot derive from IBackendObject because we can't derive from IReferenceCounted
         inline const ILogicalDevice* getOriginDevice() const {return m_originDevice;}
         inline bool wasCreatedBy(const ILogicalDevice* device) const {return device==m_originDevice;}
         // Vulkan: const VkQueue*
         virtual const void* getNativeHandle() const = 0;
+        
+		// only public because MultiTimelineEventHandlerST needs to know about it
+		class DeferredSubmitResourceDrop final
+		{
+                using smart_ptr = core::smart_refctd_ptr<IBackendObject>;
+				core::smart_refctd_dynamic_array<smart_ptr> m_resources;
+
+			public:
+				inline DeferredSubmitResourceDrop(const SSubmitInfo& info)
+				{
+                    // We could actually not hold any signal semaphore because you're expected to use the signal result somewhere else.
+                    // However it's possible to you might only wait on one from the set and then drop the rest (UB) 
+                    m_resources = core::make_refctd_dynamic_array<decltype(m_resources)>(info.signalSemaphores.size()-1+info.commandBuffers.size()+info.waitSemaphores.size());
+                    auto outRes = m_resources->data();
+                    for (const auto& sema : info.waitSemaphores)
+                        *(outRes++) = smart_ptr(sema.semaphore);
+                    for (const auto& cb : info.commandBuffers)
+                        *(outRes++) = smart_ptr(cb.cmdbuf);
+                    // We don't hold the last signal semaphore, because the timeline does as an Event trigger.
+                    for (auto i=0u; i<info.signalSemaphores.size()-1; i++)
+                        *(outRes++) = smart_ptr(info.signalSemaphores[i].semaphore);
+				}
+                DeferredSubmitResourceDrop(const DeferredSubmitResourceDrop& other) = delete;
+				inline DeferredSubmitResourceDrop(DeferredSubmitResourceDrop&& other) : m_resources(nullptr)
+				{
+					this->operator=(std::move(other));
+				}
+
+                DeferredSubmitResourceDrop& operator=(const DeferredSubmitResourceDrop& other) = delete;
+				inline DeferredSubmitResourceDrop& operator=(DeferredSubmitResourceDrop&& other)
+				{
+                    m_resources = std::move(other.m_resources);
+                    m_resources = nullptr;
+					return *this;
+				}
+
+                // always exhaustive poll, because we need to get rid of resources ASAP
+                inline void operator()()
+                {
+                    m_resources = nullptr;
+                }
+		};
 
     protected:
-        //! `flags` takes bits from E_CREATE_FLAGS
-        inline IQueue(const ILogicalDevice* originDevice, const uint32_t _famIx, const core::bitflag<CREATE_FLAGS> _flags, const float _priority)
-            : m_originDevice(originDevice), m_familyIndex(_famIx), m_priority(_priority), m_flags(_flags) {}
+        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();
 
         friend class CThreadSafeQueueAdapter;
         virtual RESULT submit_impl(const std::span<const SSubmitInfo> _submits) = 0;
+        virtual RESULT waitIdle_impl() const = 0;
 
+        // Refcounts all resources used by Pending Submits, gets occasionally cleared out
+        std::unique_ptr<MultiTimelineEventHandlerST<DeferredSubmitResourceDrop,false>> m_submittedResources;
         const ILogicalDevice* m_originDevice;
         const uint32_t m_familyIndex;
         const float m_priority;
diff --git a/include/nbl/video/ISwapchain.h b/include/nbl/video/ISwapchain.h
@@ -515,6 +515,7 @@ class ISwapchain : public IBackendObject
                     m_frameResources = nullptr;
                 }
 		};
+
     protected:
         ISwapchain(core::smart_refctd_ptr<const ILogicalDevice>&& dev, SCreationParams&& params, const uint8_t imageCount, core::smart_refctd_ptr<ISwapchain>&& oldSwapchain);
         virtual inline ~ISwapchain()
diff --git a/src/nbl/video/CVulkanLogicalDevice.cpp b/src/nbl/video/CVulkanLogicalDevice.cpp
@@ -36,8 +36,8 @@ m_vkdev(vkdev), m_devf(vkdev), m_deferred_op_mempool(NODES_PER_BLOCK_DEFERRED_OP
             m_devf.vk.vkGetDeviceQueue(m_vkdev,i,j,&q);
                         
             const uint32_t ix = offset+j;
-            auto queue = std::make_unique<CVulkanQueue>(this,rdoc,static_cast<const CVulkanConnection*>(m_api.get())->getInternalObject(),q,i,flags,priority);
-            (*m_queues)[ix] = new CThreadSafeQueueAdapter(this,std::move(queue));
+            auto queue = new CVulkanQueue(this,rdoc,static_cast<const CVulkanConnection*>(m_api.get())->getInternalObject(),q,i,flags,priority);
+            (*m_queues)[ix] = new CThreadSafeQueueAdapter(this,queue);
         }
     }
 
diff --git a/src/nbl/video/CVulkanQueue.cpp b/src/nbl/video/CVulkanQueue.cpp
@@ -7,7 +7,7 @@
 namespace nbl::video
 {
 
-auto CVulkanQueue::waitIdle() const -> RESULT
+auto CVulkanQueue::waitIdle_impl() const -> RESULT
 {
     return getResultFrom(static_cast<const CVulkanLogicalDevice*>(m_originDevice)->getFunctionTable()->vk.vkQueueWaitIdle(m_vkQueue));
 }
diff --git a/src/nbl/video/CVulkanQueue.h b/src/nbl/video/CVulkanQueue.h
@@ -15,7 +15,7 @@ class ILogicalDevice;
 class CVulkanQueue final : public IQueue
 {
     public:
-        inline CVulkanQueue(const ILogicalDevice* logicalDevice, renderdoc_api_t* rdoc, VkInstance vkinst, VkQueue vkq, const uint32_t _famIx, const core::bitflag<IQueue::CREATE_FLAGS> _flags, const float _priority)
+        inline CVulkanQueue(ILogicalDevice* logicalDevice, renderdoc_api_t* rdoc, VkInstance vkinst, VkQueue vkq, const uint32_t _famIx, const core::bitflag<IQueue::CREATE_FLAGS> _flags, const float _priority)
             : IQueue(logicalDevice, _famIx, _flags, _priority), m_vkQueue(vkq), m_rdoc_api(rdoc), m_vkInstance(vkinst) {}
 
         static inline RESULT getResultFrom(const VkResult result)
@@ -33,7 +33,6 @@ class CVulkanQueue final : public IQueue
             }
             return RESULT::OTHER_ERROR;
         }
-        RESULT waitIdle() const override;
 
         bool startCapture() override;
         bool endCapture() override;
@@ -45,7 +44,14 @@ 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;
 
         renderdoc_api_t* m_rdoc_api;
 	    VkInstance m_vkInstance;
diff --git a/src/nbl/video/IQueue.cpp b/src/nbl/video/IQueue.cpp
@@ -1,10 +1,17 @@
 #include "nbl/video/IQueue.h"
 #include "nbl/video/IGPUCommandBuffer.h"
 #include "nbl/video/ILogicalDevice.h"
+#include "nbl/video/TimelineEventHandlers.h"
 
 namespace nbl::video
 {
 
+IQueue::IQueue(ILogicalDevice* originDevice, const uint32_t _famIx, const core::bitflag<CREATE_FLAGS> _flags, const float _priority)
+    : m_originDevice(originDevice), m_familyIndex(_famIx), m_priority(_priority), m_flags(_flags)
+{
+    m_submittedResources = std::make_unique<MultiTimelineEventHandlerST<DeferredSubmitResourceDrop,false>>(originDevice);
+}
+
 auto IQueue::submit(const std::span<const SSubmitInfo> _submits) -> RESULT
 {
     if (_submits.empty())
@@ -78,11 +85,40 @@ auto IQueue::submit(const std::span<const SSubmitInfo> _submits) -> RESULT
     auto result = submit_impl(_submits);
     if (result!=RESULT::SUCCESS)
         return result;
-    // mark cmdbufs as done (wrongly but conservatively wrong)
+    // poll for whatever is done, free up memory ASAP
+    // we poll everything (full GC run) because we would release memory very slowly otherwise
+    m_submittedResources->poll();
+    //
     for (const auto& submit : _submits)
-    for (const auto& commandBuffer : submit.commandBuffers)
-        commandBuffer.cmdbuf->m_state = commandBuffer.cmdbuf->getRecordingFlags().hasFlags(IGPUCommandBuffer::USAGE::ONE_TIME_SUBMIT_BIT) ? IGPUCommandBuffer::STATE::INVALID:IGPUCommandBuffer::STATE::EXECUTABLE;
+    {
+        // hold onto the semaphores and commandbuffers until the submit signals the last semaphore
+        const auto& lastSignal = submit.signalSemaphores.back();
+        m_submittedResources->latch({.semaphore=lastSignal.semaphore,.value=lastSignal.value},DeferredSubmitResourceDrop(submit));
+        // Mark cmdbufs as done (wrongly but conservatively wrong)
+        // We can't use `m_submittedResources` to mark them done, because the functor may run "late" in the future, after the cmdbuf has already been validly reset or resubmitted
+        for (const auto& commandBuffer : submit.commandBuffers)
+            commandBuffer.cmdbuf->m_state = commandBuffer.cmdbuf->getRecordingFlags().hasFlags(IGPUCommandBuffer::USAGE::ONE_TIME_SUBMIT_BIT) ? IGPUCommandBuffer::STATE::INVALID:IGPUCommandBuffer::STATE::EXECUTABLE;
+    }
     return result;
 }
 
+auto IQueue::waitIdle() -> RESULT
+{
+    const auto retval = waitIdle_impl();
+    // everything will be murdered because every submitted semaphore so far will signal
+    m_submittedResources->abortAll();
+    return retval;
+}
+
+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
+}
+
 }

Original file line number	Diff line number	Diff line change
`@@ -515,6 +515,7 @@ class ISwapchain : public IBackendObject`
`515`	`515`	`m_frameResources = nullptr;`
`516`	`516`	`}`
`517`	`517`	`};`
	`518`	`+`
`518`	`519`	`protected:`
`519`	`520`	`ISwapchain(core::smart_refctd_ptr<const ILogicalDevice>&& dev, SCreationParams&& params, const uint8_t imageCount, core::smart_refctd_ptr<ISwapchain>&& oldSwapchain);`
`520`	`521`	`virtual inline ~ISwapchain()`
Original file line number	Diff line number	Diff line change
`@@ -36,8 +36,8 @@ m_vkdev(vkdev), m_devf(vkdev), m_deferred_op_mempool(NODES_PER_BLOCK_DEFERRED_OP`
`36`	`36`	`m_devf.vk.vkGetDeviceQueue(m_vkdev,i,j,&q);`
`37`	`37`
`38`	`38`	`const uint32_t ix = offset+j;`
`39`		`- auto queue = std::make_unique<CVulkanQueue>(this,rdoc,static_cast<const CVulkanConnection*>(m_api.get())->getInternalObject(),q,i,flags,priority);`
`40`		`- (*m_queues)[ix] = new CThreadSafeQueueAdapter(this,std::move(queue));`
	`39`	`+ auto queue = new CVulkanQueue(this,rdoc,static_cast<const CVulkanConnection*>(m_api.get())->getInternalObject(),q,i,flags,priority);`
	`40`	`+ (*m_queues)[ix] = new CThreadSafeQueueAdapter(this,queue);`
`41`	`41`	`}`
`42`	`42`	`}`
`43`	`43`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@`
`7`	`7`	`namespace nbl::video`
`8`	`8`	`{`
`9`	`9`
`10`		`-auto CVulkanQueue::waitIdle() const -> RESULT`
	`10`	`+auto CVulkanQueue::waitIdle_impl() const -> RESULT`
`11`	`11`	`{`
`12`	`12`	`return getResultFrom(static_cast<const CVulkanLogicalDevice*>(m_originDevice)->getFunctionTable()->vk.vkQueueWaitIdle(m_vkQueue));`
`13`	`13`	`}`