From 262d0c6dfcab23d9eef0fd8c7fd9673cec68dd77 Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Thu, 4 Feb 2021 01:16:40 -0500 Subject: [PATCH] Isolate binding compatibility logic into a separate module --- wgpu-core/src/command/bind.rs | 347 +++++++++++++++---------------- wgpu-core/src/command/compute.rs | 77 +++---- wgpu-core/src/command/render.rs | 80 ++++--- 3 files changed, 233 insertions(+), 271 deletions(-) diff --git a/wgpu-core/src/command/bind.rs b/wgpu-core/src/command/bind.rs index a62b38d5b7..a2063b4134 100644 --- a/wgpu-core/src/command/bind.rs +++ b/wgpu-core/src/command/bind.rs @@ -11,236 +11,215 @@ use crate::{ }; use arrayvec::ArrayVec; -use std::slice; -use wgt::DynamicOffset; type BindGroupMask = u8; -#[derive(Clone, Debug)] -pub(super) struct BindGroupPair { - layout_id: Valid, - group_id: Stored, -} +mod compat { + use std::ops::Range; -#[derive(Debug)] -pub(super) enum LayoutChange<'a> { - Unchanged, - Match(Valid, &'a [DynamicOffset]), - Mismatch, -} - -#[derive(Debug)] -pub enum Provision { - Unchanged, - Changed { was_compatible: bool }, -} - -#[derive(Clone)] -pub(super) struct FollowUpIter<'a> { - iter: slice::Iter<'a, BindGroupEntry>, -} -impl<'a> Iterator for FollowUpIter<'a> { - type Item = (Valid, &'a [DynamicOffset]); - fn next(&mut self) -> Option { - self.iter - .next() - .and_then(|entry| Some((entry.actual_value()?, entry.dynamic_offsets.as_slice()))) + #[derive(Debug)] + struct Entry { + assigned: Option, + expected: Option, } -} - -#[derive(Clone, Default, Debug)] -pub(super) struct BindGroupEntry { - expected_layout_id: Option>, - provided: Option, - dynamic_offsets: Vec, -} - -impl BindGroupEntry { - fn provide( - &mut self, - bind_group_id: Valid, - bind_group: &BindGroup, - offsets: &[DynamicOffset], - ) -> Provision { - debug_assert_eq!(B::VARIANT, bind_group_id.0.backend()); - - let was_compatible = match self.provided { - Some(BindGroupPair { - layout_id, - ref group_id, - }) => { - if group_id.value == bind_group_id && offsets == self.dynamic_offsets.as_slice() { - assert_eq!(layout_id, bind_group.layout_id); - return Provision::Unchanged; - } - self.expected_layout_id == Some(layout_id) + impl Default for Entry { + fn default() -> Self { + Entry { + assigned: None, + expected: None, } - None => false, - }; - - self.provided = Some(BindGroupPair { - layout_id: bind_group.layout_id, - group_id: Stored { - value: bind_group_id, - ref_count: bind_group.life_guard.add_ref(), - }, - }); - self.dynamic_offsets.clear(); - self.dynamic_offsets.extend_from_slice(offsets); - - Provision::Changed { was_compatible } + } } + impl Entry { + fn is_active(&self) -> bool { + self.assigned.is_some() && self.expected.is_some() + } - pub fn expect_layout( - &mut self, - bind_group_layout_id: Valid, - ) -> LayoutChange { - let some = Some(bind_group_layout_id); - if self.expected_layout_id != some { - self.expected_layout_id = some; - match self.provided { - Some(BindGroupPair { - layout_id, - ref group_id, - }) if layout_id == bind_group_layout_id => { - LayoutChange::Match(group_id.value, &self.dynamic_offsets) - } - Some(_) | None => LayoutChange::Mismatch, - } - } else { - LayoutChange::Unchanged + fn is_valid(&self) -> bool { + self.expected.is_none() || self.expected == self.assigned } } - fn is_valid(&self) -> Option { - match (self.expected_layout_id, self.provided.as_ref()) { - (None, None) => Some(true), - (None, Some(_)) => None, - (Some(_), None) => Some(false), - (Some(layout), Some(pair)) => Some(layout == pair.layout_id), - } + #[derive(Debug)] + pub struct Manager { + entries: [Entry; crate::MAX_BIND_GROUPS], } - fn actual_value(&self) -> Option> { - self.expected_layout_id.and_then(|layout_id| { - self.provided.as_ref().and_then(|pair| { - if pair.layout_id == layout_id { - Some(pair.group_id.value) + impl Manager { + pub fn new() -> Self { + Manager { + entries: Default::default(), + } + } + + fn make_range(&self, start_index: usize) -> Range { + // find first incompatible entry + let end = self + .entries + .iter() + .position(|e| e.expected.is_none() || e.assigned != e.expected) + .unwrap_or(self.entries.len()); + start_index..end.max(start_index) + } + + pub fn update_expectations(&mut self, expectations: &[T]) -> Range { + let start_index = self + .entries + .iter() + .zip(expectations) + .position(|(e, &expect)| e.expected != Some(expect)) + .unwrap_or(expectations.len()); + for (e, &expect) in self.entries[start_index..] + .iter_mut() + .zip(expectations[start_index..].iter()) + { + e.expected = Some(expect); + } + for e in self.entries[expectations.len()..].iter_mut() { + e.expected = None; + } + self.make_range(start_index) + } + + pub fn assign(&mut self, index: usize, value: T) -> Range { + self.entries[index].assigned = Some(value); + self.make_range(index) + } + + pub fn list_active(&self) -> impl Iterator + '_ { + self.entries + .iter() + .enumerate() + .filter_map(|(i, e)| if e.is_active() { Some(i) } else { None }) + } + + pub fn invalid_mask(&self) -> super::BindGroupMask { + self.entries.iter().enumerate().fold(0, |mask, (i, entry)| { + if entry.is_valid() { + mask } else { - None + mask | 1u8 << i } }) - }) + } + } + + #[test] + fn test_compatibility() { + let mut man = Manager::::new(); + man.entries[0] = Entry { + expected: Some(3), + assigned: Some(2), + }; + man.entries[1] = Entry { + expected: Some(1), + assigned: Some(1), + }; + man.entries[2] = Entry { + expected: Some(4), + assigned: Some(5), + }; + // check that we rebind [1] after [0] became compatible + assert_eq!(man.assign(0, 3), 0..2); + // check that nothing is rebound + assert_eq!(man.update_expectations(&[3, 2]), 1..1); + // check that [1] and [2] are rebound on expectations change + assert_eq!(man.update_expectations(&[3, 1, 5]), 1..3); + // reset the first two bindings + assert_eq!(man.update_expectations(&[4, 6, 5]), 0..0); + // check that nothing is rebound, even if there is a match, + // since earlier binding is incompatible. + assert_eq!(man.assign(1, 6), 1..1); + // finally, bind everything + assert_eq!(man.assign(0, 4), 0..3); } } +#[derive(Debug, Default)] +pub(super) struct EntryPayload { + pub(super) group_id: Option>, + pub(super) dynamic_offsets: Vec, +} + #[derive(Debug)] -pub struct Binder { +pub(super) struct Binder { pub(super) pipeline_layout_id: Option>, //TODO: strongly `Stored` - pub(super) entries: ArrayVec<[BindGroupEntry; MAX_BIND_GROUPS]>, + manager: compat::Manager>, + payloads: [EntryPayload; MAX_BIND_GROUPS], } impl Binder { - pub(super) fn new(max_bind_groups: u32) -> Self { - Self { + pub(super) fn new() -> Self { + Binder { pipeline_layout_id: None, - entries: (0..max_bind_groups) - .map(|_| BindGroupEntry::default()) - .collect(), + manager: compat::Manager::new(), + payloads: Default::default(), } } pub(super) fn reset(&mut self) { self.pipeline_layout_id = None; - self.entries.clear(); - } - - pub(super) fn change_pipeline_layout( - &mut self, - guard: &Storage, PipelineLayoutId>, - new_id: Valid, - ) { - let old_id_opt = self.pipeline_layout_id.replace(new_id); - let new = &guard[new_id]; - - let length = if let Some(old_id) = old_id_opt { - let old = &guard[old_id]; - if old.push_constant_ranges == new.push_constant_ranges { - new.bind_group_layout_ids.len() - } else { - 0 - } - } else { - 0 - }; - - for entry in self.entries[length..].iter_mut() { - entry.expected_layout_id = None; + self.manager = compat::Manager::new(); + for payload in self.payloads.iter_mut() { + payload.group_id = None; + payload.dynamic_offsets.clear(); } } - /// Attempt to set the value of the specified bind group index. - /// Returns Some() when the new bind group is ready to be actually bound - /// (i.e. compatible with current expectations). Also returns an iterator - /// of bind group IDs to be bound with it: those are compatible bind groups - /// that were previously blocked because the current one was incompatible. - pub(super) fn provide_entry<'a, B: GfxBackend>( + pub(super) fn change_pipeline_layout<'a, B: GfxBackend>( + &'a mut self, + guard: &Storage, PipelineLayoutId>, + new_id: Valid, + ) -> (usize, &'a [EntryPayload]) { + let old_id_opt = self.pipeline_layout_id.replace(new_id); + let new = &guard[new_id]; + + let mut bind_range = self.manager.update_expectations(&new.bind_group_layout_ids); + + if let Some(old_id) = old_id_opt { + let old = &guard[old_id]; + // root constants are the base compatibility property + if old.push_constant_ranges != new.push_constant_ranges + && bind_range.end != bind_range.start + { + bind_range.start = 0; + } + } + + (bind_range.start, &self.payloads[bind_range]) + } + + pub(super) fn assign_group<'a, B: GfxBackend>( &'a mut self, index: usize, bind_group_id: Valid, bind_group: &BindGroup, - offsets: &[DynamicOffset], - ) -> Option<(Valid, FollowUpIter<'a>)> { + offsets: &[wgt::DynamicOffset], + ) -> &'a [EntryPayload] { tracing::trace!("\tBinding [{}] = group {:?}", index, bind_group_id); debug_assert_eq!(B::VARIANT, bind_group_id.0.backend()); - match self.entries[index].provide(bind_group_id, bind_group, offsets) { - Provision::Unchanged => None, - Provision::Changed { was_compatible, .. } => { - let compatible_count = self.compatible_count(); - if index < compatible_count { - let end = compatible_count.min(if was_compatible { - index + 1 - } else { - self.entries.len() - }); - tracing::trace!("\t\tbinding up to {}", end); - Some(( - self.pipeline_layout_id?, - FollowUpIter { - iter: self.entries[index + 1..end].iter(), - }, - )) - } else { - tracing::trace!("\t\tskipping above compatible {}", compatible_count); - None - } - } - } + let payload = &mut self.payloads[index]; + payload.group_id = Some(Stored { + value: bind_group_id, + ref_count: bind_group.life_guard.add_ref(), + }); + payload.dynamic_offsets.clear(); + payload.dynamic_offsets.extend_from_slice(offsets); + + let bind_range = self.manager.assign(index, bind_group.layout_id); + &self.payloads[bind_range] } pub(super) fn list_active(&self) -> impl Iterator> + '_ { - self.entries.iter().filter_map(|e| match e.provided { - Some(ref pair) if e.expected_layout_id.is_some() => Some(pair.group_id.value), - _ => None, - }) + let payloads = &self.payloads; + self.manager + .list_active() + .map(move |index| payloads[index].group_id.as_ref().unwrap().value) } pub(super) fn invalid_mask(&self) -> BindGroupMask { - self.entries.iter().enumerate().fold(0, |mask, (i, entry)| { - if entry.is_valid().unwrap_or(true) { - mask - } else { - mask | 1u8 << i - } - }) - } - - fn compatible_count(&self) -> usize { - self.entries - .iter() - .position(|entry| !entry.is_valid().unwrap_or(false)) - .unwrap_or_else(|| self.entries.len()) + self.manager.invalid_mask() } } diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index c810b327ef..5e1d898daf 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -5,9 +5,8 @@ use crate::{ binding_model::{BindError, BindGroup, PushConstantUploadError}, command::{ - bind::{Binder, LayoutChange}, - end_pipeline_statistics_query, BasePass, BasePassRef, CommandBuffer, CommandEncoderError, - MapPassErr, PassErrorScope, QueryUseError, StateChange, + bind::Binder, end_pipeline_statistics_query, BasePass, BasePassRef, CommandBuffer, + CommandEncoderError, MapPassErr, PassErrorScope, QueryUseError, StateChange, }, hub::{GfxBackend, Global, GlobalIdentityHandlerFactory, Storage, Token}, id, @@ -24,7 +23,7 @@ use thiserror::Error; use wgt::{BufferAddress, BufferUsage, ShaderStage}; use crate::track::UseExtendError; -use std::{fmt, iter, mem, str}; +use std::{fmt, mem, str}; #[doc(hidden)] #[derive(Clone, Copy, Debug)] @@ -287,7 +286,7 @@ impl Global { let (texture_guard, _) = hub.textures.read(&mut token); let mut state = State { - binder: Binder::new(cmd_buf.limits.max_bind_groups), + binder: Binder::new(), pipeline: StateChange::new(), trackers: TrackerSet::new(B::VARIANT), debug_scope_depth: 0, @@ -348,24 +347,28 @@ impl Global { ), ); - if let Some((pipeline_layout_id, follow_ups)) = state.binder.provide_entry( + let pipeline_layout_id = state.binder.pipeline_layout_id; + let entries = state.binder.assign_group( index as usize, id::Valid(bind_group_id), bind_group, &temp_offsets, - ) { - let bind_groups = iter::once(bind_group.raw.raw()).chain( - follow_ups - .clone() - .map(|(bg_id, _)| bind_group_guard[bg_id].raw.raw()), - ); - temp_offsets.extend(follow_ups.flat_map(|(_, offsets)| offsets)); + ); + if !entries.is_empty() { + let pipeline_layout = + &pipeline_layout_guard[pipeline_layout_id.unwrap()].raw; + let desc_sets = entries.iter().map(|e| { + bind_group_guard[e.group_id.as_ref().unwrap().value] + .raw + .raw() + }); + let offsets = entries.iter().flat_map(|e| &e.dynamic_offsets).cloned(); unsafe { raw.bind_compute_descriptor_sets( - &pipeline_layout_guard[pipeline_layout_id].raw, + pipeline_layout, index as usize, - bind_groups, - temp_offsets.iter().cloned(), + desc_sets, + offsets, ); } } @@ -392,36 +395,24 @@ impl Global { if state.binder.pipeline_layout_id != Some(pipeline.layout_id.value) { let pipeline_layout = &pipeline_layout_guard[pipeline.layout_id.value]; - state.binder.change_pipeline_layout( + let (start_index, entries) = state.binder.change_pipeline_layout( &*pipeline_layout_guard, pipeline.layout_id.value, ); - - let mut is_compatible = true; - - for (index, (entry, &bgl_id)) in state - .binder - .entries - .iter_mut() - .zip(&pipeline_layout.bind_group_layout_ids) - .enumerate() - { - match entry.expect_layout(bgl_id) { - LayoutChange::Match(bg_id, offsets) if is_compatible => { - let desc_set = bind_group_guard[bg_id].raw.raw(); - unsafe { - raw.bind_compute_descriptor_sets( - &pipeline_layout.raw, - index, - iter::once(desc_set), - offsets.iter().cloned(), - ); - } - } - LayoutChange::Match(..) | LayoutChange::Unchanged => {} - LayoutChange::Mismatch => { - is_compatible = false; - } + if !entries.is_empty() { + let desc_sets = entries.iter().map(|e| { + bind_group_guard[e.group_id.as_ref().unwrap().value] + .raw + .raw() + }); + let offsets = entries.iter().flat_map(|e| &e.dynamic_offsets).cloned(); + unsafe { + raw.bind_compute_descriptor_sets( + &pipeline_layout.raw, + start_index, + desc_sets, + offsets, + ); } } diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index cb5547750a..ba73ee7852 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -5,10 +5,9 @@ use crate::{ binding_model::BindError, command::{ - bind::{Binder, LayoutChange}, - end_pipeline_statistics_query, BasePass, BasePassRef, CommandBuffer, CommandEncoderError, - DrawError, ExecutionError, MapPassErr, PassErrorScope, QueryResetMap, QueryUseError, - RenderCommand, RenderCommandError, StateChange, + bind::Binder, end_pipeline_statistics_query, BasePass, BasePassRef, CommandBuffer, + CommandEncoderError, DrawError, ExecutionError, MapPassErr, PassErrorScope, QueryResetMap, + QueryUseError, RenderCommand, RenderCommandError, StateChange, }, conv, device::{ @@ -1067,7 +1066,7 @@ impl Global { let mut state = State { pipeline_flags: PipelineFlags::empty(), - binder: Binder::new(cmd_buf.limits.max_bind_groups), + binder: Binder::new(), blend_color: OptionalState::Unused, stencil_reference: 0, pipeline: StateChange::new(), @@ -1134,27 +1133,31 @@ impl Global { }), ); - if let Some((pipeline_layout_id, follow_ups)) = state.binder.provide_entry( + let pipeline_layout_id = state.binder.pipeline_layout_id; + let entries = state.binder.assign_group( index as usize, id::Valid(bind_group_id), bind_group, &temp_offsets, - ) { - let bind_groups = iter::once(bind_group.raw.raw()).chain( - follow_ups - .clone() - .map(|(bg_id, _)| bind_group_guard[bg_id].raw.raw()), - ); - temp_offsets.extend(follow_ups.flat_map(|(_, offsets)| offsets)); + ); + if !entries.is_empty() { + let pipeline_layout = + &pipeline_layout_guard[pipeline_layout_id.unwrap()].raw; + let desc_sets = entries.iter().map(|e| { + bind_group_guard[e.group_id.as_ref().unwrap().value] + .raw + .raw() + }); + let offsets = entries.iter().flat_map(|e| &e.dynamic_offsets).cloned(); unsafe { raw.bind_graphics_descriptor_sets( - &pipeline_layout_guard[pipeline_layout_id].raw, + pipeline_layout, index as usize, - bind_groups, - temp_offsets.iter().cloned(), + desc_sets, + offsets, ); } - }; + } } RenderCommand::SetPipeline(pipeline_id) => { let scope = PassErrorScope::SetPipelineRender(pipeline_id); @@ -1204,36 +1207,25 @@ impl Global { if state.binder.pipeline_layout_id != Some(pipeline.layout_id.value) { let pipeline_layout = &pipeline_layout_guard[pipeline.layout_id.value]; - state.binder.change_pipeline_layout( + let (start_index, entries) = state.binder.change_pipeline_layout( &*pipeline_layout_guard, pipeline.layout_id.value, ); - - let mut is_compatible = true; - - for (index, (entry, &bgl_id)) in state - .binder - .entries - .iter_mut() - .zip(&pipeline_layout.bind_group_layout_ids) - .enumerate() - { - match entry.expect_layout(bgl_id) { - LayoutChange::Match(bg_id, offsets) if is_compatible => { - let desc_set = bind_group_guard[bg_id].raw.raw(); - unsafe { - raw.bind_graphics_descriptor_sets( - &pipeline_layout.raw, - index, - iter::once(desc_set), - offsets.iter().cloned(), - ); - } - } - LayoutChange::Match(..) | LayoutChange::Unchanged => {} - LayoutChange::Mismatch => { - is_compatible = false; - } + if !entries.is_empty() { + let desc_sets = entries.iter().map(|e| { + bind_group_guard[e.group_id.as_ref().unwrap().value] + .raw + .raw() + }); + let offsets = + entries.iter().flat_map(|e| &e.dynamic_offsets).cloned(); + unsafe { + raw.bind_graphics_descriptor_sets( + &pipeline_layout.raw, + start_index, + desc_sets, + offsets, + ); } }