From 5ff7062c1c785bcb749cd2b717c0c34712febcac Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 12 Feb 2025 14:39:04 -0800 Subject: [PATCH] Switch bins from parallel key/value arrays to `IndexMap`s. (#17819) Conceptually, bins are ordered hash maps. We currently implement these as a list of keys with an associated hash map. But we already have a data type that implements ordered hash maps directly: `IndexMap`. This patch switches Bevy to use `IndexMap`s for bins. Because we're memory bound, this doesn't affect performance much, but it is cleaner. --- .../bevy_core_pipeline/src/deferred/node.rs | 6 +- .../src/batching/gpu_preprocessing.rs | 12 +- crates/bevy_render/src/batching/mod.rs | 20 +-- .../src/batching/no_gpu_preprocessing.rs | 7 +- crates/bevy_render/src/render_phase/mod.rs | 147 +++++++----------- 5 files changed, 72 insertions(+), 120 deletions(-) diff --git a/crates/bevy_core_pipeline/src/deferred/node.rs b/crates/bevy_core_pipeline/src/deferred/node.rs index 5485e8fc00..7fe111de9b 100644 --- a/crates/bevy_core_pipeline/src/deferred/node.rs +++ b/crates/bevy_core_pipeline/src/deferred/node.rs @@ -147,9 +147,9 @@ impl ViewNode for DeferredGBufferPrepassNode { } // Opaque draws - if !opaque_deferred_phase.multidrawable_mesh_keys.is_empty() - || !opaque_deferred_phase.batchable_mesh_keys.is_empty() - || !opaque_deferred_phase.unbatchable_mesh_keys.is_empty() + if !opaque_deferred_phase.multidrawable_meshes.is_empty() + || !opaque_deferred_phase.batchable_meshes.is_empty() + || !opaque_deferred_phase.unbatchable_meshes.is_empty() { #[cfg(feature = "trace")] let _opaque_prepass_span = info_span!("opaque_deferred_prepass").entered(); diff --git a/crates/bevy_render/src/batching/gpu_preprocessing.rs b/crates/bevy_render/src/batching/gpu_preprocessing.rs index 484beb4c6a..ccfd272970 100644 --- a/crates/bevy_render/src/batching/gpu_preprocessing.rs +++ b/crates/bevy_render/src/batching/gpu_preprocessing.rs @@ -1374,11 +1374,11 @@ pub fn batch_and_prepare_binned_render_phase( // Prepare multidrawables. - for batch_set_key in &phase.multidrawable_mesh_keys { + for (batch_set_key, bins) in &phase.multidrawable_meshes { let mut batch_set = None; let indirect_parameters_base = indirect_parameters_buffers.batch_count(batch_set_key.indexed()) as u32; - for (bin_key, bin) in &phase.multidrawable_mesh_values[batch_set_key] { + for (bin_key, bin) in bins { let first_output_index = data_buffer.len() as u32; let mut batch: Option = None; @@ -1472,11 +1472,11 @@ pub fn batch_and_prepare_binned_render_phase( // Prepare batchables. - for key in &phase.batchable_mesh_keys { + for (key, bin) in &phase.batchable_meshes { let first_output_index = data_buffer.len() as u32; let mut batch: Option = None; - for (&main_entity, &input_index) in phase.batchable_mesh_values[key].entities() { + for (&main_entity, &input_index) in bin.entities() { let output_index = data_buffer.add() as u32; match batch { @@ -1589,9 +1589,7 @@ pub fn batch_and_prepare_binned_render_phase( } // Prepare unbatchables. - for key in &phase.unbatchable_mesh_keys { - let unbatchables = phase.unbatchable_mesh_values.get_mut(key).unwrap(); - + for (key, unbatchables) in &mut phase.unbatchable_meshes { // Allocate the indirect parameters if necessary. let mut indirect_parameters_offset = if no_indirect_drawing { None diff --git a/crates/bevy_render/src/batching/mod.rs b/crates/bevy_render/src/batching/mod.rs index 8f2dc7844f..ddafb6f516 100644 --- a/crates/bevy_render/src/batching/mod.rs +++ b/crates/bevy_render/src/batching/mod.rs @@ -190,23 +190,9 @@ where BPI: BinnedPhaseItem, { for phase in phases.values_mut() { - phase.multidrawable_mesh_keys.clear(); - phase - .multidrawable_mesh_keys - .extend(phase.multidrawable_mesh_values.keys().cloned()); - phase.multidrawable_mesh_keys.sort_unstable(); - - phase.batchable_mesh_keys.clear(); - phase - .batchable_mesh_keys - .extend(phase.batchable_mesh_values.keys().cloned()); - phase.batchable_mesh_keys.sort_unstable(); - - phase.unbatchable_mesh_keys.clear(); - phase - .unbatchable_mesh_keys - .extend(phase.unbatchable_mesh_values.keys().cloned()); - phase.unbatchable_mesh_keys.sort_unstable(); + phase.multidrawable_meshes.sort_unstable_keys(); + phase.batchable_meshes.sort_unstable_keys(); + phase.unbatchable_meshes.sort_unstable_keys(); } } diff --git a/crates/bevy_render/src/batching/no_gpu_preprocessing.rs b/crates/bevy_render/src/batching/no_gpu_preprocessing.rs index f5a68a6281..8bbbff8dd9 100644 --- a/crates/bevy_render/src/batching/no_gpu_preprocessing.rs +++ b/crates/bevy_render/src/batching/no_gpu_preprocessing.rs @@ -108,9 +108,9 @@ pub fn batch_and_prepare_binned_render_phase( for phase in phases.values_mut() { // Prepare batchables. - for key in &phase.batchable_mesh_keys { + for bin in phase.batchable_meshes.values_mut() { let mut batch_set: SmallVec<[BinnedRenderPhaseBatch; 1]> = smallvec![]; - for main_entity in phase.batchable_mesh_values[key].entities().keys() { + for main_entity in bin.entities().keys() { let Some(buffer_data) = GFBD::get_binned_batch_data(&system_param_item, *main_entity) else { @@ -156,8 +156,7 @@ pub fn batch_and_prepare_binned_render_phase( } // Prepare unbatchables. - for key in &phase.unbatchable_mesh_keys { - let unbatchables = phase.unbatchable_mesh_values.get_mut(key).unwrap(); + for unbatchables in phase.unbatchable_meshes.values_mut() { for main_entity in unbatchables.entities.keys() { let Some(buffer_data) = GFBD::get_binned_batch_data(&system_param_item, *main_entity) diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index e25f83b58c..4ddd7a86ab 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -92,13 +92,7 @@ pub struct BinnedRenderPhase where BPI: BinnedPhaseItem, { - /// A list of `BatchSetKey`s for batchable, multidrawable items. - /// - /// These are accumulated in `queue_material_meshes` and then sorted in - /// `batching::sort_binned_render_phase`. - pub multidrawable_mesh_keys: Vec, - - /// The multidrawable bins themselves. + /// The multidrawable bins. /// /// Each batch set key maps to a *batch set*, which in this case is a set of /// meshes that can be drawn together in one multidraw call. Each batch set @@ -111,36 +105,18 @@ where /// the same pipeline. The first bin, corresponding to the cubes, will have /// two entities in it. The second bin, corresponding to the sphere, will /// have one entity in it. - pub multidrawable_mesh_values: HashMap>, - - /// A list of `BinKey`s for batchable items that aren't multidrawable. - /// - /// These are accumulated in `queue_material_meshes` and then sorted in - /// `batch_and_prepare_binned_render_phase`. - /// - /// Usually, batchable items aren't multidrawable due to platform or - /// hardware limitations. However, it's also possible to have batchable - /// items alongside multidrawable items with custom mesh pipelines. See - /// `specialized_mesh_pipeline` for an example. - pub batchable_mesh_keys: Vec<(BPI::BatchSetKey, BPI::BinKey)>, + pub multidrawable_meshes: IndexMap>, /// The bins corresponding to batchable items that aren't multidrawable. /// - /// For multidrawable entities, use `multidrawable_mesh_values`; for + /// For multidrawable entities, use `multidrawable_meshes`; for /// unbatchable entities, use `unbatchable_values`. - pub batchable_mesh_values: HashMap<(BPI::BatchSetKey, BPI::BinKey), RenderBin>, - - /// A list of `BinKey`s for unbatchable items. - /// - /// These are accumulated in `queue_material_meshes` and then sorted in - /// `batch_and_prepare_binned_render_phase`. - pub unbatchable_mesh_keys: Vec<(BPI::BatchSetKey, BPI::BinKey)>, + pub batchable_meshes: IndexMap<(BPI::BatchSetKey, BPI::BinKey), RenderBin>, /// The unbatchable bins. /// /// Each entity here is rendered in a separate drawcall. - pub unbatchable_mesh_values: - HashMap<(BPI::BatchSetKey, BPI::BinKey), UnbatchableBinnedEntities>, + pub unbatchable_meshes: IndexMap<(BPI::BatchSetKey, BPI::BinKey), UnbatchableBinnedEntities>, /// Items in the bin that aren't meshes at all. /// @@ -149,7 +125,7 @@ where /// entity are simply called in order at rendering time. /// /// See the `custom_phase_item` example for an example of how to use this. - pub non_mesh_items: HashMap<(BPI::BatchSetKey, BPI::BinKey), RenderBin>, + pub non_mesh_items: IndexMap<(BPI::BatchSetKey, BPI::BinKey), RenderBin>, /// Information on each batch set. /// @@ -456,16 +432,16 @@ where ) { match phase_type { BinnedRenderPhaseType::MultidrawableMesh => { - match self.multidrawable_mesh_values.entry(batch_set_key.clone()) { - Entry::Occupied(mut entry) => { + match self.multidrawable_meshes.entry(batch_set_key.clone()) { + indexmap::map::Entry::Occupied(mut entry) => { entry .get_mut() .entry(bin_key.clone()) .or_default() .insert(main_entity, input_uniform_index); } - Entry::Vacant(entry) => { - let mut new_batch_set = HashMap::default(); + indexmap::map::Entry::Vacant(entry) => { + let mut new_batch_set = IndexMap::default(); new_batch_set.insert( bin_key.clone(), RenderBin::from_entity(main_entity, input_uniform_index), @@ -477,13 +453,13 @@ where BinnedRenderPhaseType::BatchableMesh => { match self - .batchable_mesh_values + .batchable_meshes .entry((batch_set_key.clone(), bin_key.clone()).clone()) { - Entry::Occupied(mut entry) => { + indexmap::map::Entry::Occupied(mut entry) => { entry.get_mut().insert(main_entity, input_uniform_index); } - Entry::Vacant(entry) => { + indexmap::map::Entry::Vacant(entry) => { entry.insert(RenderBin::from_entity(main_entity, input_uniform_index)); } } @@ -491,13 +467,13 @@ where BinnedRenderPhaseType::UnbatchableMesh => { match self - .unbatchable_mesh_values + .unbatchable_meshes .entry((batch_set_key.clone(), bin_key.clone())) { - Entry::Occupied(mut entry) => { + indexmap::map::Entry::Occupied(mut entry) => { entry.get_mut().entities.insert(main_entity, entity); } - Entry::Vacant(entry) => { + indexmap::map::Entry::Vacant(entry) => { let mut entities = MainEntityHashMap::default(); entities.insert(main_entity, entity); entry.insert(UnbatchableBinnedEntities { @@ -514,10 +490,10 @@ where .non_mesh_items .entry((batch_set_key.clone(), bin_key.clone()).clone()) { - Entry::Occupied(mut entry) => { + indexmap::map::Entry::Occupied(mut entry) => { entry.get_mut().insert(main_entity, input_uniform_index); } - Entry::Vacant(entry) => { + indexmap::map::Entry::Vacant(entry) => { entry.insert(RenderBin::from_entity(main_entity, input_uniform_index)); } } @@ -592,10 +568,10 @@ where match self.batch_sets { BinnedRenderPhaseBatchSets::DynamicUniforms(ref batch_sets) => { - debug_assert_eq!(self.batchable_mesh_keys.len(), batch_sets.len()); + debug_assert_eq!(self.batchable_meshes.len(), batch_sets.len()); for ((batch_set_key, bin_key), batch_set) in - self.batchable_mesh_keys.iter().zip(batch_sets.iter()) + self.batchable_meshes.keys().zip(batch_sets.iter()) { for batch in batch_set { let binned_phase_item = BPI::new( @@ -620,7 +596,7 @@ where BinnedRenderPhaseBatchSets::Direct(ref batch_set) => { for (batch, (batch_set_key, bin_key)) in - batch_set.iter().zip(self.batchable_mesh_keys.iter()) + batch_set.iter().zip(self.batchable_meshes.keys()) { let binned_phase_item = BPI::new( batch_set_key.clone(), @@ -643,11 +619,11 @@ where BinnedRenderPhaseBatchSets::MultidrawIndirect(ref batch_sets) => { for (batch_set_key, batch_set) in self - .multidrawable_mesh_keys - .iter() + .multidrawable_meshes + .keys() .chain( - self.batchable_mesh_keys - .iter() + self.batchable_meshes + .keys() .map(|(batch_set_key, _)| batch_set_key), ) .zip(batch_sets.iter()) @@ -704,9 +680,9 @@ where let draw_functions = world.resource::>(); let mut draw_functions = draw_functions.write(); - for (batch_set_key, bin_key) in &self.unbatchable_mesh_keys { + for (batch_set_key, bin_key) in self.unbatchable_meshes.keys() { let unbatchable_entities = - &self.unbatchable_mesh_values[&(batch_set_key.clone(), bin_key.clone())]; + &self.unbatchable_meshes[&(batch_set_key.clone(), bin_key.clone())]; for (entity_index, entity) in unbatchable_entities.entities.iter().enumerate() { let unbatchable_dynamic_offset = match &unbatchable_entities.buffer_indices { UnbatchableBinnedEntityIndexSet::NoEntities => { @@ -795,16 +771,13 @@ where } pub fn is_empty(&self) -> bool { - self.multidrawable_mesh_values.is_empty() - && self.batchable_mesh_values.is_empty() - && self.unbatchable_mesh_values.is_empty() + self.multidrawable_meshes.is_empty() + && self.batchable_meshes.is_empty() + && self.unbatchable_meshes.is_empty() && self.non_mesh_items.is_empty() } pub fn prepare_for_new_frame(&mut self) { - self.multidrawable_mesh_keys.clear(); - self.batchable_mesh_keys.clear(); - self.unbatchable_mesh_keys.clear(); self.batch_sets.clear(); self.valid_cached_entity_bin_keys.clear(); @@ -858,9 +831,9 @@ where remove_entity_from_bin( entity, &entity_bin_key, - &mut self.multidrawable_mesh_values, - &mut self.batchable_mesh_values, - &mut self.unbatchable_mesh_values, + &mut self.multidrawable_meshes, + &mut self.batchable_meshes, + &mut self.unbatchable_meshes, &mut self.non_mesh_items, ); } @@ -870,9 +843,9 @@ where remove_entity_from_bin( entity_that_changed_bins.main_entity, &entity_that_changed_bins.old_bin_key, - &mut self.multidrawable_mesh_values, - &mut self.batchable_mesh_values, - &mut self.unbatchable_mesh_values, + &mut self.multidrawable_meshes, + &mut self.batchable_meshes, + &mut self.unbatchable_meshes, &mut self.non_mesh_items, ); } @@ -888,22 +861,19 @@ where fn remove_entity_from_bin( entity: MainEntity, entity_bin_key: &CachedBinKey, - multidrawable_mesh_values: &mut HashMap>, - batchable_mesh_values: &mut HashMap<(BPI::BatchSetKey, BPI::BinKey), RenderBin>, - unbatchable_mesh_values: &mut HashMap< - (BPI::BatchSetKey, BPI::BinKey), - UnbatchableBinnedEntities, - >, - non_mesh_items: &mut HashMap<(BPI::BatchSetKey, BPI::BinKey), RenderBin>, + multidrawable_meshes: &mut IndexMap>, + batchable_meshes: &mut IndexMap<(BPI::BatchSetKey, BPI::BinKey), RenderBin>, + unbatchable_meshes: &mut IndexMap<(BPI::BatchSetKey, BPI::BinKey), UnbatchableBinnedEntities>, + non_mesh_items: &mut IndexMap<(BPI::BatchSetKey, BPI::BinKey), RenderBin>, ) where BPI: BinnedPhaseItem, { match entity_bin_key.phase_type { BinnedRenderPhaseType::MultidrawableMesh => { - if let Entry::Occupied(mut batch_set_entry) = - multidrawable_mesh_values.entry(entity_bin_key.batch_set_key.clone()) + if let indexmap::map::Entry::Occupied(mut batch_set_entry) = + multidrawable_meshes.entry(entity_bin_key.batch_set_key.clone()) { - if let Entry::Occupied(mut bin_entry) = batch_set_entry + if let indexmap::map::Entry::Occupied(mut bin_entry) = batch_set_entry .get_mut() .entry(entity_bin_key.bin_key.clone()) { @@ -911,19 +881,21 @@ fn remove_entity_from_bin( // If the bin is now empty, remove the bin. if bin_entry.get_mut().is_empty() { - bin_entry.remove(); + bin_entry.swap_remove(); } } - // If the batch set is now empty, remove it. + // If the batch set is now empty, remove it. This will perturb + // the order, but that's OK because we're going to sort the bin + // afterwards. if batch_set_entry.get_mut().is_empty() { - batch_set_entry.remove(); + batch_set_entry.swap_remove(); } } } BinnedRenderPhaseType::BatchableMesh => { - if let Entry::Occupied(mut bin_entry) = batchable_mesh_values.entry(( + if let indexmap::map::Entry::Occupied(mut bin_entry) = batchable_meshes.entry(( entity_bin_key.batch_set_key.clone(), entity_bin_key.bin_key.clone(), )) { @@ -931,13 +903,13 @@ fn remove_entity_from_bin( // If the bin is now empty, remove the bin. if bin_entry.get_mut().is_empty() { - bin_entry.remove(); + bin_entry.swap_remove(); } } } BinnedRenderPhaseType::UnbatchableMesh => { - if let Entry::Occupied(mut bin_entry) = unbatchable_mesh_values.entry(( + if let indexmap::map::Entry::Occupied(mut bin_entry) = unbatchable_meshes.entry(( entity_bin_key.batch_set_key.clone(), entity_bin_key.bin_key.clone(), )) { @@ -945,13 +917,13 @@ fn remove_entity_from_bin( // If the bin is now empty, remove the bin. if bin_entry.get_mut().entities.is_empty() { - bin_entry.remove(); + bin_entry.swap_remove(); } } } BinnedRenderPhaseType::NonMesh => { - if let Entry::Occupied(mut bin_entry) = non_mesh_items.entry(( + if let indexmap::map::Entry::Occupied(mut bin_entry) = non_mesh_items.entry(( entity_bin_key.batch_set_key.clone(), entity_bin_key.bin_key.clone(), )) { @@ -959,7 +931,7 @@ fn remove_entity_from_bin( // If the bin is now empty, remove the bin. if bin_entry.get_mut().is_empty() { - bin_entry.remove(); + bin_entry.swap_remove(); } } } @@ -972,13 +944,10 @@ where { fn new(gpu_preprocessing: GpuPreprocessingMode) -> Self { Self { - multidrawable_mesh_keys: vec![], - multidrawable_mesh_values: HashMap::default(), - batchable_mesh_keys: vec![], - batchable_mesh_values: HashMap::default(), - unbatchable_mesh_keys: vec![], - unbatchable_mesh_values: HashMap::default(), - non_mesh_items: HashMap::default(), + multidrawable_meshes: IndexMap::default(), + batchable_meshes: IndexMap::default(), + unbatchable_meshes: IndexMap::default(), + non_mesh_items: IndexMap::default(), batch_sets: match gpu_preprocessing { GpuPreprocessingMode::Culling => { BinnedRenderPhaseBatchSets::MultidrawIndirect(vec![])