From b35c3d18e8671808b2cba1a40d4669f6406e170e Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 13 Mar 2024 23:50:35 -0700 Subject: [PATCH 01/17] Remove unnecessary matched ID storage --- crates/bevy_ecs/src/query/iter.rs | 44 ++++++------- crates/bevy_ecs/src/query/par_iter.rs | 10 +-- crates/bevy_ecs/src/query/state.rs | 94 ++++++++++++++++----------- 3 files changed, 82 insertions(+), 66 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index cc911c1b95a89..187786e59671f 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -1,9 +1,9 @@ use crate::{ - archetype::{Archetype, ArchetypeEntity, ArchetypeId, Archetypes}, + archetype::{Archetype, ArchetypeEntity, Archetypes}, component::Tick, entity::{Entities, Entity}, - query::{ArchetypeFilter, DebugCheckedUnwrap, QueryState}, - storage::{Table, TableId, TableRow, Tables}, + query::{ArchetypeFilter, DebugCheckedUnwrap, QueryState, StorageId}, + storage::{Table, TableRow, Tables}, world::unsafe_world_cell::UnsafeWorldCell, }; use std::{borrow::Borrow, iter::FusedIterator, mem::MaybeUninit, ops::Range}; @@ -240,9 +240,9 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Iterator for QueryIter<'w, 's, D, F> accum = func(accum, item); } if D::IS_DENSE && F::IS_DENSE { - for table_id in self.cursor.table_id_iter.clone() { + for id in self.cursor.storage_id_iter.clone() { // SAFETY: Matched table IDs are guaranteed to still exist. - let table = unsafe { self.tables.get(*table_id).debug_checked_unwrap() }; + let table = unsafe { self.tables.get(id.table_id).debug_checked_unwrap() }; accum = // SAFETY: // - The fetched table matches both D and F @@ -251,10 +251,10 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Iterator for QueryIter<'w, 's, D, F> unsafe { self.fold_over_table_range(accum, &mut func, table, 0..table.entity_count()) }; } } else { - for archetype_id in self.cursor.archetype_id_iter.clone() { + for id in self.cursor.storage_id_iter.clone() { let archetype = // SAFETY: Matched archetype IDs are guaranteed to still exist. - unsafe { self.archetypes.get(*archetype_id).debug_checked_unwrap() }; + unsafe { self.archetypes.get(id.archetype_id).debug_checked_unwrap() }; accum = // SAFETY: // - The fetched archetype matches both D and F @@ -650,8 +650,7 @@ impl<'w, 's, D: ReadOnlyQueryData, F: QueryFilter, const K: usize> FusedIterator } struct QueryIterationCursor<'w, 's, D: QueryData, F: QueryFilter> { - table_id_iter: std::slice::Iter<'s, TableId>, - archetype_id_iter: std::slice::Iter<'s, ArchetypeId>, + storage_id_iter: std::slice::Iter<'s, StorageId>, table_entities: &'w [Entity], archetype_entities: &'w [ArchetypeEntity], fetch: D::Fetch<'w>, @@ -665,8 +664,7 @@ struct QueryIterationCursor<'w, 's, D: QueryData, F: QueryFilter> { impl Clone for QueryIterationCursor<'_, '_, D, F> { fn clone(&self) -> Self { Self { - table_id_iter: self.table_id_iter.clone(), - archetype_id_iter: self.archetype_id_iter.clone(), + storage_id_iter: self.storage_id_iter.clone(), table_entities: self.table_entities, archetype_entities: self.archetype_entities, fetch: self.fetch.clone(), @@ -687,8 +685,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIterationCursor<'w, 's, D, F> { this_run: Tick, ) -> Self { QueryIterationCursor { - table_id_iter: [].iter(), - archetype_id_iter: [].iter(), + storage_id_iter: [].iter(), ..Self::init(world, query_state, last_run, this_run) } } @@ -709,8 +706,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIterationCursor<'w, 's, D, F> { filter, table_entities: &[], archetype_entities: &[], - table_id_iter: query_state.matched_table_ids.iter(), - archetype_id_iter: query_state.matched_archetype_ids.iter(), + storage_id_iter: query_state.matched_storage_ids.iter(), current_len: 0, current_row: 0, } @@ -747,11 +743,13 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIterationCursor<'w, 's, D, F> { /// will be **the exact count of remaining values**. fn max_remaining(&self, tables: &'w Tables, archetypes: &'w Archetypes) -> usize { let remaining_matched: usize = if Self::IS_DENSE { - let ids = self.table_id_iter.clone(); - ids.map(|id| tables[*id].entity_count()).sum() + let ids = self.storage_id_iter.clone(); + // SAFETY: The if check ensures that storage_id_iter stores TableIds + unsafe { ids.map(|id| tables[id.table_id].entity_count()).sum() } } else { - let ids = self.archetype_id_iter.clone(); - ids.map(|id| archetypes[*id].len()).sum() + let ids = self.storage_id_iter.clone(); + // SAFETY: The if check ensures that storage_id_iter stores ArchetypeIds + unsafe { ids.map(|id| archetypes[id.archetype_id].len()).sum() } }; remaining_matched + self.current_len - self.current_row } @@ -773,8 +771,8 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIterationCursor<'w, 's, D, F> { loop { // we are on the beginning of the query, or finished processing a table, so skip to the next if self.current_row == self.current_len { - let table_id = self.table_id_iter.next()?; - let table = tables.get(*table_id).debug_checked_unwrap(); + let table_id = self.storage_id_iter.next()?.table_id; + let table = tables.get(table_id).debug_checked_unwrap(); // SAFETY: `table` is from the world that `fetch/filter` were created for, // `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with unsafe { @@ -809,8 +807,8 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIterationCursor<'w, 's, D, F> { } else { loop { if self.current_row == self.current_len { - let archetype_id = self.archetype_id_iter.next()?; - let archetype = archetypes.get(*archetype_id).debug_checked_unwrap(); + let archetype_id = self.storage_id_iter.next()?.archetype_id; + let archetype = archetypes.get(archetype_id).debug_checked_unwrap(); let table = tables.get(archetype.table_id()).debug_checked_unwrap(); // SAFETY: `archetype` and `tables` are from the world that `fetch/filter` were created for, // `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with diff --git a/crates/bevy_ecs/src/query/par_iter.rs b/crates/bevy_ecs/src/query/par_iter.rs index 0a7ec641d31e4..6af0b5aa5e816 100644 --- a/crates/bevy_ecs/src/query/par_iter.rs +++ b/crates/bevy_ecs/src/query/par_iter.rs @@ -164,17 +164,19 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryParIter<'w, 's, D, F> { // SAFETY: We only access table metadata. let tables = unsafe { &self.world.world_metadata().storages().tables }; self.state - .matched_table_ids + .matched_storage_ids .iter() - .map(|id| tables[*id].entity_count()) + // SAFETY: The if check ensures that matched_storage_ids stores TableIds + .map(|id| unsafe { tables[id.table_id].entity_count() }) .max() .unwrap_or(0) } else { let archetypes = &self.world.archetypes(); self.state - .matched_archetype_ids + .matched_storage_ids .iter() - .map(|id| archetypes[*id].len()) + // SAFETY: The if check ensures that matched_storage_ids stores ArchetypeIds + .map(|id| unsafe { archetypes[id.archetype_id].len() }) .max() .unwrap_or(0) }; diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 1a97e2bd70b4e..e15d150cf2b57 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -21,6 +21,17 @@ use super::{ QuerySingleError, ROQueryItem, }; +/// Either a table or an archetype. Used for Query iteration. +/// +/// # Safety +/// Must be initialized and accessed as a TableId, if both generic parameters to the query are dense. +/// Must be initialized and accessed as an ArchetypeId otherwise. +#[derive(Clone, Copy)] +pub(crate) union StorageId { + pub table_id: TableId, + pub archetype_id: ArchetypeId, +} + /// Provides scoped access to a [`World`] state according to a given [`QueryData`] and [`QueryFilter`]. #[repr(C)] // SAFETY NOTE: @@ -33,10 +44,8 @@ pub struct QueryState { pub(crate) matched_archetypes: FixedBitSet, pub(crate) archetype_component_access: Access, pub(crate) component_access: FilteredAccess, - // NOTE: we maintain both a TableId bitset and a vec because iterating the vec is faster - pub(crate) matched_table_ids: Vec, - // NOTE: we maintain both a ArchetypeId bitset and a vec because iterating the vec is faster - pub(crate) matched_archetype_ids: Vec, + // NOTE: we maintain both a bitset and a vec because iterating the vec is faster + pub(crate) matched_storage_ids: Vec, pub(crate) fetch_state: D::State, pub(crate) filter_state: F::State, #[cfg(feature = "trace")] @@ -45,11 +54,14 @@ pub struct QueryState { impl fmt::Debug for QueryState { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("QueryState") - .field("world_id", &self.world_id) - .field("matched_table_count", &self.matched_table_ids.len()) - .field("matched_archetype_count", &self.matched_archetype_ids.len()) - .finish_non_exhaustive() + let mut debug = f.debug_struct("QueryState"); + debug.field("world_id", &self.world_id); + if D::IS_DENSE && F::IS_DENSE { + debug.field("matched_table_count", &self.matched_storage_ids.len()); + } else { + debug.field("matched_archetype_count", &self.matched_storage_ids.len()); + } + debug.finish_non_exhaustive() } } @@ -105,16 +117,6 @@ impl QueryState { pub fn component_access(&self) -> &FilteredAccess { &self.component_access } - - /// Returns the tables matched by this query. - pub fn matched_tables(&self) -> &[TableId] { - &self.matched_table_ids - } - - /// Returns the archetypes matched by this query. - pub fn matched_archetypes(&self) -> &[ArchetypeId] { - &self.matched_archetype_ids - } } impl QueryState { @@ -139,8 +141,7 @@ impl QueryState { let mut state = Self { world_id: world.id(), archetype_generation: ArchetypeGeneration::initial(), - matched_table_ids: Vec::new(), - matched_archetype_ids: Vec::new(), + matched_storage_ids: Vec::new(), fetch_state, filter_state, component_access, @@ -167,8 +168,7 @@ impl QueryState { let mut state = Self { world_id: builder.world().id(), archetype_generation: ArchetypeGeneration::initial(), - matched_table_ids: Vec::new(), - matched_archetype_ids: Vec::new(), + matched_storage_ids: Vec::new(), fetch_state, filter_state, component_access: builder.access().clone(), @@ -306,13 +306,21 @@ impl QueryState { if !self.matched_archetypes.contains(archetype_index) { self.matched_archetypes.grow(archetype_index + 1); self.matched_archetypes.set(archetype_index, true); - self.matched_archetype_ids.push(archetype.id()); + if !D::IS_DENSE || !F::IS_DENSE { + self.matched_storage_ids.push(StorageId { + archetype_id: archetype.id() + }); + } } let table_index = archetype.table_id().as_usize(); if !self.matched_tables.contains(table_index) { self.matched_tables.grow(table_index + 1); self.matched_tables.set(table_index, true); - self.matched_table_ids.push(archetype.table_id()); + if D::IS_DENSE && F::IS_DENSE { + self.matched_storage_ids.push(StorageId { + table_id: archetype.table_id() + }); + } } } } @@ -384,8 +392,7 @@ impl QueryState { QueryState { world_id: self.world_id, archetype_generation: self.archetype_generation, - matched_table_ids: self.matched_table_ids.clone(), - matched_archetype_ids: self.matched_archetype_ids.clone(), + matched_storage_ids: self.matched_storage_ids.clone(), fetch_state, filter_state, component_access: self.component_access.clone(), @@ -480,20 +487,26 @@ impl QueryState { .matched_tables .intersection(&other.matched_tables) .collect(); - let matched_table_ids: Vec = - matched_tables.ones().map(TableId::from_usize).collect(); let matched_archetypes: FixedBitSet = self .matched_archetypes .intersection(&other.matched_archetypes) .collect(); - let matched_archetype_ids: Vec = - matched_archetypes.ones().map(ArchetypeId::new).collect(); + let matched_storage_ids = if NewD::IS_DENSE && NewF::IS_DENSE { + matched_tables.ones().map(|id| StorageId { + table_id: TableId::from_usize(id) + }) + .collect() + } else { + matched_archetypes.ones().map(|id| StorageId { + archetype_id: ArchetypeId::new(id) + }) + .collect() + }; QueryState { world_id: self.world_id, archetype_generation: self.archetype_generation, - matched_table_ids, - matched_archetype_ids, + matched_storage_ids, fetch_state: new_fetch_state, filter_state: new_filter_state, component_access: joined_component_access, @@ -1268,12 +1281,14 @@ impl QueryState { ) { // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: // QueryIter, QueryIterationCursor, QueryManyIter, QueryCombinationIter, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual + bevy_tasks::ComputeTaskPool::get().scope(|scope| { if D::IS_DENSE && F::IS_DENSE { // SAFETY: We only access table data that has been registered in `self.archetype_component_access`. let tables = unsafe { &world.storages().tables }; - for table_id in &self.matched_table_ids { - let table = &tables[*table_id]; + for storage_id in &self.matched_storage_ids { + let table_id = storage_id.table_id; + let table = &tables[table_id]; if table.is_empty() { continue; } @@ -1288,7 +1303,7 @@ impl QueryState { let table = &world .storages() .tables - .get(*table_id) + .get(table_id) .debug_checked_unwrap(); let batch = offset..offset + len; self.iter_unchecked_manual(world, last_run, this_run) @@ -1299,9 +1314,10 @@ impl QueryState { } } else { let archetypes = world.archetypes(); - for archetype_id in &self.matched_archetype_ids { + for storage_id in &self.matched_storage_ids { + let archetype_id = storage_id.archetype_id; let mut offset = 0; - let archetype = &archetypes[*archetype_id]; + let archetype = &archetypes[archetype_id]; if archetype.is_empty() { continue; } @@ -1313,7 +1329,7 @@ impl QueryState { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); let archetype = - world.archetypes().get(*archetype_id).debug_checked_unwrap(); + world.archetypes().get(archetype_id).debug_checked_unwrap(); let batch = offset..offset + len; self.iter_unchecked_manual(world, last_run, this_run) .for_each_in_archetype_range(&mut func, archetype, batch); From edcc766e341dc3a47ea873ba71b2032991cbba6d Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 14 Mar 2024 01:46:04 -0700 Subject: [PATCH 02/17] Adjust the if checks when updating archetypes --- crates/bevy_ecs/src/query/state.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index e15d150cf2b57..1736098dcd6d4 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -306,17 +306,20 @@ impl QueryState { if !self.matched_archetypes.contains(archetype_index) { self.matched_archetypes.grow(archetype_index + 1); self.matched_archetypes.set(archetype_index, true); + // This is inside the outer if because point queries (i.e. QueryState::get) + // always relies on matched_archetypes, while matched_tables is only use + // relied on for deduplication here. if !D::IS_DENSE || !F::IS_DENSE { self.matched_storage_ids.push(StorageId { archetype_id: archetype.id() }); } } - let table_index = archetype.table_id().as_usize(); - if !self.matched_tables.contains(table_index) { - self.matched_tables.grow(table_index + 1); - self.matched_tables.set(table_index, true); - if D::IS_DENSE && F::IS_DENSE { + if D::IS_DENSE && F::IS_DENSE { + let table_index = archetype.table_id().as_usize(); + if !self.matched_tables.contains(table_index) { + self.matched_tables.grow(table_index + 1); + self.matched_tables.set(table_index, true); self.matched_storage_ids.push(StorageId { table_id: archetype.table_id() }); From 057adf3a3e134c0924b8fd8c7ee47f0b166318cc Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 14 Mar 2024 02:19:57 -0700 Subject: [PATCH 03/17] Formatting --- crates/bevy_ecs/src/query/state.rs | 39 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 1736098dcd6d4..04a6e9e2cc0be 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -21,9 +21,9 @@ use super::{ QuerySingleError, ROQueryItem, }; -/// Either a table or an archetype. Used for Query iteration. -/// -/// # Safety +/// An ID for either a table or an archetype. Used for Query iteration. +/// +/// # Safety /// Must be initialized and accessed as a TableId, if both generic parameters to the query are dense. /// Must be initialized and accessed as an ArchetypeId otherwise. #[derive(Clone, Copy)] @@ -307,11 +307,11 @@ impl QueryState { self.matched_archetypes.grow(archetype_index + 1); self.matched_archetypes.set(archetype_index, true); // This is inside the outer if because point queries (i.e. QueryState::get) - // always relies on matched_archetypes, while matched_tables is only use + // always relies on matched_archetypes, while matched_tables is only use // relied on for deduplication here. if !D::IS_DENSE || !F::IS_DENSE { self.matched_storage_ids.push(StorageId { - archetype_id: archetype.id() + archetype_id: archetype.id(), }); } } @@ -321,7 +321,7 @@ impl QueryState { self.matched_tables.grow(table_index + 1); self.matched_tables.set(table_index, true); self.matched_storage_ids.push(StorageId { - table_id: archetype.table_id() + table_id: archetype.table_id(), }); } } @@ -495,15 +495,19 @@ impl QueryState { .intersection(&other.matched_archetypes) .collect(); let matched_storage_ids = if NewD::IS_DENSE && NewF::IS_DENSE { - matched_tables.ones().map(|id| StorageId { - table_id: TableId::from_usize(id) - }) - .collect() + matched_tables + .ones() + .map(|id| StorageId { + table_id: TableId::from_usize(id), + }) + .collect() } else { - matched_archetypes.ones().map(|id| StorageId { - archetype_id: ArchetypeId::new(id) - }) - .collect() + matched_archetypes + .ones() + .map(|id| StorageId { + archetype_id: ArchetypeId::new(id), + }) + .collect() }; QueryState { @@ -1303,11 +1307,8 @@ impl QueryState { scope.spawn(async move { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); - let table = &world - .storages() - .tables - .get(table_id) - .debug_checked_unwrap(); + let table = + &world.storages().tables.get(table_id).debug_checked_unwrap(); let batch = offset..offset + len; self.iter_unchecked_manual(world, last_run, this_run) .for_each_in_table_range(&mut func, table, batch); From 7881113e60280b09858ffbbe2aee9c0eabd01db5 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 15 Mar 2024 14:39:20 -0700 Subject: [PATCH 04/17] Cleanup code and docs --- crates/bevy_ecs/src/query/iter.rs | 11 ++++------- crates/bevy_ecs/src/query/par_iter.rs | 20 ++++++-------------- crates/bevy_ecs/src/query/state.rs | 2 +- 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 187786e59671f..411c059c0aab4 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -239,8 +239,8 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Iterator for QueryIter<'w, 's, D, F> let Some(item) = self.next() else { break }; accum = func(accum, item); } - if D::IS_DENSE && F::IS_DENSE { - for id in self.cursor.storage_id_iter.clone() { + for id in self.cursor.storage_id_iter.clone() { + if D::IS_DENSE && F::IS_DENSE { // SAFETY: Matched table IDs are guaranteed to still exist. let table = unsafe { self.tables.get(id.table_id).debug_checked_unwrap() }; accum = @@ -249,9 +249,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Iterator for QueryIter<'w, 's, D, F> // - The provided range is equivalent to [0, table.entity_count) // - The if block ensures that D::IS_DENSE and F::IS_DENSE are both true unsafe { self.fold_over_table_range(accum, &mut func, table, 0..table.entity_count()) }; - } - } else { - for id in self.cursor.storage_id_iter.clone() { + } else { let archetype = // SAFETY: Matched archetype IDs are guaranteed to still exist. unsafe { self.archetypes.get(id.archetype_id).debug_checked_unwrap() }; @@ -742,12 +740,11 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIterationCursor<'w, 's, D, F> { /// Note that if `D::IS_ARCHETYPAL && F::IS_ARCHETYPAL`, the return value /// will be **the exact count of remaining values**. fn max_remaining(&self, tables: &'w Tables, archetypes: &'w Archetypes) -> usize { + let ids = self.storage_id_iter.clone(); let remaining_matched: usize = if Self::IS_DENSE { - let ids = self.storage_id_iter.clone(); // SAFETY: The if check ensures that storage_id_iter stores TableIds unsafe { ids.map(|id| tables[id.table_id].entity_count()).sum() } } else { - let ids = self.storage_id_iter.clone(); // SAFETY: The if check ensures that storage_id_iter stores ArchetypeIds unsafe { ids.map(|id| archetypes[id.archetype_id].len()).sum() } }; diff --git a/crates/bevy_ecs/src/query/par_iter.rs b/crates/bevy_ecs/src/query/par_iter.rs index 6af0b5aa5e816..e393bf74a3ffc 100644 --- a/crates/bevy_ecs/src/query/par_iter.rs +++ b/crates/bevy_ecs/src/query/par_iter.rs @@ -160,26 +160,18 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryParIter<'w, 's, D, F> { thread_count > 0, "Attempted to run parallel iteration over a query with an empty TaskPool" ); + let id_iter = self.state.matched_storage_ids.iter(); let max_size = if D::IS_DENSE && F::IS_DENSE { // SAFETY: We only access table metadata. let tables = unsafe { &self.world.world_metadata().storages().tables }; - self.state - .matched_storage_ids - .iter() - // SAFETY: The if check ensures that matched_storage_ids stores TableIds - .map(|id| unsafe { tables[id.table_id].entity_count() }) - .max() - .unwrap_or(0) + // SAFETY: The if check ensures that matched_storage_ids stores TableIds + id_iter.map(|id| unsafe { tables[id.table_id].entity_count() }).max() } else { let archetypes = &self.world.archetypes(); - self.state - .matched_storage_ids - .iter() - // SAFETY: The if check ensures that matched_storage_ids stores ArchetypeIds - .map(|id| unsafe { archetypes[id.archetype_id].len() }) - .max() - .unwrap_or(0) + // SAFETY: The if check ensures that matched_storage_ids stores ArchetypeIds + id_iter.map(|id| unsafe { archetypes[id.archetype_id].len() }).max() }; + let max_size = max_size.unwrap_or(0); let batches = thread_count * self.batching_strategy.batches_per_thread; // Round up to the nearest batch size. diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 04a6e9e2cc0be..aac8dc22b8c9e 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -24,7 +24,7 @@ use super::{ /// An ID for either a table or an archetype. Used for Query iteration. /// /// # Safety -/// Must be initialized and accessed as a TableId, if both generic parameters to the query are dense. +/// Must be initialized and accessed as a [`TableId`], if both generic parameters to the query are dense. /// Must be initialized and accessed as an ArchetypeId otherwise. #[derive(Clone, Copy)] pub(crate) union StorageId { From 89c77fd90a45cdb45fee298b2f7d0bd85e5c65bf Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 15 Mar 2024 16:29:54 -0700 Subject: [PATCH 05/17] Formatting --- crates/bevy_ecs/src/query/par_iter.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/query/par_iter.rs b/crates/bevy_ecs/src/query/par_iter.rs index e393bf74a3ffc..1d18b69ca0469 100644 --- a/crates/bevy_ecs/src/query/par_iter.rs +++ b/crates/bevy_ecs/src/query/par_iter.rs @@ -164,12 +164,16 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryParIter<'w, 's, D, F> { let max_size = if D::IS_DENSE && F::IS_DENSE { // SAFETY: We only access table metadata. let tables = unsafe { &self.world.world_metadata().storages().tables }; - // SAFETY: The if check ensures that matched_storage_ids stores TableIds - id_iter.map(|id| unsafe { tables[id.table_id].entity_count() }).max() + id_iter + // SAFETY: The if check ensures that matched_storage_ids stores TableIds + .map(|id| unsafe { tables[id.table_id].entity_count() }) + .max() } else { let archetypes = &self.world.archetypes(); - // SAFETY: The if check ensures that matched_storage_ids stores ArchetypeIds - id_iter.map(|id| unsafe { archetypes[id.archetype_id].len() }).max() + id_iter + // SAFETY: The if check ensures that matched_storage_ids stores ArchetypeIds + .map(|id| unsafe { archetypes[id.archetype_id].len() }) + .max() }; let max_size = max_size.unwrap_or(0); From 8e8b82bdbf833bd10aaeebebb9a9796bccca1975 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 15 Mar 2024 16:35:59 -0700 Subject: [PATCH 06/17] Fix docs --- crates/bevy_ecs/src/query/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index aac8dc22b8c9e..e54999bb1284c 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -25,7 +25,7 @@ use super::{ /// /// # Safety /// Must be initialized and accessed as a [`TableId`], if both generic parameters to the query are dense. -/// Must be initialized and accessed as an ArchetypeId otherwise. +/// Must be initialized and accessed as an [`ArchetypeId`] otherwise. #[derive(Clone, Copy)] pub(crate) union StorageId { pub table_id: TableId, From f24f77f8332f416ed2ce20830a3e540713dacd65 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 15 Mar 2024 23:18:15 -0700 Subject: [PATCH 07/17] Fix soundness issue when joining queries --- crates/bevy_ecs/src/query/state.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 3ea398c17495e..a6f204c90b210 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -314,20 +314,17 @@ impl QueryState { if !self.matched_archetypes.contains(archetype_index) { self.matched_archetypes.grow(archetype_index + 1); self.matched_archetypes.set(archetype_index, true); - // This is inside the outer if because point queries (i.e. QueryState::get) - // always relies on matched_archetypes, while matched_tables is only use - // relied on for deduplication here. if !D::IS_DENSE || !F::IS_DENSE { self.matched_storage_ids.push(StorageId { archetype_id: archetype.id(), }); } } - if D::IS_DENSE && F::IS_DENSE { - let table_index = archetype.table_id().as_usize(); - if !self.matched_tables.contains(table_index) { - self.matched_tables.grow(table_index + 1); - self.matched_tables.set(table_index, true); + let table_index = archetype.table_id().as_usize(); + if !self.matched_tables.contains(table_index) { + self.matched_tables.grow(table_index + 1); + self.matched_tables.set(table_index, true); + if D::IS_DENSE && F::IS_DENSE { self.matched_storage_ids.push(StorageId { table_id: archetype.table_id(), }); From 69f267b2d1a731c8820269be74c69f79487afd4b Mon Sep 17 00:00:00 2001 From: James Liu Date: Sun, 17 Mar 2024 12:34:13 -0700 Subject: [PATCH 08/17] pub(crate) defensively Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/query/state.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index a6f204c90b210..7420a46f2a840 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -28,8 +28,8 @@ use super::{ /// Must be initialized and accessed as an [`ArchetypeId`] otherwise. #[derive(Clone, Copy)] pub(crate) union StorageId { - pub table_id: TableId, - pub archetype_id: ArchetypeId, + pub(crate) table_id: TableId, + pub(crate) archetype_id: ArchetypeId, } /// Provides scoped access to a [`World`] state according to a given [`QueryData`] and [`QueryFilter`]. From ad484eae98a0d3bfb2bd89c85dadeb50cb9cca2e Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 18 Mar 2024 00:10:57 -0700 Subject: [PATCH 09/17] Restore matched_tables and matched_archetypes --- crates/bevy_ecs/src/query/state.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index e87a5fffda564..dbb05161f263b 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -111,6 +111,20 @@ impl QueryState { pub fn component_access(&self) -> &FilteredAccess { &self.component_access } + + /// Returns the tables matched by this query. + pub fn matched_tables(&self) -> impl Iterator + '_ { + self.matched_tables + .ones() + .map(|index| TableId::from_usize(index)) + } + + /// Returns the archetypes matched by this query. + pub fn matched_archetypes(&self) -> impl Iterator + '_ { + self.matched_archetypes + .ones() + .map(|index| ArchetypeId::new(ibndex)) + } } impl QueryState { From caf91fd3079f6b146b05e6456e4c50f2dd6ee8ca Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 18 Mar 2024 00:21:38 -0700 Subject: [PATCH 10/17] Restore Debug implementation --- crates/bevy_ecs/src/query/state.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index dbb05161f263b..45928f709ec63 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -53,14 +53,14 @@ pub struct QueryState { impl fmt::Debug for QueryState { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let mut debug = f.debug_struct("QueryState"); - debug.field("world_id", &self.world_id); - if D::IS_DENSE && F::IS_DENSE { - debug.field("matched_table_count", &self.matched_storage_ids.len()); - } else { - debug.field("matched_archetype_count", &self.matched_storage_ids.len()); - } - debug.finish_non_exhaustive() + f.debug_struct("QueryState") + .field("world_id", &self.world_id) + .field("matched_table_count", &self.matched_tables.count_ones(..)) + .field( + "matched_archetype_count", + &self.matched_archetypes.count_ones(..), + ) + .finish_non_exhaustive() } } @@ -123,7 +123,7 @@ impl QueryState { pub fn matched_archetypes(&self) -> impl Iterator + '_ { self.matched_archetypes .ones() - .map(|index| ArchetypeId::new(ibndex)) + .map(|index| ArchetypeId::new(index)) } } From c7e5e8ebdc78d0ed5443481e77701a35ab44ff4d Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 18 Mar 2024 00:37:23 -0700 Subject: [PATCH 11/17] Fix CI and improve performance of bitset joins --- crates/bevy_ecs/src/query/state.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 45928f709ec63..d53c2eb1ea455 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -114,16 +114,12 @@ impl QueryState { /// Returns the tables matched by this query. pub fn matched_tables(&self) -> impl Iterator + '_ { - self.matched_tables - .ones() - .map(|index| TableId::from_usize(index)) + self.matched_tables.ones().map(TableId::from_usize) } /// Returns the archetypes matched by this query. pub fn matched_archetypes(&self) -> impl Iterator + '_ { - self.matched_archetypes - .ones() - .map(|index| ArchetypeId::new(index)) + self.matched_archetypes.ones().map(ArchetypeId::new) } } @@ -536,14 +532,10 @@ impl QueryState { } // take the intersection of the matched ids - let matched_tables: FixedBitSet = self - .matched_tables - .intersection(&other.matched_tables) - .collect(); - let matched_archetypes: FixedBitSet = self - .matched_archetypes - .intersection(&other.matched_archetypes) - .collect(); + let mut matched_tables = self.matched_tables.clone(); + let mut matched_archetypes = self.matched_archetypes.clone(); + matched_tables.intersect_with(&other.matched_tables); + matched_archetypes.intersect_with(&other.matched_archetypes); let matched_storage_ids = if NewD::IS_DENSE && NewF::IS_DENSE { matched_tables .ones() From f77497b3fa88d2dc03e739d8c55e2caf954e67d1 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 22 Mar 2024 19:08:27 -0700 Subject: [PATCH 12/17] Document more about StorageIds --- crates/bevy_ecs/src/query/state.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index d53c2eb1ea455..ffb07237ead53 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -22,6 +22,13 @@ use super::{ }; /// An ID for either a table or an archetype. Used for Query iteration. +/// +/// Query iteration is exclusively dense (over tables) or archetypal (over archetypes) based on whether +/// both D::IS_DENSE and F::IS_DENSE are true or not. +/// +/// This is a union instead of an enum as the usage is determined at compile time, as all StorageIds for +/// a QueryState will be TableIds or ArchetypeIds, and not a mixture of both. This removes the need for +/// discriminator to minimize memory usage and branching during iteration. /// /// # Safety /// Must be initialized and accessed as a [`TableId`], if both generic parameters to the query are dense. From 361decab2f11060c5039c87c2e1015b3b0367e7a Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 22 Mar 2024 23:19:37 -0700 Subject: [PATCH 13/17] Foramtting --- crates/bevy_ecs/src/query/state.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index ffb07237ead53..818549bcc43e1 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -22,10 +22,10 @@ use super::{ }; /// An ID for either a table or an archetype. Used for Query iteration. -/// +/// /// Query iteration is exclusively dense (over tables) or archetypal (over archetypes) based on whether /// both D::IS_DENSE and F::IS_DENSE are true or not. -/// +/// /// This is a union instead of an enum as the usage is determined at compile time, as all StorageIds for /// a QueryState will be TableIds or ArchetypeIds, and not a mixture of both. This removes the need for /// discriminator to minimize memory usage and branching during iteration. From 16fd5236c6cb25c9b390718de7924e01dfeb4290 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 22 Mar 2024 23:47:30 -0700 Subject: [PATCH 14/17] Cleanup parallel iteration code --- crates/bevy_ecs/src/query/state.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 818549bcc43e1..583126723d425 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1338,10 +1338,11 @@ impl QueryState { // QueryIter, QueryIterationCursor, QueryManyIter, QueryCombinationIter, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual bevy_tasks::ComputeTaskPool::get().scope(|scope| { - if D::IS_DENSE && F::IS_DENSE { - // SAFETY: We only access table data that has been registered in `self.archetype_component_access`. - let tables = unsafe { &world.storages().tables }; - for storage_id in &self.matched_storage_ids { + // SAFETY: We only access table data that has been registered in `self.archetype_component_access`. + let tables = unsafe { &world.storages().tables }; + let archetypes = world.archetypes(); + for storage_id in &self.matched_storage_ids { + if D::IS_DENSE && F::IS_DENSE { let table_id = storage_id.table_id; let table = &tables[table_id]; if table.is_empty() { @@ -1352,37 +1353,34 @@ impl QueryState { while offset < table.entity_count() { let mut func = func.clone(); let len = batch_size.min(table.entity_count() - offset); + let batch = offset..offset + len; scope.spawn(async move { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); let table = &world.storages().tables.get(table_id).debug_checked_unwrap(); - let batch = offset..offset + len; self.iter_unchecked_manual(world, last_run, this_run) .for_each_in_table_range(&mut func, table, batch); }); offset += batch_size; } - } - } else { - let archetypes = world.archetypes(); - for storage_id in &self.matched_storage_ids { + } else { let archetype_id = storage_id.archetype_id; - let mut offset = 0; let archetype = &archetypes[archetype_id]; if archetype.is_empty() { continue; } + let mut offset = 0; while offset < archetype.len() { let mut func = func.clone(); let len = batch_size.min(archetype.len() - offset); + let batch = offset..offset + len; scope.spawn(async move { #[cfg(feature = "trace")] let _span = self.par_iter_span.enter(); let archetype = world.archetypes().get(archetype_id).debug_checked_unwrap(); - let batch = offset..offset + len; self.iter_unchecked_manual(world, last_run, this_run) .for_each_in_archetype_range(&mut func, archetype, batch); }); From 18932680eec5e0fba04ec08a8f1dabe821306797 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 23 Mar 2024 01:57:43 -0700 Subject: [PATCH 15/17] Fix docs --- crates/bevy_ecs/src/query/state.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 583126723d425..41a9f905c0e04 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -23,20 +23,21 @@ use super::{ /// An ID for either a table or an archetype. Used for Query iteration. /// -/// Query iteration is exclusively dense (over tables) or archetypal (over archetypes) based on whether +/// Query iteration is exclusively dense (over tables) or archetypal (over archetypes) based on whether /// both D::IS_DENSE and F::IS_DENSE are true or not. /// -/// This is a union instead of an enum as the usage is determined at compile time, as all StorageIds for -/// a QueryState will be TableIds or ArchetypeIds, and not a mixture of both. This removes the need for -/// discriminator to minimize memory usage and branching during iteration. +/// This is a union instead of an enum as the usage is determined at compile time, as all [`StorageId`]s for +/// a [`QueryState`] will be all [`TableId`]s or all [`ArchetypeId`]s, and not a mixture of both. This +/// removes the need for discriminator to minimize memory usage and branching during iteration, but requires +/// a safety invariant be verified when disambiguating them. /// /// # Safety /// Must be initialized and accessed as a [`TableId`], if both generic parameters to the query are dense. /// Must be initialized and accessed as an [`ArchetypeId`] otherwise. #[derive(Clone, Copy)] -pub(crate) union StorageId { - pub(crate) table_id: TableId, - pub(crate) archetype_id: ArchetypeId, +pub(super) union StorageId { + pub(super) table_id: TableId, + pub(super) archetype_id: ArchetypeId, } /// Provides scoped access to a [`World`] state according to a given [`QueryData`] and [`QueryFilter`]. @@ -51,7 +52,7 @@ pub struct QueryState { pub(crate) matched_archetypes: FixedBitSet, pub(crate) component_access: FilteredAccess, // NOTE: we maintain both a bitset and a vec because iterating the vec is faster - pub(crate) matched_storage_ids: Vec, + pub(super) matched_storage_ids: Vec, pub(crate) fetch_state: D::State, pub(crate) filter_state: F::State, #[cfg(feature = "trace")] From 3cfebae4a03f8f5306bc04f7b131c36f42dbd8b1 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 24 Mar 2024 20:08:39 -0700 Subject: [PATCH 16/17] Formatting --- crates/bevy_ecs/src/query/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 41a9f905c0e04..b15358d10a3a8 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -27,7 +27,7 @@ use super::{ /// both D::IS_DENSE and F::IS_DENSE are true or not. /// /// This is a union instead of an enum as the usage is determined at compile time, as all [`StorageId`]s for -/// a [`QueryState`] will be all [`TableId`]s or all [`ArchetypeId`]s, and not a mixture of both. This +/// a [`QueryState`] will be all [`TableId`]s or all [`ArchetypeId`]s, and not a mixture of both. This /// removes the need for discriminator to minimize memory usage and branching during iteration, but requires /// a safety invariant be verified when disambiguating them. /// From c0f08350e3a8aece522fb3bc42b89a976d52391e Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Mon, 25 Mar 2024 00:47:59 -0400 Subject: [PATCH 17/17] Backticks for CI --- crates/bevy_ecs/src/query/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index b15358d10a3a8..453e74a2d4bd7 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -24,7 +24,7 @@ use super::{ /// An ID for either a table or an archetype. Used for Query iteration. /// /// Query iteration is exclusively dense (over tables) or archetypal (over archetypes) based on whether -/// both D::IS_DENSE and F::IS_DENSE are true or not. +/// both `D::IS_DENSE` and `F::IS_DENSE` are true or not. /// /// This is a union instead of an enum as the usage is determined at compile time, as all [`StorageId`]s for /// a [`QueryState`] will be all [`TableId`]s or all [`ArchetypeId`]s, and not a mixture of both. This