remove hal::Device::destroy_command_encoder, use Drop implementations instead

Co-Authored-By: Erich Gubler <erichdongubler@gmail.com>

wgpu-core/src/command/allocator.rs

@@ -1,5 +1,3 @@
-use crate::resource_log;
-
 use crate::lock::{rank, Mutex};
 
 /// A pool of free [`wgpu_hal::CommandEncoder`]s, owned by a `Device`.
@@ -49,17 +47,4 @@ impl CommandAllocator {
         let mut free_encoders = self.free_encoders.lock();
         free_encoders.push(encoder);
     }
-
-    /// Free the pool of command encoders.
-    ///
-    /// This is only called when the `Device` is dropped.
-    pub(crate) fn dispose(&self, device: &dyn hal::DynDevice) {
-        let mut free_encoders = self.free_encoders.lock();
-        resource_log!("CommandAllocator::dispose encoders {}", free_encoders.len());
-        for cmd_encoder in free_encoders.drain(..) {
-            unsafe {
-                device.destroy_command_encoder(cmd_encoder);
-            }
-        }
-    }
 }

wgpu-core/src/command/mod.rs

@@ -388,14 +388,11 @@ impl CommandBufferMutable {
         }
     }
 
-    pub(crate) fn destroy(mut self, device: &Device) {
+    pub(crate) fn destroy(mut self) {
         self.encoder.discard();
         unsafe {
             self.encoder.raw.reset_all(self.encoder.list);
         }
-        unsafe {
-            device.raw().destroy_command_encoder(self.encoder.raw);
-        }
     }
 }
 
@@ -436,7 +433,7 @@ impl Drop for CommandBuffer {
     fn drop(&mut self) {
         resource_log!("Drop {}", self.error_ident());
         if let Some(data) = self.data.lock().take() {
-            data.destroy(&self.device);
+            data.destroy();
         }
     }
 }

wgpu-core/src/device/queue.rs

@@ -31,8 +31,7 @@ use smallvec::SmallVec;
 use crate::resource::{Blas, DestroyedAccelerationStructure, Tlas};
 use crate::scratch::ScratchBuffer;
 use std::{
-    iter,
-    mem::{self, ManuallyDrop},
+    iter, mem,
     ptr::NonNull,
     sync::{atomic::Ordering, Arc},
 };
@@ -42,9 +41,10 @@ use super::{life::LifetimeTracker, Device};
 
 pub struct Queue {
     raw: Box<dyn hal::DynQueue>,
-    pub(crate) device: Arc<Device>,
-    pub(crate) pending_writes: Mutex<ManuallyDrop<PendingWrites>>,
+    pub(crate) pending_writes: Mutex<PendingWrites>,
     life_tracker: Mutex<LifetimeTracker>,
+    // The device needs to be dropped last (`Device.zero_buffer` might be referenced by the encoder in pending writes).
+    pub(crate) device: Arc<Device>,
 }
 
 impl Queue {
@@ -86,15 +86,10 @@ impl Queue {
                 }]);
         }
 
-        let pending_writes = Mutex::new(
-            rank::QUEUE_PENDING_WRITES,
-            ManuallyDrop::new(pending_writes),
-        );
-
         Ok(Queue {
             raw,
             device,
-            pending_writes,
+            pending_writes: Mutex::new(rank::QUEUE_PENDING_WRITES, pending_writes),
             life_tracker: Mutex::new(rank::QUEUE_LIFE_TRACKER, LifetimeTracker::new()),
         })
     }
@@ -232,10 +227,6 @@ impl Drop for Queue {
             device_lost_invocations: SmallVec::new(),
         };
 
-        // SAFETY: We are in the Drop impl and we don't use self.pending_writes anymore after this point.
-        let pending_writes = unsafe { ManuallyDrop::take(&mut self.pending_writes.lock()) };
-        pending_writes.dispose(self.device.raw());
-
         closures.fire();
     }
 }
@@ -327,6 +318,7 @@ impl EncoderInFlight {
 /// All uses of [`StagingBuffer`]s end up here.
 #[derive(Debug)]
 pub(crate) struct PendingWrites {
+    // The command encoder needs to be destroyed before any other resource in pending writes.
     pub command_encoder: Box<dyn hal::DynCommandEncoder>,
 
     /// True if `command_encoder` is in the "recording" state, as
@@ -356,17 +348,6 @@ impl PendingWrites {
         }
     }
 
-    pub fn dispose(mut self, device: &dyn hal::DynDevice) {
-        unsafe {
-            if self.is_recording {
-                self.command_encoder.discard_encoding();
-            }
-            device.destroy_command_encoder(self.command_encoder);
-        }
-
-        self.temp_resources.clear();
-    }
-
     pub fn insert_buffer(&mut self, buffer: &Arc<Buffer>) {
         self.dst_buffers
             .insert(buffer.tracker_index(), buffer.clone());
@@ -460,6 +441,16 @@ impl PendingWrites {
     }
 }
 
+impl Drop for PendingWrites {
+    fn drop(&mut self) {
+        unsafe {
+            if self.is_recording {
+                self.command_encoder.discard_encoding();
+            }
+        }
+    }
+}
+
 #[derive(Clone, Debug, Error)]
 #[non_exhaustive]
 pub enum QueueWriteError {
@@ -1154,7 +1145,7 @@ impl Queue {
 
                         if first_error.is_some() {
                             if let Ok(cmd_buf_data) = cmd_buf_data {
-                                cmd_buf_data.destroy(&command_buffer.device);
+                                cmd_buf_data.destroy();
                             }
                             continue;
                         }
@@ -1171,7 +1162,7 @@ impl Queue {
                                 );
                                 if let Err(err) = res {
                                     first_error.get_or_insert(err);
-                                    cmd_buf_data.destroy(&command_buffer.device);
+                                    cmd_buf_data.destroy();
                                     continue;
                                 }
                                 cmd_buf_data.into_baked_commands()

wgpu-core/src/device/resource.rs

@@ -153,7 +153,6 @@ impl Drop for Device {
         let zero_buffer = unsafe { ManuallyDrop::take(&mut self.zero_buffer) };
         // SAFETY: We are in the Drop impl and we don't use self.fence anymore after this point.
         let fence = unsafe { ManuallyDrop::take(&mut self.fence.write()) };
-        self.command_allocator.dispose(self.raw.as_ref());
         #[cfg(feature = "indirect-validation")]
         self.indirect_validation
             .take()

wgpu-hal/examples/halmark/main.rs

@@ -559,7 +559,7 @@ impl<A: hal::Api> Example<A> {
 
             for mut ctx in self.contexts {
                 ctx.wait_and_clear(&self.device);
-                self.device.destroy_command_encoder(ctx.encoder);
+                drop(ctx.encoder);
                 self.device.destroy_fence(ctx.fence);
             }
 

wgpu-hal/examples/ray-traced-triangle/main.rs

@@ -1046,7 +1046,7 @@ impl<A: hal::Api> Example<A> {
 
             for mut ctx in self.contexts {
                 ctx.wait_and_clear(&self.device);
-                self.device.destroy_command_encoder(ctx.encoder);
+                drop(ctx.encoder);
                 self.device.destroy_fence(ctx.fence);
             }
 

wgpu-hal/src/dx12/command.rs

@@ -64,6 +64,7 @@ impl Drop for super::CommandEncoder {
     fn drop(&mut self) {
         use crate::CommandEncoder;
         unsafe { self.discard_encoding() }
+        self.counters.command_encoders.sub(1);
     }
 }
 

wgpu-hal/src/dx12/device.rs

@@ -746,13 +746,10 @@ impl crate::Device for super::Device {
             pass: super::PassState::new(),
             temp: super::Temp::default(),
             end_of_pass_timer_query: None,
+            counters: Arc::clone(&self.counters),
         })
     }
 
-    unsafe fn destroy_command_encoder(&self, _encoder: super::CommandEncoder) {
-        self.counters.command_encoders.sub(1);
-    }
-
     unsafe fn create_bind_group_layout(
         &self,
         desc: &crate::BindGroupLayoutDescriptor,
@@ -1908,7 +1905,7 @@ impl crate::Device for super::Device {
     }
 
     fn get_internal_counters(&self) -> wgt::HalCounters {
-        self.counters.clone()
+        self.counters.as_ref().clone()
     }
 
     fn generate_allocator_report(&self) -> Option<wgt::AllocatorReport> {

wgpu-hal/src/dx12/mod.rs

@@ -599,7 +599,7 @@ pub struct Device {
     null_rtv_handle: descriptor::Handle,
     mem_allocator: Mutex<suballocation::GpuAllocatorWrapper>,
     dxc_container: Option<Arc<shader_compilation::DxcContainer>>,
-    counters: wgt::HalCounters,
+    counters: Arc<wgt::HalCounters>,
 }
 
 impl Drop for Device {
@@ -722,6 +722,8 @@ pub struct CommandEncoder {
     /// If set, the end of the next render/compute pass will write a timestamp at
     /// the given pool & location.
     end_of_pass_timer_query: Option<(Direct3D12::ID3D12QueryHeap, u32)>,
+
+    counters: Arc<wgt::HalCounters>,
 }
 
 unsafe impl Send for CommandEncoder {}

wgpu-hal/src/dynamic/device.rs

@@ -58,7 +58,6 @@ pub trait DynDevice: DynResource {
         &self,
         desc: &CommandEncoderDescriptor<dyn DynQueue>,
     ) -> Result<Box<dyn DynCommandEncoder>, DeviceError>;
-    unsafe fn destroy_command_encoder(&self, pool: Box<dyn DynCommandEncoder>);
 
     unsafe fn create_bind_group_layout(
         &self,
@@ -268,10 +267,6 @@ impl<D: Device + DynResource> DynDevice for D {
             .map(|b| -> Box<dyn DynCommandEncoder> { Box::new(b) })
     }
 
-    unsafe fn destroy_command_encoder(&self, encoder: Box<dyn DynCommandEncoder>) {
-        unsafe { D::destroy_command_encoder(self, encoder.unbox()) };
-    }
-
     unsafe fn create_bind_group_layout(
         &self,
         desc: &BindGroupLayoutDescriptor,

wgpu-hal/src/empty.rs

@@ -206,7 +206,6 @@ impl crate::Device for Context {
     ) -> DeviceResult<Encoder> {
         Ok(Encoder)
     }
-    unsafe fn destroy_command_encoder(&self, encoder: Encoder) {}
 
     unsafe fn create_bind_group_layout(
         &self,

wgpu-hal/src/gles/command.rs

@@ -99,6 +99,7 @@ impl Drop for super::CommandEncoder {
     fn drop(&mut self) {
         use crate::CommandEncoder;
         unsafe { self.discard_encoding() }
+        self.counters.command_encoders.sub(1);
     }
 }
 

wgpu-hal/src/gles/device.rs

@@ -1116,13 +1116,10 @@ impl crate::Device for super::Device {
             cmd_buffer: super::CommandBuffer::default(),
             state: Default::default(),
             private_caps: self.shared.private_caps,
+            counters: Arc::clone(&self.counters),
         })
     }
 
-    unsafe fn destroy_command_encoder(&self, _encoder: super::CommandEncoder) {
-        self.counters.command_encoders.sub(1);
-    }
-
     unsafe fn create_bind_group_layout(
         &self,
         desc: &crate::BindGroupLayoutDescriptor,
@@ -1638,7 +1635,7 @@ impl crate::Device for super::Device {
     }
 
     fn get_internal_counters(&self) -> wgt::HalCounters {
-        self.counters.clone()
+        self.counters.as_ref().clone()
     }
 }
 

wgpu-hal/src/gles/mod.rs

@@ -292,7 +292,7 @@ pub struct Device {
     main_vao: glow::VertexArray,
     #[cfg(all(native, feature = "renderdoc"))]
     render_doc: crate::auxil::renderdoc::RenderDoc,
-    counters: wgt::HalCounters,
+    counters: Arc<wgt::HalCounters>,
 }
 
 impl Drop for Device {
@@ -1081,6 +1081,7 @@ pub struct CommandEncoder {
     cmd_buffer: CommandBuffer,
     state: command::State,
     private_caps: PrivateCapabilities,
+    counters: Arc<wgt::HalCounters>,
 }
 
 impl fmt::Debug for CommandEncoder {

wgpu-hal/src/lib.rs

@@ -842,7 +842,6 @@ pub trait Device: WasmNotSendSync {
         &self,
         desc: &CommandEncoderDescriptor<<Self::A as Api>::Queue>,
     ) -> Result<<Self::A as Api>::CommandEncoder, DeviceError>;
-    unsafe fn destroy_command_encoder(&self, pool: <Self::A as Api>::CommandEncoder);
 
     /// Creates a bind group layout.
     unsafe fn create_bind_group_layout(
@@ -1110,8 +1109,6 @@ pub trait Queue: WasmNotSendSync {
 /// - A `CommandBuffer` must not outlive the `CommandEncoder` that
 ///   built it.
 ///
-/// - A `CommandEncoder` must not outlive its `Device`.
-///
 /// It is the user's responsibility to meet this requirements. This
 /// allows `CommandEncoder` implementations to keep their state
 /// tracking to a minimum.

wgpu-hal/src/metal/command.rs

@@ -1295,5 +1295,6 @@ impl Drop for super::CommandEncoder {
         unsafe {
             self.discard_encoding();
         }
+        self.counters.command_encoders.sub(1);
     }
 }

wgpu-hal/src/metal/device.rs

@@ -585,13 +585,10 @@ impl crate::Device for super::Device {
             raw_cmd_buf: None,
             state: super::CommandState::default(),
             temp: super::Temp::default(),
+            counters: Arc::clone(&self.counters),
         })
     }
 
-    unsafe fn destroy_command_encoder(&self, _encoder: super::CommandEncoder) {
-        self.counters.command_encoders.sub(1);
-    }
-
     unsafe fn create_bind_group_layout(
         &self,
         desc: &crate::BindGroupLayoutDescriptor,
@@ -1438,6 +1435,6 @@ impl crate::Device for super::Device {
     }
 
     fn get_internal_counters(&self) -> wgt::HalCounters {
-        self.counters.clone()
+        self.counters.as_ref().clone()
     }
 }

wgpu-hal/src/metal/mod.rs

@@ -356,7 +356,7 @@ impl Queue {
 pub struct Device {
     shared: Arc<AdapterShared>,
     features: wgt::Features,
-    counters: wgt::HalCounters,
+    counters: Arc<wgt::HalCounters>,
 }
 
 pub struct Surface {
@@ -910,6 +910,7 @@ pub struct CommandEncoder {
     raw_cmd_buf: Option<metal::CommandBuffer>,
     state: CommandState,
     temp: Temp,
+    counters: Arc<wgt::HalCounters>,
 }
 
 impl fmt::Debug for CommandEncoder {

wgpu-hal/src/vulkan/device.rs

@@ -1373,19 +1373,9 @@ impl crate::Device for super::Device {
             discarded: Vec::new(),
             rpass_debug_marker_active: false,
             end_of_pass_timer_query: None,
+            counters: Arc::clone(&self.counters),
         })
     }
-    unsafe fn destroy_command_encoder(&self, cmd_encoder: super::CommandEncoder) {
-        unsafe {
-            // `vkDestroyCommandPool` also frees any command buffers allocated
-            // from that pool, so there's no need to explicitly call
-            // `vkFreeCommandBuffers` on `cmd_encoder`'s `free` and `discarded`
-            // fields.
-            self.shared.raw.destroy_command_pool(cmd_encoder.raw, None);
-        }
-
-        self.counters.command_encoders.sub(1);
-    }
 
     unsafe fn create_bind_group_layout(
         &self,
@@ -2556,7 +2546,7 @@ impl crate::Device for super::Device {
             .memory_allocations
             .set(self.shared.memory_allocations_counter.read());
 
-        self.counters.clone()
+        self.counters.as_ref().clone()
     }
 
     fn tlas_instance_to_bytes(&self, instance: TlasInstance) -> Vec<u8> {

wgpu-hal/src/vulkan/mod.rs

@@ -669,7 +669,7 @@ pub struct Device {
     naga_options: naga::back::spv::Options<'static>,
     #[cfg(feature = "renderdoc")]
     render_doc: crate::auxil::renderdoc::RenderDoc,
-    counters: wgt::HalCounters,
+    counters: Arc<wgt::HalCounters>,
 }
 
 impl Drop for Device {
@@ -918,6 +918,30 @@ pub struct CommandEncoder {
     /// If set, the end of the next render/compute pass will write a timestamp at
     /// the given pool & location.
     end_of_pass_timer_query: Option<(vk::QueryPool, u32)>,
+
+    counters: Arc<wgt::HalCounters>,
+}
+
+impl Drop for CommandEncoder {
+    fn drop(&mut self) {
+        // SAFETY:
+        //
+        // VUID-vkDestroyCommandPool-commandPool-00041: wgpu_hal requires that a
+        // `CommandBuffer` must live until its execution is complete, and that a
+        // `CommandBuffer` must not outlive the `CommandEncoder` that built it.
+        // Thus, we know that none of our `CommandBuffers` are in the "pending"
+        // state.
+        //
+        // The other VUIDs are pretty obvious.
+        unsafe {
+            // `vkDestroyCommandPool` also frees any command buffers allocated
+            // from that pool, so there's no need to explicitly call
+            // `vkFreeCommandBuffers` on `cmd_encoder`'s `free` and `discarded`
+            // fields.
+            self.device.raw.destroy_command_pool(self.raw, None);
+        }
+        self.counters.command_encoders.sub(1);
+    }
 }
 
 impl CommandEncoder {