From 69fce9771a6198cb3b0774784ce59820c2b4b200 Mon Sep 17 00:00:00 2001 From: Bryan Bernhart Date: Tue, 5 Jul 2022 17:20:18 -0700 Subject: [PATCH] Improve debug name error handling. Refacators ResourceAllocation so debug name update is done by CreateResourceAllocation, which is fail-able. --- src/gpgmm/d3d12/JSONSerializerD3D12.cpp | 8 ++--- src/gpgmm/d3d12/ResourceAllocationD3D12.cpp | 24 +++++++++++++- src/gpgmm/d3d12/ResourceAllocationD3D12.h | 27 ++++++++++------ src/gpgmm/d3d12/ResourceAllocatorD3D12.cpp | 31 ++++++++++--------- .../end2end/D3D12ResourceAllocatorTests.cpp | 12 +++++++ 5 files changed, 73 insertions(+), 29 deletions(-) diff --git a/src/gpgmm/d3d12/JSONSerializerD3D12.cpp b/src/gpgmm/d3d12/JSONSerializerD3D12.cpp index c7f35e288..91dae69b3 100644 --- a/src/gpgmm/d3d12/JSONSerializerD3D12.cpp +++ b/src/gpgmm/d3d12/JSONSerializerD3D12.cpp @@ -173,6 +173,9 @@ namespace gpgmm::d3d12 { dict.AddItem("OffsetFromResource", desc.OffsetFromResource); dict.AddItem("Method", desc.Method); dict.AddItem("ResourceHeap", gpgmm::JSONSerializer::Serialize(desc.ResourceHeap)); + if (!desc.DebugName.empty()) { + dict.AddItem("DebugName", desc.DebugName); + } return dict; } @@ -181,11 +184,6 @@ namespace gpgmm::d3d12 { JSONDict dict; dict.AddItem("SizeInBytes", info.SizeInBytes); dict.AddItem("Alignment", info.Alignment); - - if (!info.DebugName.empty()) { - dict.AddItem("DebugName", info.DebugName); - } - return dict; } diff --git a/src/gpgmm/d3d12/ResourceAllocationD3D12.cpp b/src/gpgmm/d3d12/ResourceAllocationD3D12.cpp index 5d52c667a..67f3e0711 100644 --- a/src/gpgmm/d3d12/ResourceAllocationD3D12.cpp +++ b/src/gpgmm/d3d12/ResourceAllocationD3D12.cpp @@ -41,6 +41,28 @@ namespace gpgmm::d3d12 { } // namespace + // static + HRESULT ResourceAllocation::CreateResourceAllocation( + const RESOURCE_ALLOCATION_DESC& descriptor, + ResidencyManager* residencyManager, + MemoryAllocator* allocator, + MemoryBlock* block, + ComPtr resource, + ResourceAllocation** ppResourceAllocationOut) { + std::unique_ptr resourceAllocation(new ResourceAllocation( + descriptor, residencyManager, allocator, block, std::move(resource))); + + if (!descriptor.DebugName.empty()) { + ReturnIfFailed(resourceAllocation->SetDebugName(descriptor.DebugName)); + } + + if (ppResourceAllocationOut != nullptr) { + *ppResourceAllocationOut = resourceAllocation.release(); + } + + return S_OK; + } + ResourceAllocation::ResourceAllocation(const RESOURCE_ALLOCATION_DESC& desc, ResidencyManager* residencyManager, MemoryAllocator* allocator, @@ -145,7 +167,7 @@ namespace gpgmm::d3d12 { } RESOURCE_ALLOCATION_INFO ResourceAllocation::GetInfo() const { - return {GetSize(), GetAlignment(), GetDebugName()}; + return {GetSize(), GetAlignment()}; } const char* ResourceAllocation::GetTypename() const { diff --git a/src/gpgmm/d3d12/ResourceAllocationD3D12.h b/src/gpgmm/d3d12/ResourceAllocationD3D12.h index a0b36da9b..6e6ff140a 100644 --- a/src/gpgmm/d3d12/ResourceAllocationD3D12.h +++ b/src/gpgmm/d3d12/ResourceAllocationD3D12.h @@ -62,6 +62,10 @@ namespace gpgmm::d3d12 { Must be valid for the duration of the resource allocation. */ Heap* ResourceHeap; + + /** \brief Debug name associated with the resource allocation. + */ + std::string DebugName; }; /** \struct RESOURCE_ALLOCATION_INFO @@ -79,10 +83,6 @@ namespace gpgmm::d3d12 { Must be non-zero. */ uint64_t Alignment; - - /** \brief Debug name associated with the resource allocation. - */ - std::string DebugName; }; /** \brief ResourceAllocation is MemoryAllocation that contains a ID3D12Resource. @@ -108,12 +108,15 @@ namespace gpgmm::d3d12 { resource. @param block A pointer to MemoryBlock which describes the region in Heap being allocated. @param resource A pointer to the ID3D12Resource used for the allocation. + @param[out] ppResourceAllocationOut Pointer to a resource allocation that recieves a pointer + to the resource allocation. */ - ResourceAllocation(const RESOURCE_ALLOCATION_DESC& desc, - ResidencyManager* residencyManager, - MemoryAllocator* allocator, - MemoryBlock* block, - ComPtr resource); + static HRESULT CreateResourceAllocation(const RESOURCE_ALLOCATION_DESC& desc, + ResidencyManager* residencyManager, + MemoryAllocator* allocator, + MemoryBlock* block, + ComPtr resource, + ResourceAllocation** ppResourceAllocationOut); ~ResourceAllocation() override; @@ -195,6 +198,12 @@ namespace gpgmm::d3d12 { Heap* GetMemory() const; private: + ResourceAllocation(const RESOURCE_ALLOCATION_DESC& desc, + ResidencyManager* residencyManager, + MemoryAllocator* allocator, + MemoryBlock* block, + ComPtr resource); + // Only DebugResourceAllocator may inject itself to ensure |this| cannot leak. friend DebugResourceAllocator; void SetDebugAllocator(MemoryAllocator* allocator); diff --git a/src/gpgmm/d3d12/ResourceAllocatorD3D12.cpp b/src/gpgmm/d3d12/ResourceAllocatorD3D12.cpp index 9db27ebff..18215a1da 100644 --- a/src/gpgmm/d3d12/ResourceAllocatorD3D12.cpp +++ b/src/gpgmm/d3d12/ResourceAllocatorD3D12.cpp @@ -685,10 +685,6 @@ namespace gpgmm::d3d12 { initialResourceState, pClearValue, ppResourceAllocationOut)); - if (!allocationDescriptor.DebugName.empty()) { - (*ppResourceAllocationOut)->SetDebugName(allocationDescriptor.DebugName); - } - // Insert a new (debug) allocator layer into the allocation so it can report details used // during leak checks. Since we don't want to use it unless we are debugging, we hide it // behind a macro. @@ -837,10 +833,12 @@ namespace gpgmm::d3d12 { allocationDesc.Method = AllocationMethod::kSubAllocatedWithin; allocationDesc.OffsetFromResource = subAllocation.GetOffset(); allocationDesc.ResourceHeap = resourceHeap; + allocationDesc.DebugName = allocationDescriptor.DebugName; - *ppResourceAllocationOut = new ResourceAllocation{ + ReturnIfFailed(ResourceAllocation::CreateResourceAllocation( allocationDesc, mResidencyManager.Get(), subAllocation.GetAllocator(), - subAllocation.GetBlock(), std::move(committedResource)}; + subAllocation.GetBlock(), std::move(committedResource), + ppResourceAllocationOut)); return S_OK; })); @@ -876,10 +874,12 @@ namespace gpgmm::d3d12 { allocationDesc.Method = subAllocation.GetMethod(); allocationDesc.OffsetFromResource = 0; allocationDesc.ResourceHeap = resourceHeap; + allocationDesc.DebugName = allocationDescriptor.DebugName; - *ppResourceAllocationOut = new ResourceAllocation{ + ReturnIfFailed(ResourceAllocation::CreateResourceAllocation( allocationDesc, mResidencyManager.Get(), subAllocation.GetAllocator(), - subAllocation.GetBlock(), std::move(placedResource)}; + subAllocation.GetBlock(), std::move(placedResource), + ppResourceAllocationOut)); return S_OK; })); @@ -915,10 +915,11 @@ namespace gpgmm::d3d12 { allocationDesc.Method = allocation.GetMethod(); allocationDesc.OffsetFromResource = 0; allocationDesc.ResourceHeap = resourceHeap; + allocationDesc.DebugName = allocationDescriptor.DebugName; - *ppResourceAllocationOut = new ResourceAllocation{ + ReturnIfFailed(ResourceAllocation::CreateResourceAllocation( allocationDesc, mResidencyManager.Get(), allocation.GetAllocator(), - allocation.GetBlock(), std::move(placedResource)}; + allocation.GetBlock(), std::move(placedResource), ppResourceAllocationOut)); return S_OK; })); @@ -956,9 +957,11 @@ namespace gpgmm::d3d12 { allocationDesc.Method = AllocationMethod::kStandalone; allocationDesc.OffsetFromResource = 0; allocationDesc.ResourceHeap = resourceHeap; + allocationDesc.DebugName = allocationDescriptor.DebugName; - *ppResourceAllocationOut = new ResourceAllocation{ - allocationDesc, mResidencyManager.Get(), this, nullptr, std::move(committedResource)}; + ReturnIfFailed(ResourceAllocation::CreateResourceAllocation( + allocationDesc, mResidencyManager.Get(), this, nullptr, std::move(committedResource), + ppResourceAllocationOut)); return S_OK; } @@ -1005,8 +1008,8 @@ namespace gpgmm::d3d12 { allocationDesc.OffsetFromResource = 0; allocationDesc.ResourceHeap = resourceHeap; - *ppResourceAllocationOut = - new ResourceAllocation{allocationDesc, nullptr, this, nullptr, std::move(resource)}; + ReturnIfFailed(ResourceAllocation::CreateResourceAllocation( + allocationDesc, nullptr, this, nullptr, std::move(resource), ppResourceAllocationOut)); return S_OK; } diff --git a/src/tests/end2end/D3D12ResourceAllocatorTests.cpp b/src/tests/end2end/D3D12ResourceAllocatorTests.cpp index e6b04f71e..7984555df 100644 --- a/src/tests/end2end/D3D12ResourceAllocatorTests.cpp +++ b/src/tests/end2end/D3D12ResourceAllocatorTests.cpp @@ -349,6 +349,18 @@ TEST_F(D3D12ResourceAllocatorTests, CreateBuffer) { allocationDescWithInvalidHeapFlags, CreateBasicBufferDesc(kDefaultBufferSize), D3D12_RESOURCE_STATE_COMMON, nullptr, &allocation)); } + + // Creating a buffer with a name should be always specified. + { + ComPtr allocation; + ALLOCATION_DESC allocationDesc = {}; + allocationDesc.DebugName = "Buffer"; + ASSERT_SUCCEEDED(resourceAllocator->CreateResource( + allocationDesc, CreateBasicBufferDesc(kDefaultBufferSize), D3D12_RESOURCE_STATE_COMMON, + nullptr, &allocation)); + ASSERT_NE(allocation, nullptr); + EXPECT_EQ(allocation->GetDebugName(), allocationDesc.DebugName); + } } TEST_F(D3D12ResourceAllocatorTests, CreateSmallTexture) {