Resource ownership cleanup (#151)

* Get rid of manually drop for dx12 resources.
Warn the user when a resource is not freed before dropping.
Adjust some comments and function signatures based on feedback.

* Expect dx12 resources to be present when freeing the allocation.
Print information about the resource when the user forgets to free one.

* Don't rely on dx12 functions to extract a resource name, and instead store it in the resource manually.

Co-authored-by: Marijn Suijten <[email protected]>

Author: janie177

src/d3d12/mod.rs

@@ -1,8 +1,8 @@
 #![deny(clippy::unimplemented, clippy::unwrap_used, clippy::ok_expect)]
 
-use std::{fmt, mem::ManuallyDrop};
+use std::fmt;
 
-use log::{debug, Level};
+use log::{debug, warn, Level};
 
 use windows::Win32::{Foundation::E_OUTOFMEMORY, Graphics::Direct3D12::*};
 
@@ -233,18 +233,6 @@ pub struct AllocatorCreateDesc {
     pub debug_settings: AllocatorDebugSettings,
 }
 
-#[derive(Debug)]
-pub struct Allocation {
-    chunk_id: Option<std::num::NonZeroU64>,
-    offset: u64,
-    size: u64,
-    memory_block_index: usize,
-    memory_type_index: usize,
-    heap: ID3D12Heap,
-
-    name: Option<Box<str>>,
-}
-
 pub enum ResourceType<'a> {
     /// Allocation equivalent to Dx12's CommittedResource.
     Committed {
@@ -257,19 +245,46 @@ pub enum ResourceType<'a> {
 
 #[derive(Debug)]
 pub struct Resource {
+    name: String,
     pub allocation: Option<Allocation>,
-    pub resource: ManuallyDrop<ID3D12Resource>,
+    resource: Option<ID3D12Resource>,
     pub memory_location: MemoryLocation,
     memory_type_index: Option<usize>,
     pub size: u64,
 }
 
+impl Resource {
+    pub fn resource(&self) -> &ID3D12Resource {
+        self.resource.as_ref().expect("Resource was already freed.")
+    }
+}
+
+impl Drop for Resource {
+    fn drop(&mut self) {
+        if self.resource.is_some() {
+            warn!("Dropping resource `{}` that was not freed. Call `Allocator::free_resource(resource)` instead.", self.name);
+        }
+    }
+}
+
 #[derive(Debug)]
 pub struct CommittedAllocationStatistics {
     pub num_allocations: usize,
     pub total_size: u64,
 }
 
+#[derive(Debug)]
+pub struct Allocation {
+    chunk_id: Option<std::num::NonZeroU64>,
+    offset: u64,
+    size: u64,
+    memory_block_index: usize,
+    memory_type_index: usize,
+    heap: ID3D12Heap,
+
+    name: Option<Box<str>>,
+}
+
 impl Allocation {
     pub fn chunk_id(&self) -> Option<std::num::NonZeroU64> {
         self.chunk_id
@@ -813,8 +828,9 @@ impl Allocator {
                     memory_type.committed_allocations.total_size += allocation_info.SizeInBytes;
 
                     Ok(Resource {
+                        name: desc.name.into(),
                         allocation: None,
-                        resource: ManuallyDrop::new(resource),
+                        resource: Some(resource),
                         size: allocation_info.SizeInBytes,
                         memory_location: desc.memory_location,
                         memory_type_index: Some(memory_type.memory_type_index),
@@ -856,8 +872,9 @@ impl Allocator {
                         result.expect("Allocation succeeded but no resource was returned?");
                     let size = allocation.size();
                     Ok(Resource {
+                        name: desc.name.into(),
                         allocation: Some(allocation),
-                        resource: ManuallyDrop::new(resource),
+                        resource: Some(resource),
                         size,
                         memory_location: desc.memory_location,
                         memory_type_index: None,
@@ -868,13 +885,19 @@ impl Allocator {
     }
 
     /// Free a resource and its memory.
-    pub fn free_resource(&mut self, resource: Resource) -> Result<()> {
-        let _ = ManuallyDrop::into_inner(resource.resource);
-        if let Some(allocation) = resource.allocation {
+    pub fn free_resource(&mut self, mut resource: Resource) -> Result<()> {
+        // Explicitly drop the resource (which is backed by a refcounted COM object)
+        // before freeing allocated memory. Windows-rs performs a Release() on drop().
+        let _ = resource
+            .resource
+            .take()
+            .expect("Resource was already freed.");
+
+        if let Some(allocation) = resource.allocation.take() {
             self.free(allocation)
         } else {
-            // Committed resources are implicitly dropped when their refcount reaches 0.
-            // We only have to change the allocation and size tracking.
+            // Dx12 CommittedResources do not have an application managed allocation.
+            // We only have to update the tracked allocation count and memory usage.
             if let Some(memory_type_index) = resource.memory_type_index {
                 let memory_type = &mut self.memory_types[memory_type_index];
 

src/vulkan/mod.rs

@@ -17,14 +17,13 @@ use crate::{
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub enum AllocationScheme {
-    /// This allocation will be for a buffer, and it will be driver managed using vulkans DedicatedAllocation feature.
-    /// The driver will be able to perform optimizations in some cases with this type of allocation.
-    DedicatedBuffer { buffer: vk::Buffer },
-    /// This allocation will be for a image, and it will be driver managed using vulkans DedicatedAllocation feature.
-    /// The driver will be able to perform optimizations in some cases with this type of allocation.
-    DedicatedImage { image: vk::Image },
-    /// This allocation will be managed by the GPU allocator.
-    /// It is possible that the allocation will reside in an allocation block shared with other resources.
+    /// Perform a dedicated, driver-managed allocation for the given buffer, allowing
+    /// it to perform optimizations on this type of allocation.
+    DedicatedBuffer(vk::Buffer),
+    /// Perform a dedicated, driver-managed allocation for the given image, allowing
+    /// it to perform optimizations on this type of allocation.
+    DedicatedImage(vk::Image),
+    /// The memory for this resource will be allocated and managed by gpu-allocator.
     GpuAllocatorManaged,
 }
 
@@ -169,10 +168,10 @@ impl MemoryBlock {
         mem_type_index: usize,
         mapped: bool,
         buffer_device_address: bool,
-        allocation_scheme: &AllocationScheme,
+        allocation_scheme: AllocationScheme,
         requires_personal_block: bool,
     ) -> Result<Self> {
-        let dedicated_allocation = *allocation_scheme != AllocationScheme::GpuAllocatorManaged;
+        let dedicated_allocation = allocation_scheme != AllocationScheme::GpuAllocatorManaged;
         let device_memory = {
             let alloc_info = vk::MemoryAllocateInfo::builder()
                 .allocation_size(size)
@@ -190,12 +189,12 @@ impl MemoryBlock {
             // Flag the memory as dedicated if required.
             let mut dedicated_memory_info = vk::MemoryDedicatedAllocateInfo::builder();
             let alloc_info = match allocation_scheme {
-                AllocationScheme::DedicatedBuffer { buffer } => {
-                    dedicated_memory_info = dedicated_memory_info.buffer(*buffer);
+                AllocationScheme::DedicatedBuffer(buffer) => {
+                    dedicated_memory_info = dedicated_memory_info.buffer(buffer);
                     alloc_info.push_next(&mut dedicated_memory_info)
                 }
-                AllocationScheme::DedicatedImage { image } => {
-                    dedicated_memory_info = dedicated_memory_info.image(*image);
+                AllocationScheme::DedicatedImage(image) => {
+                    dedicated_memory_info = dedicated_memory_info.image(image);
                     alloc_info.push_next(&mut dedicated_memory_info)
                 }
                 AllocationScheme::GpuAllocatorManaged => alloc_info,
@@ -232,14 +231,14 @@ impl MemoryBlock {
             })
             .transpose()?;
 
-        let sub_allocator: Box<dyn allocator::SubAllocator> =
-            if !matches!(allocation_scheme, AllocationScheme::GpuAllocatorManaged)
-                || requires_personal_block
-            {
-                Box::new(allocator::DedicatedBlockAllocator::new(size))
-            } else {
-                Box::new(allocator::FreeListAllocator::new(size))
-            };
+        let sub_allocator: Box<dyn allocator::SubAllocator> = if allocation_scheme
+            != AllocationScheme::GpuAllocatorManaged
+            || requires_personal_block
+        {
+            Box::new(allocator::DedicatedBlockAllocator::new(size))
+        } else {
+            Box::new(allocator::FreeListAllocator::new(size))
+        };
 
         Ok(Self {
             device_memory,
@@ -310,7 +309,7 @@ impl MemoryType {
                 self.memory_type_index,
                 self.mappable,
                 self.buffer_device_address,
-                &desc.allocation_scheme,
+                desc.allocation_scheme,
                 requires_personal_block,
             )?;
 
@@ -408,7 +407,7 @@ impl MemoryType {
             self.memory_type_index,
             self.mappable,
             self.buffer_device_address,
-            &desc.allocation_scheme,
+            desc.allocation_scheme,
             false,
         )?;