Reworked command list management in D3D12 backend

7 files changed, 105 insertions, 82 deletions

diff --git a/Graphics/GraphicsEngineD3D12/include/CommandContext.h b/Graphics/GraphicsEngineD3D12/include/CommandContext.h
index 13fd65eb..717354f3 100644
--- a/Graphics/GraphicsEngineD3D12/include/CommandContext.h
+++ b/Graphics/GraphicsEngineD3D12/include/CommandContext.h
@@ -61,11 +61,11 @@ public:
 
 	CommandContext( class CommandListManager& CmdListManager);
 
-    ~CommandContext(void);
+    ~CommandContext();
 
 	// Submit the command buffer and reset it.  This is encouraged to keep the GPU busy and reduce latency.
 	// Taking too long to build command lists and submit them can idle the GPU.
-	ID3D12GraphicsCommandList* Close(ID3D12CommandAllocator **ppAllocator);
+	ID3D12GraphicsCommandList* Close(CComPtr<ID3D12CommandAllocator>& pAllocator);
 	void Reset( CommandListManager& CmdListManager );
 
 	class GraphicsContext& AsGraphicsContext();
@@ -134,7 +134,7 @@ protected:
     void InsertAliasBarrier(D3D12ResourceBase& Before, D3D12ResourceBase& After, IDeviceObject &BeforeObj, IDeviceObject &AfterObj, bool FlushImmediate = false);
 
 	CComPtr<ID3D12GraphicsCommandList> m_pCommandList;
-	CComPtr<ID3D12CommandAllocator> m_pCurrentAllocator;
+	CComPtr<ID3D12CommandAllocator>    m_pCurrentAllocator;
 
     ID3D12PipelineState* m_pCurPipelineState         = nullptr;
 	ID3D12RootSignature* m_pCurGraphicsRootSignature = nullptr;
diff --git a/Graphics/GraphicsEngineD3D12/include/CommandListD3D12Impl.h b/Graphics/GraphicsEngineD3D12/include/CommandListD3D12Impl.h
index e3221fe4..b9c43692 100644
--- a/Graphics/GraphicsEngineD3D12/include/CommandListD3D12Impl.h
+++ b/Graphics/GraphicsEngineD3D12/include/CommandListD3D12Impl.h
@@ -53,7 +53,7 @@ public:
     
     ~CommandListD3D12Impl()
     {
-        VERIFY(m_pCmdContext == nullptr && m_pDeferredCtx == nullptr, "Destroying a command list that has not been executed");
+        DEV_CHECK_ERR(m_pCmdContext == nullptr && m_pDeferredCtx == nullptr, "Destroying a command list that has not been executed");
     }
 
     void Close(CommandContext*& pCmdContext, RefCntAutoPtr<DeviceContextD3D12Impl>& pDeferredCtx)
diff --git a/Graphics/GraphicsEngineD3D12/include/CommandListManager.h b/Graphics/GraphicsEngineD3D12/include/CommandListManager.h
index b88eb622..7f338d24 100644
--- a/Graphics/GraphicsEngineD3D12/include/CommandListManager.h
+++ b/Graphics/GraphicsEngineD3D12/include/CommandListManager.h
@@ -24,43 +24,49 @@
 #pragma once
 
 #include <vector>
-#include <deque>
 #include <mutex>
 #include <stdint.h>
 
 namespace Diligent
 {
 
+class RenderDeviceD3D12Impl;
+
 class CommandListManager
 {
 public:
-	CommandListManager(class RenderDeviceD3D12Impl *pDeviceD3D12);
+	CommandListManager(RenderDeviceD3D12Impl& DeviceD3D12Impl);
 	~CommandListManager();
 
-    CommandListManager(const CommandListManager&) = delete;
-    CommandListManager(CommandListManager&&) = delete;
-    CommandListManager& operator = (const CommandListManager&) = delete;
-    CommandListManager& operator = (CommandListManager&&) = delete;
+    CommandListManager             (const CommandListManager&)  = delete;
+    CommandListManager             (      CommandListManager&&) = delete;
+    CommandListManager& operator = (const CommandListManager&)  = delete;
+    CommandListManager& operator = (      CommandListManager&&) = delete;
 
     void CreateNewCommandList( ID3D12GraphicsCommandList** ppList, ID3D12CommandAllocator** ppAllocator );
     
-    // Discards the allocator.
-    // FenceValue is the value that was signaled by the command queue after it 
-    // executed the the command list created by the allocator
-    void DiscardAllocator( Uint64 FenceValue, ID3D12CommandAllocator* pAllocator );
-
     void RequestAllocator(ID3D12CommandAllocator** ppAllocator);
+    void ReleaseAllocator(CComPtr<ID3D12CommandAllocator>&& Allocator, Uint32 CmdQueue, Uint64 FenceValue);
 
-private:
-    // fist    - the fence value associated with the command list that was created by the allocator 
-    // second  - the allocator to be discarded
-    typedef std::pair<Uint64, CComPtr<ID3D12CommandAllocator> > DiscardedAllocatorQueueElemType;
-	std::deque< DiscardedAllocatorQueueElemType, STDAllocatorRawMem<DiscardedAllocatorQueueElemType> > m_DiscardedAllocators;
+    // Returns allocator to the list of available allocators. The GPU must have finished using the
+    // allocator
+    void FreeAllocator( CComPtr<ID3D12CommandAllocator>&& Allocator );
 
-	std::mutex m_AllocatorMutex;
-    RenderDeviceD3D12Impl *m_pDeviceD3D12;
+#ifdef DEVELOPMENT
+    Atomics::Long GetAllocatorCounter() const {return m_AllocatorCounter;}
+#endif
+
+private:
+    std::mutex m_AllocatorMutex;
+	std::vector< CComPtr<ID3D12CommandAllocator>, STDAllocatorRawMem<CComPtr<ID3D12CommandAllocator>> > m_FreeAllocators;
+	
+    RenderDeviceD3D12Impl& m_DeviceD3D12Impl;
 
     Atomics::AtomicLong m_NumAllocators = 0; // For debug purposes only
+
+#ifdef DEVELOPMENT
+    Atomics::AtomicLong m_AllocatorCounter = 0;
+#endif
 };
 
-}
\ No newline at end of file
+}
diff --git a/Graphics/GraphicsEngineD3D12/src/CommandContext.cpp b/Graphics/GraphicsEngineD3D12/src/CommandContext.cpp
index 5010e1c6..c0236a4a 100644
--- a/Graphics/GraphicsEngineD3D12/src/CommandContext.cpp
+++ b/Graphics/GraphicsEngineD3D12/src/CommandContext.cpp
@@ -45,19 +45,20 @@ CommandContext::CommandContext( CommandListManager& CmdListManager) :
 
 CommandContext::~CommandContext( void )
 {
+    DEV_CHECK_ERR(m_pCurrentAllocator == nullptr, "Command allocator must be released prior to destroying the command context");
 }
 
-
-
 void CommandContext::Reset( CommandListManager& CmdListManager )
 {
 	// We only call Reset() on previously freed contexts. The command list persists, but we need to
-	// request a new allocator if there is none
-    // The allocator may not be null if the command context was previously disposed without being executed
+	// request a new allocator
 	VERIFY_EXPR(m_pCommandList != nullptr);
     if( !m_pCurrentAllocator )
     {
         CmdListManager.RequestAllocator(&m_pCurrentAllocator);
+        // Unlike ID3D12CommandAllocator::Reset, ID3D12GraphicsCommandList::Reset can be called while the 
+        // command list is still being executed. A typical pattern is to submit a command list and then 
+        // immediately reset it to reuse the allocated memory for another command list.
         m_pCommandList->Reset(m_pCurrentAllocator, nullptr);
     }
 
@@ -76,7 +77,7 @@ void CommandContext::Reset( CommandListManager& CmdListManager )
 #endif
 }
 
-ID3D12GraphicsCommandList* CommandContext::Close(ID3D12CommandAllocator** ppAllocator)
+ID3D12GraphicsCommandList* CommandContext::Close(CComPtr<ID3D12CommandAllocator>& pAllocator)
 {
 	FlushResourceBarriers();
 
@@ -85,15 +86,12 @@ ID3D12GraphicsCommandList* CommandContext::Close(ID3D12CommandAllocator** ppAllo
 
 	VERIFY_EXPR(m_pCurrentAllocator != nullptr);
 	auto hr = m_pCommandList->Close();
-    VERIFY(SUCCEEDED(hr), "Failed to close the command list");
-
-    if( ppAllocator != nullptr )
-        *ppAllocator = m_pCurrentAllocator.Detach();
+    DEV_CHECK_ERR(SUCCEEDED(hr), "Failed to close the command list");
+    
+    pAllocator = std::move(m_pCurrentAllocator);
     return m_pCommandList;
 }
 
-
-
 void GraphicsContext::SetRenderTargets( UINT NumRTVs, ITextureViewD3D12** ppRTVs, ITextureViewD3D12* pDSV )
 {
     D3D12_CPU_DESCRIPTOR_HANDLE RTVHandles[8]; // Do not waste time initializing array to zero
diff --git a/Graphics/GraphicsEngineD3D12/src/CommandListManager.cpp b/Graphics/GraphicsEngineD3D12/src/CommandListManager.cpp
index 98095c1d..52ed168b 100644
--- a/Graphics/GraphicsEngineD3D12/src/CommandListManager.cpp
+++ b/Graphics/GraphicsEngineD3D12/src/CommandListManager.cpp
@@ -28,21 +28,22 @@
 namespace Diligent
 {
 
-CommandListManager::CommandListManager(RenderDeviceD3D12Impl *pDeviceD3D12) : 
-    m_pDeviceD3D12(pDeviceD3D12),
-    m_DiscardedAllocators(STD_ALLOCATOR_RAW_MEM(DiscardedAllocatorQueueElemType, GetRawAllocator(), "Allocator for deque<DiscardedAllocatorQueueElemType>"))
+CommandListManager::CommandListManager(RenderDeviceD3D12Impl& DeviceD3D12Impl) : 
+    m_DeviceD3D12Impl(DeviceD3D12Impl),
+    m_FreeAllocators(STD_ALLOCATOR_RAW_MEM(CComPtr<ID3D12CommandAllocator>, GetRawAllocator(), "Allocator for vector<CComPtr<ID3D12CommandAllocator>>"))
 {
 }
 
 CommandListManager::~CommandListManager()
 {
+    DEV_CHECK_ERR(m_AllocatorCounter == 0, m_AllocatorCounter, " allocator(s) have not been returned to the manager. This will cause a crash if these allocators are referenced by release queues and later returned via FreeAllocator()");
+    LOG_INFO_MESSAGE("Command list manager: created ", m_FreeAllocators.size(), " allocators");
 }
 
-
 void CommandListManager::CreateNewCommandList( ID3D12GraphicsCommandList** List, ID3D12CommandAllocator** Allocator )
 {
 	RequestAllocator(Allocator);
-    auto *pd3d12Device = m_pDeviceD3D12->GetD3D12Device();
+    auto* pd3d12Device = m_DeviceD3D12Impl.GetD3D12Device();
 	auto hr = pd3d12Device->CreateCommandList(1, D3D12_COMMAND_LIST_TYPE_DIRECT, *Allocator, nullptr, __uuidof(*List), reinterpret_cast<void**>(List) );
     VERIFY(SUCCEEDED(hr), "Failed to create command list");
 	(*List)->SetName(L"CommandList");
@@ -56,53 +57,69 @@ void CommandListManager::RequestAllocator(ID3D12CommandAllocator** ppAllocator)
     VERIFY( (*ppAllocator) == nullptr, "Allocator pointer is not null" );
     (*ppAllocator) = nullptr;
 
-    if (!m_DiscardedAllocators.empty())
+    if (!m_FreeAllocators.empty())
 	{
-        // Pick the oldest allocator at the front of the deque
-        // If this allocator is not yet available, there is no point in
-        // looking at other allocators since they were released even
-        // later
-		auto& AllocatorPair = m_DiscardedAllocators.front();
-        
-        // TODO: Rework
-        // Get the last completed fence value
-        auto CompletedFenceValue = m_pDeviceD3D12->GetCompletedFenceValue(0);
-        // Note that CompletedFenceValue only grows. So if after we queried
-        // the value, the actual value is increased in other thread, this will not
-        // be an issue as the only consequence is that potentially available  
-        // allocator may not be used.
-
-        // AllocatorPair.first is the fence value that was signaled AFTER the
-        // command list has been submitted. If CompletedFenceValue is at least
-        // this value, the allocator can be safely reused
-		if ( CompletedFenceValue >= AllocatorPair.first )
-		{
-			*ppAllocator = AllocatorPair.second.Detach();
-			auto hr = (*ppAllocator)->Reset();
-            VERIFY_EXPR(SUCCEEDED(hr));
-			m_DiscardedAllocators.pop_front();
-		}
+		*ppAllocator = m_FreeAllocators.back().Detach();
+		auto hr = (*ppAllocator)->Reset();
+        DEV_CHECK_ERR(SUCCEEDED(hr), "Failed to reset command allocator");
+		m_FreeAllocators.pop_back();
 	}
 
 	// If no allocators were ready to be reused, create a new one
 	if ((*ppAllocator) == nullptr)
 	{
-        auto *pd3d12Device = m_pDeviceD3D12->GetD3D12Device();
+        auto* pd3d12Device = m_DeviceD3D12Impl.GetD3D12Device();
 		auto hr = pd3d12Device->CreateCommandAllocator(D3D12_COMMAND_LIST_TYPE_DIRECT, __uuidof(*ppAllocator), reinterpret_cast<void**>(ppAllocator));
         VERIFY(SUCCEEDED(hr), "Failed to create command allocator");
 		wchar_t AllocatorName[32];
         swprintf(AllocatorName, _countof(AllocatorName), L"Cmd list allocator %ld", Atomics::AtomicIncrement(m_NumAllocators)-1);
 		(*ppAllocator)->SetName(AllocatorName);
 	}
+#ifdef DEVELOPMENT
+    Atomics::AtomicIncrement(m_AllocatorCounter);
+#endif
 }
 
-void CommandListManager::DiscardAllocator( Uint64 FenceValue, ID3D12CommandAllocator* pAllocator )
+void CommandListManager::ReleaseAllocator( CComPtr<ID3D12CommandAllocator>&& Allocator, Uint32 CmdQueue, Uint64 FenceValue )
 {
-	std::lock_guard<std::mutex> LockGuard(m_AllocatorMutex);
+    struct StaleAllocator
+    {
+        CComPtr<ID3D12CommandAllocator> Allocator;
+        CommandListManager*             Mgr;
+
+        StaleAllocator(CComPtr<ID3D12CommandAllocator>&& _Allocator, CommandListManager& _Mgr)noexcept :
+            Allocator (std::move(_Allocator)),
+            Mgr       (&_Mgr)
+        {
+        }
+
+        StaleAllocator            (const StaleAllocator&)  = delete;
+        StaleAllocator& operator= (const StaleAllocator&)  = delete;
+        StaleAllocator& operator= (      StaleAllocator&&) = delete;
+            
+        StaleAllocator(StaleAllocator&& rhs)noexcept : 
+            Allocator (std::move(rhs.Allocator)),
+            Mgr       (rhs.Mgr)
+        {
+            rhs.Mgr       = nullptr;
+        }
 
-    // FenceValue is the value that was signaled by the command queue after it 
-    // executed the command list created by the allocator
-	m_DiscardedAllocators.emplace_back( FenceValue, CComPtr<ID3D12CommandAllocator>(pAllocator) );
+        ~StaleAllocator()
+        {
+            if (Mgr != nullptr)
+                Mgr->FreeAllocator( std::move(Allocator) );
+        }
+    };
+    m_DeviceD3D12Impl.GetReleaseQueue(CmdQueue).DiscardResource(StaleAllocator{std::move(Allocator), *this}, FenceValue);
+}
+
+void CommandListManager::FreeAllocator( CComPtr<ID3D12CommandAllocator>&& Allocator )
+{
+	std::lock_guard<std::mutex> LockGuard(m_AllocatorMutex);
+	m_FreeAllocators.emplace_back( std::move(Allocator) );
+#ifdef DEVELOPMENT
+    Atomics::AtomicDecrement(m_AllocatorCounter);
+#endif
 }
 
 }
diff --git a/Graphics/GraphicsEngineD3D12/src/DeviceContextD3D12Impl.cpp b/Graphics/GraphicsEngineD3D12/src/DeviceContextD3D12Impl.cpp
index b98c526f..f2732a65 100644
--- a/Graphics/GraphicsEngineD3D12/src/DeviceContextD3D12Impl.cpp
+++ b/Graphics/GraphicsEngineD3D12/src/DeviceContextD3D12Impl.cpp
@@ -547,7 +547,7 @@ namespace Diligent
             if (m_NumCommandsInCurCtx != 0)
             {
                 m_pCurrCmdCtx->FlushResourceBarriers();
-                auto FenceValue = pDeviceD3D12Impl->CloseAndExecuteCommandContext(m_pCurrCmdCtx, true, &m_PendingFences);
+                pDeviceD3D12Impl->CloseAndExecuteCommandContext(m_pCurrCmdCtx, true, &m_PendingFences);
                 m_PendingFences.clear();
             }
             else
@@ -1072,7 +1072,7 @@ namespace Diligent
         CommandContext* pCmdContext = nullptr;
         RefCntAutoPtr<DeviceContextD3D12Impl> pDeferredCtx;
         pCmdListD3D12->Close(pCmdContext, pDeferredCtx);
-        auto FenceValue = m_pDevice.RawPtr<RenderDeviceD3D12Impl>()->CloseAndExecuteCommandContext(pCmdContext, true, nullptr);
+        m_pDevice.RawPtr<RenderDeviceD3D12Impl>()->CloseAndExecuteCommandContext(pCmdContext, true, nullptr);
         // Set the bit in the deferred context cmd queue mask corresponding to cmd queue of this context
         pDeferredCtx->m_SubmittedBuffersCmdQueueMask |= Uint64{1} << m_CommandQueueId;
     }
diff --git a/Graphics/GraphicsEngineD3D12/src/RenderDeviceD3D12Impl.cpp b/Graphics/GraphicsEngineD3D12/src/RenderDeviceD3D12Impl.cpp
index e4b07ab8..2812402d 100644
--- a/Graphics/GraphicsEngineD3D12/src/RenderDeviceD3D12Impl.cpp
+++ b/Graphics/GraphicsEngineD3D12/src/RenderDeviceD3D12Impl.cpp
@@ -63,7 +63,7 @@ RenderDeviceD3D12Impl :: RenderDeviceD3D12Impl(IReferenceCounters*          pRef
     },
     m_pd3d12Device  (pd3d12Device),
     m_EngineAttribs (CreationAttribs),
-    m_CmdListManager(this),
+    m_CmdListManager(*this),
     m_CPUDescriptorHeaps
     {
         {RawMemAllocator, *this, CreationAttribs.CPUDescriptorHeapAllocationSize[0], D3D12_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV, D3D12_DESCRIPTOR_HEAP_FLAG_NONE},
@@ -100,6 +100,7 @@ RenderDeviceD3D12Impl::~RenderDeviceD3D12Impl()
 
     DEV_CHECK_ERR(m_DynamicMemoryManager.GetAllocatedPageCounter() == 0, "All allocated dynamic pages must have been returned to the manager at this point.");
     m_DynamicMemoryManager.Destroy();
+    DEV_CHECK_ERR(m_CmdListManager.GetAllocatorCounter() == 0, "All allocators must have been returned to the manager at this point.");
 
 	m_ContextPool.clear();
     DestroyCommandQueues();
@@ -108,13 +109,17 @@ RenderDeviceD3D12Impl::~RenderDeviceD3D12Impl()
 void RenderDeviceD3D12Impl::DisposeCommandContext(CommandContext* pCtx)
 {
 	std::lock_guard<std::mutex> LockGuard(m_ContextAllocationMutex);
+    CComPtr<ID3D12CommandAllocator> pAllocator; 
+    pCtx->Close(pAllocator);
+    // Since allocator has not been used, we can add it directly to the free allocator list
+    m_CmdListManager.FreeAllocator(std::move(pAllocator));
     m_AvailableContexts.push_back(pCtx);
 }
 
 void RenderDeviceD3D12Impl::CloseAndExecuteTransientCommandContext(Uint32 CommandQueueIndex, CommandContext *pCtx)
 {
     CComPtr<ID3D12CommandAllocator> pAllocator;
-	auto *pCmdList = pCtx->Close(&pAllocator);
+    ID3D12GraphicsCommandList* pCmdList = pCtx->Close(pAllocator);
     Uint64 FenceValue = 0;
     // Execute command list directly through the queue to avoid interference with command list numbers in the queue
     LockCommandQueue(CommandQueueIndex, 
@@ -123,9 +128,8 @@ void RenderDeviceD3D12Impl::CloseAndExecuteTransientCommandContext(Uint32 Comman
             FenceValue = pCmdQueue->Submit(pCmdList);
         }
     );
-    // DiscardAllocator() is thread-safe
-    // TODO: Rework
-	m_CmdListManager.DiscardAllocator(FenceValue, pAllocator);
+
+	m_CmdListManager.ReleaseAllocator(std::move(pAllocator), CommandQueueIndex, FenceValue);
 
     {
 	    std::lock_guard<std::mutex> LockGuard(m_ContextAllocationMutex);
@@ -136,7 +140,7 @@ void RenderDeviceD3D12Impl::CloseAndExecuteTransientCommandContext(Uint32 Comman
 Uint64 RenderDeviceD3D12Impl::CloseAndExecuteCommandContext(CommandContext* pCtx, bool DiscardStaleObjects, std::vector<std::pair<Uint64, RefCntAutoPtr<IFence> > >* pSignalFences)
 {
     CComPtr<ID3D12CommandAllocator> pAllocator;
-	auto *pCmdList = pCtx->Close(&pAllocator);
+    ID3D12GraphicsCommandList* pCmdList = pCtx->Close(pAllocator);
 
     Uint32 QueueIndex = 0;
     Uint64 FenceValue = 0;
@@ -172,9 +176,7 @@ Uint64 RenderDeviceD3D12Impl::CloseAndExecuteCommandContext(CommandContext* pCtx
         }
     }
 
-    // DiscardAllocator() is thread-safe
-    // TODO: Rework
-	m_CmdListManager.DiscardAllocator(FenceValue, pAllocator);
+	m_CmdListManager.ReleaseAllocator(std::move(pAllocator), QueueIndex, FenceValue);
 
     {
 	    std::lock_guard<std::mutex> LockGuard(m_ContextAllocationMutex);
@@ -198,7 +200,7 @@ void RenderDeviceD3D12Impl::FlushStaleResources(Uint32 CmdQueueIndex)
     // Submit empty command list to the queue. This will effectively signal the fence and 
     // discard all resources
     ID3D12GraphicsCommandList* pNullCmdList = nullptr;
-    TRenderDeviceBase::SubmitCommandBuffer(0, pNullCmdList, true);
+    TRenderDeviceBase::SubmitCommandBuffer(CmdQueueIndex, pNullCmdList, true);
 }
 
 void RenderDeviceD3D12Impl::ReleaseStaleResources(bool ForceRelease)