Skip to content

[wgpu-core] remove implicit PL & BGL IDs from pipeline creation #7967

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.

Sign up for GitHub

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
ErichDonGubler merged 1 commit into from
Jul 18, 2025
Merged

[wgpu-core] remove implicit PL & BGL IDs from pipeline creation #7967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions deno_webgpu/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ impl GPUDevice {

let (id, err) =
self.instance
.device_create_compute_pipeline(self.id, &wgpu_descriptor, None, None);
.device_create_compute_pipeline(self.id, &wgpu_descriptor, None);

self.error_handler.push_error(err);

Expand Down Expand Up @@ -818,7 +818,7 @@ impl GPUDevice {

let (id, err) =
self.instance
.device_create_render_pipeline(self.id, &wgpu_descriptor, None, None);
.device_create_render_pipeline(self.id, &wgpu_descriptor, None);

self.error_handler.push_error(err);

Expand Down
32 changes: 4 additions & 28 deletions player/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,41 +298,17 @@ impl GlobalPlay for wgc::global::Global {
Action::DestroyShaderModule(id) => {
self.shader_module_drop(id);
}
Action::CreateComputePipeline {
id,
desc,
implicit_context,
} => {
let implicit_ids =
implicit_context
.as_ref()
.map(|ic| wgc::device::ImplicitPipelineIds {
root_id: ic.root_id,
group_ids: &ic.group_ids,
});
let (_, error) =
self.device_create_compute_pipeline(device, &desc, Some(id), implicit_ids);
Action::CreateComputePipeline { id, desc } => {
let (_, error) = self.device_create_compute_pipeline(device, &desc, Some(id));
if let Some(e) = error {
panic!("{e}");
}
}
Action::DestroyComputePipeline(id) => {
self.compute_pipeline_drop(id);
}
Action::CreateRenderPipeline {
id,
desc,
implicit_context,
} => {
let implicit_ids =
implicit_context
.as_ref()
.map(|ic| wgc::device::ImplicitPipelineIds {
root_id: ic.root_id,
group_ids: &ic.group_ids,
});
let (_, error) =
self.device_create_render_pipeline(device, &desc, Some(id), implicit_ids);
Action::CreateRenderPipeline { id, desc } => {
let (_, error) = self.device_create_render_pipeline(device, &desc, Some(id));
if let Some(e) = error {
panic!("{e}");
}
Expand Down
114 changes: 1 addition & 113 deletions wgpu-core/src/device/global.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::{

use wgt::{BufferAddress, TextureFormat};

use super::{ImplicitPipelineIds, UserClosures};
use super::UserClosures;

impl Global {
pub fn adapter_is_surface_supported(
Expand Down Expand Up @@ -1228,7 +1228,6 @@ impl Global {
device_id: DeviceId,
desc: &pipeline::RenderPipelineDescriptor,
id_in: Option<id::RenderPipelineId>,
implicit_pipeline_ids: Option<ImplicitPipelineIds<'_>>,
) -> (
id::RenderPipelineId,
Option<pipeline::CreateRenderPipelineError>,
Expand All @@ -1237,26 +1236,16 @@ impl Global {

let hub = &self.hub;

let missing_implicit_pipeline_ids =
desc.layout.is_none() && id_in.is_some() && implicit_pipeline_ids.is_none();

let fid = hub.render_pipelines.prepare(id_in);
let implicit_context = implicit_pipeline_ids.map(|ipi| ipi.prepare(hub));

let error = 'error: {
if missing_implicit_pipeline_ids {
// TODO: categorize this error as API misuse
break 'error pipeline::ImplicitLayoutError::MissingImplicitPipelineIds.into();
}

let device = self.hub.devices.get(device_id);

#[cfg(feature = "trace")]
if let Some(ref mut trace) = *device.trace.lock() {
trace.add(trace::Action::CreateRenderPipeline {
id: fid.id(),
desc: desc.clone(),
implicit_context: implicit_context.clone(),
});
}

Expand Down Expand Up @@ -1357,40 +1346,6 @@ impl Global {
Err(e) => break 'error e,
};

if let Some(ids) = implicit_context.as_ref() {
let group_count = pipeline.layout.bind_group_layouts.len();
if ids.group_ids.len() < group_count {
log::error!(
"Not enough bind group IDs ({}) specified for the implicit layout ({})",
ids.group_ids.len(),
group_count
);
// TODO: categorize this error as API misuse
break 'error pipeline::ImplicitLayoutError::MissingIds(group_count as _)
.into();
}

let mut pipeline_layout_guard = hub.pipeline_layouts.write();
let mut bgl_guard = hub.bind_group_layouts.write();
pipeline_layout_guard.insert(ids.root_id, Fallible::Valid(pipeline.layout.clone()));
let mut group_ids = ids.group_ids.iter();
// NOTE: If the first iterator is longer than the second, the `.zip()` impl will still advance the
// the first iterator before realizing that the second iterator has finished.
// The `pipeline.layout.bind_group_layouts` iterator will always be shorter than `ids.group_ids`,
// so using it as the first iterator for `.zip()` will work properly.
for (bgl, bgl_id) in pipeline
.layout
.bind_group_layouts
.iter()
.zip(&mut group_ids)
{
bgl_guard.insert(*bgl_id, Fallible::Valid(bgl.clone()));
}
for bgl_id in group_ids {
bgl_guard.insert(*bgl_id, Fallible::Invalid(Arc::new(String::new())));
}
}

let id = fid.assign(Fallible::Valid(pipeline));
api_log!("Device::create_render_pipeline -> {id:?}");

Expand All @@ -1399,17 +1354,6 @@ impl Global {

let id = fid.assign(Fallible::Invalid(Arc::new(desc.label.to_string())));

// We also need to assign errors to the implicit pipeline layout and the
// implicit bind group layouts.
if let Some(ids) = implicit_context {
let mut pipeline_layout_guard = hub.pipeline_layouts.write();
let mut bgl_guard = hub.bind_group_layouts.write();
pipeline_layout_guard.insert(ids.root_id, Fallible::Invalid(Arc::new(String::new())));
for bgl_id in ids.group_ids {
bgl_guard.insert(bgl_id, Fallible::Invalid(Arc::new(String::new())));
}
}

(id, Some(error))
}

Expand Down Expand Up @@ -1467,7 +1411,6 @@ impl Global {
device_id: DeviceId,
desc: &pipeline::ComputePipelineDescriptor,
id_in: Option<id::ComputePipelineId>,
implicit_pipeline_ids: Option<ImplicitPipelineIds<'_>>,
) -> (
id::ComputePipelineId,
Option<pipeline::CreateComputePipelineError>,
Expand All @@ -1476,26 +1419,16 @@ impl Global {

let hub = &self.hub;

let missing_implicit_pipeline_ids =
desc.layout.is_none() && id_in.is_some() && implicit_pipeline_ids.is_none();

let fid = hub.compute_pipelines.prepare(id_in);
let implicit_context = implicit_pipeline_ids.map(|ipi| ipi.prepare(hub));

let error = 'error: {
if missing_implicit_pipeline_ids {
// TODO: categorize this error as API misuse
break 'error pipeline::ImplicitLayoutError::MissingImplicitPipelineIds.into();
}

let device = self.hub.devices.get(device_id);

#[cfg(feature = "trace")]
if let Some(ref mut trace) = *device.trace.lock() {
trace.add(trace::Action::CreateComputePipeline {
id: fid.id(),
desc: desc.clone(),
implicit_context: implicit_context.clone(),
});
}

Expand Down Expand Up @@ -1545,40 +1478,6 @@ impl Global {
Err(e) => break 'error e,
};

if let Some(ids) = implicit_context.as_ref() {
let group_count = pipeline.layout.bind_group_layouts.len();
if ids.group_ids.len() < group_count {
log::error!(
"Not enough bind group IDs ({}) specified for the implicit layout ({})",
ids.group_ids.len(),
group_count
);
// TODO: categorize this error as API misuse
break 'error pipeline::ImplicitLayoutError::MissingIds(group_count as _)
.into();
}

let mut pipeline_layout_guard = hub.pipeline_layouts.write();
let mut bgl_guard = hub.bind_group_layouts.write();
pipeline_layout_guard.insert(ids.root_id, Fallible::Valid(pipeline.layout.clone()));
let mut group_ids = ids.group_ids.iter();
// NOTE: If the first iterator is longer than the second, the `.zip()` impl will still advance the
// the first iterator before realizing that the second iterator has finished.
// The `pipeline.layout.bind_group_layouts` iterator will always be shorter than `ids.group_ids`,
// so using it as the first iterator for `.zip()` will work properly.
for (bgl, bgl_id) in pipeline
.layout
.bind_group_layouts
.iter()
.zip(&mut group_ids)
{
bgl_guard.insert(*bgl_id, Fallible::Valid(bgl.clone()));
}
for bgl_id in group_ids {
bgl_guard.insert(*bgl_id, Fallible::Invalid(Arc::new(String::new())));
}
}

let id = fid.assign(Fallible::Valid(pipeline));
api_log!("Device::create_compute_pipeline -> {id:?}");

Expand All @@ -1587,17 +1486,6 @@ impl Global {

let id = fid.assign(Fallible::Invalid(Arc::new(desc.label.to_string())));

// We also need to assign errors to the implicit pipeline layout and the
// implicit bind group layouts.
if let Some(ids) = implicit_context {
let mut pipeline_layout_guard = hub.pipeline_layouts.write();
let mut bgl_guard = hub.bind_group_layouts.write();
pipeline_layout_guard.insert(ids.root_id, Fallible::Invalid(Arc::new(String::new())));
for bgl_id in ids.group_ids {
bgl_guard.insert(bgl_id, Fallible::Invalid(Arc::new(String::new())));
}
}

(id, Some(error))
}

Expand Down
27 changes: 0 additions & 27 deletions wgpu-core/src/device/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ use core::{fmt, num::NonZeroU32};

use crate::{
binding_model,
hub::Hub,
id::{BindGroupLayoutId, PipelineLayoutId},
ray_tracing::BlasCompactReadyPendingClosure,
resource::{
Buffer, BufferAccessError, BufferAccessResult, BufferMapOperation, Labeled,
Expand Down Expand Up @@ -385,31 +383,6 @@ impl WebGpuError for MissingDownlevelFlags {
}
}

#[derive(Clone, Debug)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub struct ImplicitPipelineContext {
pub root_id: PipelineLayoutId,
pub group_ids: ArrayVec<BindGroupLayoutId, { hal::MAX_BIND_GROUPS }>,
}

pub struct ImplicitPipelineIds<'a> {
pub root_id: PipelineLayoutId,
pub group_ids: &'a [BindGroupLayoutId],
}

impl ImplicitPipelineIds<'_> {
fn prepare(self, hub: &Hub) -> ImplicitPipelineContext {
ImplicitPipelineContext {
root_id: hub.pipeline_layouts.prepare(Some(self.root_id)).id(),
group_ids: self
.group_ids
.iter()
.map(|id_in| hub.bind_group_layouts.prepare(Some(*id_in)).id())
.collect(),
}
}
}

/// Create a validator with the given validation flags.
pub fn create_validator(
features: wgt::Features,
Expand Down
4 changes: 0 additions & 4 deletions wgpu-core/src/device/trace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,11 @@ pub enum Action<'a> {
CreateComputePipeline {
id: id::ComputePipelineId,
desc: crate::pipeline::ComputePipelineDescriptor<'a>,
#[cfg_attr(feature = "replay", serde(default))]
implicit_context: Option<super::ImplicitPipelineContext>,
},
DestroyComputePipeline(id::ComputePipelineId),
CreateRenderPipeline {
id: id::RenderPipelineId,
desc: crate::pipeline::RenderPipelineDescriptor<'a>,
#[cfg_attr(feature = "replay", serde(default))]
implicit_context: Option<super::ImplicitPipelineContext>,
},
DestroyRenderPipeline(id::RenderPipelineId),
CreatePipelineCache {
Expand Down
8 changes: 1 addition & 7 deletions wgpu-core/src/pipeline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,6 @@ pub type ImplicitBindGroupCount = u8;
#[derive(Clone, Debug, Error)]
#[non_exhaustive]
pub enum ImplicitLayoutError {
#[error("The implicit_pipeline_ids arg is required")]
MissingImplicitPipelineIds,
#[error("Missing IDs for deriving {0} bind groups")]
MissingIds(ImplicitBindGroupCount),
#[error("Unable to reflect the shader {0:?} interface")]
ReflectionError(wgt::ShaderStages),
#[error(transparent)]
Expand All @@ -205,9 +201,7 @@ pub enum ImplicitLayoutError {
impl WebGpuError for ImplicitLayoutError {
fn webgpu_error_type(&self) -> ErrorType {
let e: &dyn WebGpuError = match self {
Self::MissingImplicitPipelineIds | Self::MissingIds(_) | Self::ReflectionError(_) => {
return ErrorType::Validation
}
Self::ReflectionError(_) => return ErrorType::Validation,
Self::BindGroup(e) => e,
Self::Pipeline(e) => e,
};
Expand Down
7 changes: 2 additions & 5 deletions wgpu-core/src/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use alloc::sync::Arc;
use crate::{
id::Id,
identity::IdentityManager,
lock::{rank, RwLock, RwLockReadGuard, RwLockWriteGuard},
lock::{rank, RwLock, RwLockReadGuard},
storage::{Element, Storage, StorageItem},
};

Expand Down Expand Up @@ -55,6 +55,7 @@ pub(crate) struct FutureId<'a, T: StorageItem> {
}

impl<T: StorageItem> FutureId<'_, T> {
#[cfg(feature = "trace")]
pub fn id(&self) -> Id<T::Marker> {
self.id
}
Expand Down Expand Up @@ -87,10 +88,6 @@ impl<T: StorageItem> Registry<T> {
pub(crate) fn read<'a>(&'a self) -> RwLockReadGuard<'a, Storage<T>> {
self.storage.read()
}
#[track_caller]
pub(crate) fn write<'a>(&'a self) -> RwLockWriteGuard<'a, Storage<T>> {
self.storage.write()
}
pub(crate) fn remove(&self, id: Id<T::Marker>) -> T {
let value = self.storage.write().remove(id);
// This needs to happen *after* removing it from the storage, to maintain the
Expand Down
16 changes: 8 additions & 8 deletions wgpu/src/backend/wgpu_core.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1321,10 +1321,10 @@ impl dispatch::DeviceInterface for CoreDevice {
cache: desc.cache.map(|cache| cache.inner.as_core().id),
};

let (id, error) =
self.context
.0
.device_create_render_pipeline(self.id, &descriptor, None, None);
let (id, error) = self
.context
.0
.device_create_render_pipeline(self.id, &descriptor, None);
if let Some(cause) = error {
if let wgc::pipeline::CreateRenderPipelineError::Internal { stage, ref error } = cause {
log::error!("Shader translation error for stage {:?}: {}", stage, error);
Expand Down Expand Up @@ -1372,10 +1372,10 @@ impl dispatch::DeviceInterface for CoreDevice {
cache: desc.cache.map(|cache| cache.inner.as_core().id),
};

let (id, error) =
self.context
.0
.device_create_compute_pipeline(self.id, &descriptor, None, None);
let (id, error) = self
.context
.0
.device_create_compute_pipeline(self.id, &descriptor, None);
if let Some(cause) = error {
if let wgc::pipeline::CreateComputePipelineError::Internal(ref error) = cause {
log::error!(
Expand Down