[wgpu-core] split command encoders from command buffers

Author: teoxoy

deno_webgpu/command_encoder.rs

@@ -2,7 +2,6 @@
 
 use std::borrow::Cow;
 use std::cell::RefCell;
-use std::sync::atomic::{AtomicBool, Ordering};
 
 use deno_core::cppgc::Ptr;
 use deno_core::op2;
@@ -29,19 +28,11 @@ pub struct GPUCommandEncoder {
 
     pub id: wgpu_core::id::CommandEncoderId,
     pub label: String,
-
-    pub finished: AtomicBool,
 }
 
 impl Drop for GPUCommandEncoder {
     fn drop(&mut self) {
-        // Command encoders and command buffers are both the same wgpu object.
-        // At the time `finished` is set, ownership of the id (and
-        // responsibility for dropping it) transfers from the encoder to the
-        // buffer.
-        if !self.finished.load(Ordering::SeqCst) {
-            self.instance.command_encoder_drop(self.id);
-        }
+        self.instance.command_encoder_drop(self.id);
     }
 }
 
@@ -416,34 +407,22 @@ impl GPUCommandEncoder {
     fn finish(
         &self,
         #[webidl] descriptor: crate::command_buffer::GPUCommandBufferDescriptor,
-    ) -> Result<GPUCommandBuffer, JsErrorBox> {
+    ) -> GPUCommandBuffer {
         let wgpu_descriptor = wgpu_types::CommandBufferDescriptor {
             label: crate::transform_label(descriptor.label.clone()),
         };
 
-        // TODO(https://github.com/gfx-rs/wgpu/issues/7812): This is not right,
-        // it should be a validation error, and it would be nice if we can just
-        // let wgpu generate it for us. The problem is that if the encoder was
-        // already finished, we transferred ownership of the id to a command
-        // buffer, so we have to bail out before we mint a duplicate command
-        // buffer with the same id below.
-        if self.finished.fetch_or(true, Ordering::SeqCst) {
-            return Err(JsErrorBox::type_error(
-                "The command encoder has already finished.",
-            ));
-        }
-
         let (id, err) = self
             .instance
-            .command_encoder_finish(self.id, &wgpu_descriptor);
+            .command_encoder_finish(self.id, &wgpu_descriptor, None);
 
         self.error_handler.push_error(err);
 
-        Ok(GPUCommandBuffer {
+        GPUCommandBuffer {
             instance: self.instance.clone(),
             id,
             label: descriptor.label,
-        })
+        }
     }
 
     fn push_debug_group(&self, #[webidl] group_label: String) {

deno_webgpu/device.rs

@@ -498,7 +498,6 @@ impl GPUDevice {
             error_handler: self.error_handler.clone(),
             id,
             label,
-            finished: Default::default(),
         }
     }
 

player/src/bin/play.rs

@@ -7,6 +7,7 @@ fn main() {
 
     use player::GlobalPlay as _;
     use wgc::device::trace;
+    use wgpu_core::identity::IdentityManager;
 
     use std::{
         fs,
@@ -52,7 +53,8 @@ fn main() {
         .unwrap();
 
     let global = wgc::global::Global::new("player", &wgt::InstanceDescriptor::default());
-    let mut command_buffer_id_manager = wgc::identity::IdentityManager::new();
+    let mut command_encoder_id_manager = IdentityManager::new();
+    let mut command_buffer_id_manager = IdentityManager::new();
 
     #[cfg(feature = "winit")]
     let surface = unsafe {
@@ -102,7 +104,14 @@ fn main() {
         unsafe { global.device_start_graphics_debugger_capture(device) };
 
         while let Some(action) = actions.pop() {
-            global.process(device, queue, action, &dir, &mut command_buffer_id_manager);
+            global.process(
+                device,
+                queue,
+                action,
+                &dir,
+                &mut command_encoder_id_manager,
+                &mut command_buffer_id_manager,
+            );
         }
 
         unsafe { global.device_stop_graphics_debugger_capture(device) };
@@ -164,6 +173,7 @@ fn main() {
                                         queue,
                                         action,
                                         &dir,
+                                        &mut command_encoder_id_manager,
                                         &mut command_buffer_id_manager,
                                     );
                                 }

player/src/lib.rs

@@ -6,7 +6,7 @@
 extern crate wgpu_core as wgc;
 extern crate wgpu_types as wgt;
 
-use wgc::device::trace;
+use wgc::{device::trace, identity::IdentityManager};
 
 use std::{borrow::Cow, fs, path::Path};
 
@@ -15,14 +15,16 @@ pub trait GlobalPlay {
         &self,
         encoder: wgc::id::CommandEncoderId,
         commands: Vec<trace::Command>,
+        command_buffer_id_manager: &mut IdentityManager<wgc::id::markers::CommandBuffer>,
     ) -> wgc::id::CommandBufferId;
     fn process(
         &self,
         device: wgc::id::DeviceId,
         queue: wgc::id::QueueId,
         action: trace::Action,
         dir: &Path,
-        comb_manager: &mut wgc::identity::IdentityManager<wgc::id::markers::CommandBuffer>,
+        command_encoder_id_manager: &mut IdentityManager<wgc::id::markers::CommandEncoder>,
+        command_buffer_id_manager: &mut IdentityManager<wgc::id::markers::CommandBuffer>,
     );
 }
 
@@ -31,6 +33,7 @@ impl GlobalPlay for wgc::global::Global {
         &self,
         encoder: wgc::id::CommandEncoderId,
         commands: Vec<trace::Command>,
+        command_buffer_id_manager: &mut IdentityManager<wgc::id::markers::CommandBuffer>,
     ) -> wgc::id::CommandBufferId {
         for command in commands {
             match command {
@@ -172,8 +175,11 @@ impl GlobalPlay for wgc::global::Global {
                 }
             }
         }
-        let (cmd_buf, error) =
-            self.command_encoder_finish(encoder, &wgt::CommandBufferDescriptor { label: None });
+        let (cmd_buf, error) = self.command_encoder_finish(
+            encoder,
+            &wgt::CommandBufferDescriptor { label: None },
+            Some(command_buffer_id_manager.process()),
+        );
         if let Some(e) = error {
             panic!("{e}");
         }
@@ -186,7 +192,8 @@ impl GlobalPlay for wgc::global::Global {
         queue: wgc::id::QueueId,
         action: trace::Action,
         dir: &Path,
-        comb_manager: &mut wgc::identity::IdentityManager<wgc::id::markers::CommandBuffer>,
+        command_encoder_id_manager: &mut IdentityManager<wgc::id::markers::CommandEncoder>,
+        command_buffer_id_manager: &mut IdentityManager<wgc::id::markers::CommandBuffer>,
     ) {
         use wgc::device::trace::Action;
         log::debug!("action {action:?}");
@@ -379,12 +386,12 @@ impl GlobalPlay for wgc::global::Global {
                 let (encoder, error) = self.device_create_command_encoder(
                     device,
                     &wgt::CommandEncoderDescriptor { label: None },
-                    Some(comb_manager.process().into_command_encoder_id()),
+                    Some(command_encoder_id_manager.process()),
                 );
                 if let Some(e) = error {
                     panic!("{e}");
                 }
-                let cmdbuf = self.encode_commands(encoder, commands);
+                let cmdbuf = self.encode_commands(encoder, commands, command_buffer_id_manager);
                 self.queue_submit(queue, &[cmdbuf]).unwrap();
             }
             Action::CreateBlas { id, desc, sizes } => {

player/tests/player/main.rs

@@ -20,6 +20,7 @@ use std::{
     path::{Path, PathBuf},
     slice,
 };
+use wgc::identity::IdentityManager;
 
 #[derive(serde::Deserialize)]
 struct RawId {
@@ -104,14 +105,16 @@ impl Test<'_> {
             panic!("{e:?}");
         }
 
-        let mut command_buffer_id_manager = wgc::identity::IdentityManager::new();
+        let mut command_encoder_id_manager = IdentityManager::new();
+        let mut command_buffer_id_manager = IdentityManager::new();
         println!("\t\t\tRunning...");
         for action in self.actions {
             global.process(
                 device_id,
                 queue_id,
                 action,
                 dir,
+                &mut command_encoder_id_manager,
                 &mut command_buffer_id_manager,
             );
         }

tests/tests/wgpu-gpu/mem_leaks.rs

@@ -179,7 +179,7 @@ async fn draw_test_with_reports(
 
     let global_report = ctx.instance.generate_report().unwrap();
     let report = global_report.hub_report();
-    assert_eq!(report.command_buffers.num_allocated, 1);
+    assert_eq!(report.command_encoders.num_allocated, 1);
     assert_eq!(report.buffers.num_allocated, 1);
 
     let mut rpass = encoder.begin_render_pass(&wgpu::RenderPassDescriptor {
@@ -206,7 +206,7 @@ async fn draw_test_with_reports(
     assert_eq!(report.pipeline_layouts.num_allocated, 1);
     assert_eq!(report.render_pipelines.num_allocated, 1);
     assert_eq!(report.compute_pipelines.num_allocated, 0);
-    assert_eq!(report.command_buffers.num_allocated, 1);
+    assert_eq!(report.command_encoders.num_allocated, 1);
     assert_eq!(report.render_bundles.num_allocated, 0);
     assert_eq!(report.texture_views.num_allocated, 1);
     assert_eq!(report.textures.num_allocated, 1);
@@ -223,15 +223,15 @@ async fn draw_test_with_reports(
 
     let global_report = ctx.instance.generate_report().unwrap();
     let report = global_report.hub_report();
-    assert_eq!(report.command_buffers.num_kept_from_user, 1);
+    assert_eq!(report.command_encoders.num_kept_from_user, 1);
     assert_eq!(report.render_pipelines.num_kept_from_user, 0);
     assert_eq!(report.pipeline_layouts.num_kept_from_user, 0);
     assert_eq!(report.bind_group_layouts.num_kept_from_user, 0);
     assert_eq!(report.bind_groups.num_kept_from_user, 0);
     assert_eq!(report.buffers.num_kept_from_user, 0);
     assert_eq!(report.texture_views.num_kept_from_user, 0);
     assert_eq!(report.textures.num_kept_from_user, 0);
-    assert_eq!(report.command_buffers.num_allocated, 1);
+    assert_eq!(report.command_encoders.num_allocated, 1);
     assert_eq!(report.render_pipelines.num_allocated, 0);
     assert_eq!(report.pipeline_layouts.num_allocated, 0);
     assert_eq!(report.bind_group_layouts.num_allocated, 0);
@@ -240,12 +240,18 @@ async fn draw_test_with_reports(
     assert_eq!(report.texture_views.num_allocated, 0);
     assert_eq!(report.textures.num_allocated, 0);
 
-    let submit_index = ctx.queue.submit(Some(encoder.finish()));
+    let command_buffer = encoder.finish();
+
+    let global_report = ctx.instance.generate_report().unwrap();
+    let report = global_report.hub_report();
+    assert_eq!(report.command_encoders.num_allocated, 0);
+    assert_eq!(report.command_buffers.num_allocated, 1);
+
+    let submit_index = ctx.queue.submit(Some(command_buffer));
 
-    // TODO: fix in https://github.com/gfx-rs/wgpu/pull/5141
-    // let global_report = ctx.instance.generate_report().unwrap();
-    // let report = global_report.hub_report();
-    // assert_eq!(report.command_buffers.num_allocated, 0);
+    let global_report = ctx.instance.generate_report().unwrap();
+    let report = global_report.hub_report();
+    assert_eq!(report.command_buffers.num_allocated, 0);
 
     ctx.async_poll(wgpu::PollType::wait_for(submit_index))
         .await

wgpu-core/src/as_hal.rs

@@ -346,7 +346,7 @@ impl Global {
 
         let hub = &self.hub;
 
-        let cmd_buf = hub.command_buffers.get(id.into_command_buffer_id());
+        let cmd_buf = hub.command_encoders.get(id);
         let mut cmd_buf_data = cmd_buf.data.lock();
         cmd_buf_data.record_as_hal_mut(|opt_cmd_buf| -> R {
             hal_command_encoder_callback(opt_cmd_buf.and_then(|cmd_buf| {

wgpu-core/src/command/clear.rs

@@ -6,7 +6,7 @@ use crate::device::trace::Command as TraceCommand;
 use crate::{
     api_log,
     command::EncoderStateError,
-    device::DeviceError,
+    device::{DeviceError, MissingFeatures},
     get_lowest_common_denom,
     global::Global,
     id::{BufferId, CommandEncoderId, TextureId},
@@ -30,10 +30,10 @@ use wgt::{
 #[derive(Clone, Debug, Error)]
 #[non_exhaustive]
 pub enum ClearError {
-    #[error("To use clear_texture the CLEAR_TEXTURE feature needs to be enabled")]
-    MissingClearTextureFeature,
     #[error(transparent)]
     DestroyedResource(#[from] DestroyedResourceError),
+    #[error(transparent)]
+    MissingFeatures(#[from] MissingFeatures),
     #[error("{0} can not be cleared")]
     NoValidTextureClearMode(ResourceErrorIdent),
     #[error("Buffer clear size {0:?} is not a multiple of `COPY_BUFFER_ALIGNMENT`")]
@@ -84,12 +84,12 @@ impl WebGpuError for ClearError {
     fn webgpu_error_type(&self) -> ErrorType {
         let e: &dyn WebGpuError = match self {
             Self::DestroyedResource(e) => e,
+            Self::MissingFeatures(e) => e,
             Self::MissingBufferUsage(e) => e,
             Self::Device(e) => e,
             Self::EncoderState(e) => e,
             Self::InvalidResource(e) => e,
             Self::NoValidTextureClearMode(..)
-            | Self::MissingClearTextureFeature
             | Self::UnalignedFillSize(..)
             | Self::UnalignedBufferOffset(..)
             | Self::OffsetPlusSizeExceeds64BitBounds { .. }
@@ -115,9 +115,7 @@ impl Global {
 
         let hub = &self.hub;
 
-        let cmd_buf = hub
-            .command_buffers
-            .get(command_encoder_id.into_command_buffer_id());
+        let cmd_buf = hub.command_encoders.get(command_encoder_id);
         let mut cmd_buf_data = cmd_buf.data.lock();
         cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), ClearError> {
             #[cfg(feature = "trace")]
@@ -202,9 +200,7 @@ impl Global {
 
         let hub = &self.hub;
 
-        let cmd_buf = hub
-            .command_buffers
-            .get(command_encoder_id.into_command_buffer_id());
+        let cmd_buf = hub.command_encoders.get(command_encoder_id);
         let mut cmd_buf_data = cmd_buf.data.lock();
         cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), ClearError> {
             #[cfg(feature = "trace")]
@@ -217,9 +213,9 @@ impl Global {
 
             cmd_buf.device.check_is_valid()?;
 
-            if !cmd_buf.support_clear_texture {
-                return Err(ClearError::MissingClearTextureFeature);
-            }
+            cmd_buf
+                .device
+                .require_features(wgt::Features::CLEAR_TEXTURE)?;
 
             let dst_texture = hub.textures.get(dst).get()?;