Skip to content

[wgpu-core] split command encoders from command buffers #7979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 5 additions & 26 deletions deno_webgpu/command_encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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) {
Expand Down
1 change: 0 additions & 1 deletion deno_webgpu/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,6 @@ impl GPUDevice {
error_handler: self.error_handler.clone(),
id,
label,
finished: Default::default(),
}
}

Expand Down
14 changes: 12 additions & 2 deletions player/src/bin/play.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ fn main() {

use player::GlobalPlay as _;
use wgc::device::trace;
use wgpu_core::identity::IdentityManager;

use std::{
fs,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) };
Expand Down Expand Up @@ -164,6 +173,7 @@ fn main() {
queue,
action,
&dir,
&mut command_encoder_id_manager,
&mut command_buffer_id_manager,
);
}
Expand Down
21 changes: 14 additions & 7 deletions player/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -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>,
);
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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}");
}
Expand All @@ -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:?}");
Expand Down Expand Up @@ -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 } => {
Expand Down
5 changes: 4 additions & 1 deletion player/tests/player/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use std::{
path::{Path, PathBuf},
slice,
};
use wgc::identity::IdentityManager;

#[derive(serde::Deserialize)]
struct RawId {
Expand Down Expand Up @@ -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,
);
}
Expand Down
24 changes: 15 additions & 9 deletions tests/tests/wgpu-gpu/mem_leaks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions wgpu-core/src/as_hal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,8 @@ impl Global {

let hub = &self.hub;

let cmd_buf = hub.command_buffers.get(id.into_command_buffer_id());
let mut cmd_buf_data = cmd_buf.data.lock();
let cmd_enc = hub.command_encoders.get(id);
let mut cmd_buf_data = cmd_enc.data.lock();
cmd_buf_data.record_as_hal_mut(|opt_cmd_buf| -> R {
hal_command_encoder_callback(opt_cmd_buf.and_then(|cmd_buf| {
cmd_buf
Expand Down
36 changes: 16 additions & 20 deletions wgpu-core/src/command/clear.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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`")]
Expand Down Expand Up @@ -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 { .. }
Expand All @@ -115,21 +115,19 @@ impl Global {

let hub = &self.hub;

let cmd_buf = hub
.command_buffers
.get(command_encoder_id.into_command_buffer_id());
let mut cmd_buf_data = cmd_buf.data.lock();
let cmd_enc = hub.command_encoders.get(command_encoder_id);
let mut cmd_buf_data = cmd_enc.data.lock();
cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), ClearError> {
#[cfg(feature = "trace")]
if let Some(ref mut list) = cmd_buf_data.commands {
list.push(TraceCommand::ClearBuffer { dst, offset, size });
}

cmd_buf.device.check_is_valid()?;
cmd_enc.device.check_is_valid()?;

let dst_buffer = hub.buffers.get(dst).get()?;

dst_buffer.same_device_as(cmd_buf.as_ref())?;
dst_buffer.same_device_as(cmd_enc.as_ref())?;

let dst_pending = cmd_buf_data
.trackers
Expand Down Expand Up @@ -202,10 +200,8 @@ impl Global {

let hub = &self.hub;

let cmd_buf = hub
.command_buffers
.get(command_encoder_id.into_command_buffer_id());
let mut cmd_buf_data = cmd_buf.data.lock();
let cmd_enc = hub.command_encoders.get(command_encoder_id);
let mut cmd_buf_data = cmd_enc.data.lock();
cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), ClearError> {
#[cfg(feature = "trace")]
if let Some(ref mut list) = cmd_buf_data.commands {
Expand All @@ -215,15 +211,15 @@ impl Global {
});
}

cmd_buf.device.check_is_valid()?;
cmd_enc.device.check_is_valid()?;

if !cmd_buf.support_clear_texture {
return Err(ClearError::MissingClearTextureFeature);
}
cmd_enc
.device
.require_features(wgt::Features::CLEAR_TEXTURE)?;

let dst_texture = hub.textures.get(dst).get()?;

dst_texture.same_device_as(cmd_buf.as_ref())?;
dst_texture.same_device_as(cmd_enc.as_ref())?;

// Check if subresource aspects are valid.
let clear_aspects =
Expand Down Expand Up @@ -260,7 +256,7 @@ impl Global {
});
}

let device = &cmd_buf.device;
let device = &cmd_enc.device;
device.check_is_valid()?;
let (encoder, tracker) = cmd_buf_data.open_encoder_and_tracker()?;

Expand Down
Loading