From 64668ed5515b7db2b225c55618d79b63ae2b6d88 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Tue, 3 Oct 2023 10:38:43 +0200 Subject: [PATCH] Don't create a raw bind group layout for duplicates. --- wgpu-core/src/binding_model.rs | 53 ++++++++++++++++++++----- wgpu-core/src/command/bind.rs | 2 +- wgpu-core/src/device/global.rs | 34 ++++++++++------ wgpu-core/src/device/life.rs | 6 ++- wgpu-core/src/device/resource.rs | 68 +++++++++++++++++++++----------- wgpu-core/src/hub.rs | 6 ++- 6 files changed, 118 insertions(+), 51 deletions(-) diff --git a/wgpu-core/src/binding_model.rs b/wgpu-core/src/binding_model.rs index e9723fa60c..95ff7b506f 100644 --- a/wgpu-core/src/binding_model.rs +++ b/wgpu-core/src/binding_model.rs @@ -446,18 +446,25 @@ pub type BindGroupLayouts = crate::storage::Storage, BindG /// - produced bind groups /// - produced pipeline layouts /// - pipelines with implicit layouts -#[derive(Debug)] pub struct BindGroupLayout { - pub(crate) raw: A::BindGroupLayout, pub(crate) device_id: Stored, pub(crate) multi_ref_count: MultiRefCount, - pub(crate) entries: BindEntryMap, // When a layout created and there already exists a compatible layout the new layout // keeps a reference to the older compatible one. In some places we substitute the // bind group layout id with its compatible sibling. // Since this substitution can come at a cost, it is skipped when wgpu-core generates // its own resource IDs. - pub(crate) compatible_layout: Option>, + pub(crate) inner: BglOrDuplicate, +} + +pub(crate) enum BglOrDuplicate { + Inner(BindGroupLayoutInner), + Duplicate(Valid), +} + +pub struct BindGroupLayoutInner { + pub(crate) raw: A::BindGroupLayout, + pub(crate) entries: BindEntryMap, #[allow(unused)] pub(crate) dynamic_count: usize, pub(crate) count_validator: BindingTypeMaxCountValidator, @@ -465,6 +472,33 @@ pub struct BindGroupLayout { pub(crate) label: String, } +impl BindGroupLayout { + pub(crate) fn assume_deduplicated(&self) -> &BindGroupLayoutInner { + self.as_inner().unwrap() + } + + pub(crate) fn as_inner(&self) -> Option<&BindGroupLayoutInner> { + match self.inner { + BglOrDuplicate::Inner(ref inner) => Some(inner), + BglOrDuplicate::Duplicate(_) => None, + } + } + + pub(crate) fn into_inner(self) -> Option> { + match self.inner { + BglOrDuplicate::Inner(inner) => Some(inner), + BglOrDuplicate::Duplicate(_) => None, + } + } + + pub(crate) fn as_duplicate(&self) -> Option> { + match self.inner { + BglOrDuplicate::Duplicate(id) => Some(id), + BglOrDuplicate::Inner(_) => None, + } + } +} + impl Resource for BindGroupLayout { const TYPE: &'static str = "BindGroupLayout"; @@ -474,7 +508,7 @@ impl Resource for BindGroupLayout { fn label(&self) -> &str { #[cfg(debug_assertions)] - return &self.label; + return self.as_inner().map_or("", |inner| &inner.label); #[cfg(not(debug_assertions))] return ""; } @@ -486,8 +520,8 @@ pub(crate) fn try_get_bind_group_layout( id: BindGroupLayoutId, ) -> Option<&BindGroupLayout> { let layout = layouts.get(id).ok()?; - if let Some(compat) = layout.compatible_layout { - return Some(&layouts[compat]); + if let BglOrDuplicate::Duplicate(original_id) = layout.inner { + return Some(&layouts[original_id]); } Some(layout) @@ -499,9 +533,8 @@ pub(crate) fn get_bind_group_layout( ) -> (Valid, &BindGroupLayout) { let layout = &layouts[id]; layout - .compatible_layout - .map(|compat| (compat, &layouts[compat])) - .unwrap_or((id, layout)) + .as_duplicate() + .map_or((id, layout), |deduped| (deduped, &layouts[deduped])) } #[derive(Clone, Debug, Error)] diff --git a/wgpu-core/src/command/bind.rs b/wgpu-core/src/command/bind.rs index 05f90c6bc9..1cdec465e2 100644 --- a/wgpu-core/src/command/bind.rs +++ b/wgpu-core/src/command/bind.rs @@ -38,7 +38,7 @@ mod compat { } if let Some(id) = self.assigned { - return bind_group_layouts[id].compatible_layout == self.expected; + return bind_group_layouts[id].as_duplicate() == self.expected; } false diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index a9be65ae8f..b8817146a2 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -1,7 +1,8 @@ #[cfg(feature = "trace")] use crate::device::trace; use crate::{ - binding_model, command, conv, + binding_model::{self, BindGroupLayout}, + command, conv, device::{life::WaitIdleError, map_buffer, queue, Device, DeviceError, HostMap}, global::Global, hal_api::HalApi, @@ -1115,7 +1116,7 @@ impl Global { } let mut compatible_layout = None; - { + let layout = { let (bgl_guard, _) = hub.bind_group_layouts.read(&mut token); if let Some(id) = Device::deduplicate_bind_group_layout(device_id, &entry_map, &*bgl_guard) @@ -1134,19 +1135,26 @@ impl Global { compatible_layout = Some(id::Valid(id)); } - } - let mut layout = match device.create_bind_group_layout( - device_id, - desc.label.borrow_option(), - entry_map, - ) { - Ok(layout) => layout, - Err(e) => break e, + if let Some(original_id) = compatible_layout { + let original = &bgl_guard[original_id]; + BindGroupLayout { + device_id: original.device_id.clone(), + inner: crate::binding_model::BglOrDuplicate::Duplicate(original_id), + multi_ref_count: crate::MultiRefCount::new(), + } + } else { + match device.create_bind_group_layout( + device_id, + desc.label.borrow_option(), + entry_map, + ) { + Ok(layout) => layout, + Err(e) => break e, + } + } }; - layout.compatible_layout = compatible_layout; - let id = fid.assign(layout, &mut token); if let Some(dupe) = compatible_layout { @@ -1319,7 +1327,7 @@ impl Global { }; let mut layout_id = id::Valid(desc.layout); - if let Some(id) = bind_group_layout.compatible_layout { + if let Some(id) = bind_group_layout.as_duplicate() { layout_id = id; bind_group_layout = &bind_group_layout_guard[id]; } diff --git a/wgpu-core/src/device/life.rs b/wgpu-core/src/device/life.rs index b641567d71..e1d01ccaba 100644 --- a/wgpu-core/src/device/life.rs +++ b/wgpu-core/src/device/life.rs @@ -776,7 +776,7 @@ impl LifetimeTracker { if bgl.multi_ref_count.dec_and_check_empty() { // If This layout points to a compatible one, go over the latter // to decrement the ref count and potentially destroy it. - bgl_to_check = bgl.compatible_layout; + bgl_to_check = bgl.as_duplicate(); log::debug!("Bind group layout {:?} will be destroyed", id); #[cfg(feature = "trace")] @@ -786,7 +786,9 @@ impl LifetimeTracker { if let Some(lay) = hub.bind_group_layouts.unregister_locked(id.0, &mut *guard) { - self.free_resources.bind_group_layouts.push(lay.raw); + if let Some(inner) = lay.into_inner() { + self.free_resources.bind_group_layouts.push(inner.raw); + } } } } diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index 8f37f68be9..c839d6a828 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -1,7 +1,10 @@ #[cfg(feature = "trace")] use crate::device::trace; use crate::{ - binding_model::{self, get_bind_group_layout, try_get_bind_group_layout}, + binding_model::{ + self, get_bind_group_layout, try_get_bind_group_layout, BglOrDuplicate, + BindGroupLayoutInner, + }, command, conv, device::life::WaitIdleError, device::{ @@ -1392,8 +1395,9 @@ impl Device { .iter(self_id.backend()) .find(|&(_, bgl)| { bgl.device_id.value.0 == self_id - && bgl.compatible_layout.is_none() - && bgl.entries == *entry_map + && bgl + .as_inner() + .map_or(false, |inner| inner.entries == *entry_map) }) .map(|(id, value)| { value.multi_ref_count.inc(); @@ -1408,7 +1412,12 @@ impl Device { pipeline_layout .bind_group_layout_ids .iter() - .map(|&id| &bgl_guard[id].entries) + .map(|&id| { + &get_bind_group_layout(bgl_guard, id) + .1 + .assume_deduplicated() + .entries + }) .collect() } @@ -1427,7 +1436,9 @@ impl Device { .iter() .enumerate() .map(|(group_index, &bgl_id)| pipeline::LateSizedBufferGroup { - shader_sizes: bgl_guard[bgl_id] + shader_sizes: get_bind_group_layout(bgl_guard, bgl_id) + .1 + .assume_deduplicated() .entries .values() .filter_map(|entry| match entry.ty { @@ -1639,21 +1650,22 @@ impl Device { .map_err(binding_model::CreateBindGroupLayoutError::TooManyBindings)?; Ok(binding_model::BindGroupLayout { - raw, device_id: Stored { value: id::Valid(self_id), ref_count: self.life_guard.add_ref(), }, multi_ref_count: MultiRefCount::new(), - dynamic_count: entry_map - .values() - .filter(|b| b.ty.has_dynamic_offset()) - .count(), - count_validator, - entries: entry_map, - #[cfg(debug_assertions)] - label: label.unwrap_or("").to_string(), - compatible_layout: None, + inner: BglOrDuplicate::Inner(BindGroupLayoutInner { + raw, + dynamic_count: entry_map + .values() + .filter(|b| b.ty.has_dynamic_offset()) + .count(), + count_validator, + entries: entry_map, + #[cfg(debug_assertions)] + label: label.unwrap_or("").to_string(), + }), }) } @@ -1823,6 +1835,8 @@ impl Device { Ok(()) } + // This function expects the provided bind group layout to be resolved + // (not passing a duplicate) beforehand. pub(super) fn create_bind_group( &self, self_id: id::DeviceId, @@ -1837,7 +1851,7 @@ impl Device { // Check that the number of entries in the descriptor matches // the number of entries in the layout. let actual = desc.entries.len(); - let expected = layout.entries.len(); + let expected = layout.assume_deduplicated().entries.len(); if actual != expected { return Err(Error::BindingsNumMismatch { expected, actual }); } @@ -1868,6 +1882,7 @@ impl Device { let binding = entry.binding; // Find the corresponding declaration in the layout let decl = layout + .assume_deduplicated() .entries .get(&binding) .ok_or(Error::MissingBindingDeclaration(binding))?; @@ -2044,9 +2059,11 @@ impl Device { } } + let layout_inner = layout.assume_deduplicated(); + let hal_desc = hal::BindGroupDescriptor { label: desc.label.borrow_option(), - layout: &layout.raw, + layout: &layout_inner.raw, entries: &hal_entries, buffers: &hal_buffers, samplers: &hal_samplers, @@ -2074,7 +2091,7 @@ impl Device { used_texture_ranges, dynamic_binding_info, // collect in the order of BGL iteration - late_buffer_binding_sizes: layout + late_buffer_binding_sizes: layout_inner .entries .keys() .flat_map(|binding| late_buffer_binding_sizes.get(binding).cloned()) @@ -2304,10 +2321,10 @@ impl Device { // validate total resource counts for &id in desc.bind_group_layouts.iter() { - let bind_group_layout = bgl_guard - .get(id) - .map_err(|_| Error::InvalidBindGroupLayout(id))?; - count_validator.merge(&bind_group_layout.count_validator); + let Some(bind_group_layout) = try_get_bind_group_layout(bgl_guard, id) else { + return Err(Error::InvalidBindGroupLayout(id)); + }; + count_validator.merge(&bind_group_layout.assume_deduplicated().count_validator); } count_validator .validate(&self.limits) @@ -2316,7 +2333,12 @@ impl Device { let bgl_vec = desc .bind_group_layouts .iter() - .map(|&id| &try_get_bind_group_layout(bgl_guard, id).unwrap().raw) + .map(|&id| { + &try_get_bind_group_layout(bgl_guard, id) + .unwrap() + .assume_deduplicated() + .raw + }) .collect::>(); let hal_desc = hal::PipelineLayoutDescriptor { label: desc.label.borrow_option(), diff --git a/wgpu-core/src/hub.rs b/wgpu-core/src/hub.rs index c0670e085c..0435a28607 100644 --- a/wgpu-core/src/hub.rs +++ b/wgpu-core/src/hub.rs @@ -575,8 +575,10 @@ impl Hub { for element in self.bind_group_layouts.data.write().map.drain(..) { if let Element::Occupied(bgl, _) = element { let device = &devices[bgl.device_id.value]; - unsafe { - device.raw.destroy_bind_group_layout(bgl.raw); + if let Some(inner) = bgl.into_inner() { + unsafe { + device.raw.destroy_bind_group_layout(inner.raw); + } } } }