From 58e5dcda86d6eb00fd82f1739ca9a4259c58528a Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Thu, 30 Nov 2023 13:59:24 +1100 Subject: [PATCH 1/5] Replaced `TableRow` `new` and `index` with `from_*` and `as_*` Localizes all instances of `as` casting into a pair of properly checked functions with `debug_assertions`. By using `debug_assertions` any runtime costs are avoided. --- crates/bevy_ecs/src/query/fetch.rs | 14 +- crates/bevy_ecs/src/query/filter.rs | 4 +- crates/bevy_ecs/src/query/iter.rs | 8 +- crates/bevy_ecs/src/query/state.rs | 6 +- crates/bevy_ecs/src/storage/resource.rs | 2 +- crates/bevy_ecs/src/storage/sparse_set.rs | 82 ++++---- crates/bevy_ecs/src/storage/table.rs | 226 ++++++++++++++-------- 7 files changed, 197 insertions(+), 145 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 00df1fe63e17c..5ecd94a562058 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -605,7 +605,7 @@ unsafe impl WorldQuery for &T { StorageType::Table => fetch .table_components .debug_checked_unwrap() - .get(table_row.index()) + .get(table_row.as_usize()) .deref(), StorageType::SparseSet => fetch .sparse_set @@ -760,10 +760,10 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { let (table_components, added_ticks, changed_ticks) = fetch.table_data.debug_checked_unwrap(); Ref { - value: table_components.get(table_row.index()).deref(), + value: table_components.get(table_row.as_usize()).deref(), ticks: Ticks { - added: added_ticks.get(table_row.index()).deref(), - changed: changed_ticks.get(table_row.index()).deref(), + added: added_ticks.get(table_row.as_usize()).deref(), + changed: changed_ticks.get(table_row.as_usize()).deref(), this_run: fetch.this_run, last_run: fetch.last_run, }, @@ -927,10 +927,10 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { let (table_components, added_ticks, changed_ticks) = fetch.table_data.debug_checked_unwrap(); Mut { - value: table_components.get(table_row.index()).deref_mut(), + value: table_components.get(table_row.as_usize()).deref_mut(), ticks: TicksMut { - added: added_ticks.get(table_row.index()).deref_mut(), - changed: changed_ticks.get(table_row.index()).deref_mut(), + added: added_ticks.get(table_row.as_usize()).deref_mut(), + changed: changed_ticks.get(table_row.as_usize()).deref_mut(), this_run: fetch.this_run, last_run: fetch.last_run, }, diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index dcc2cedeea20b..6e5f8a86d7259 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -626,7 +626,7 @@ unsafe impl WorldQuery for Added { StorageType::Table => fetch .table_ticks .debug_checked_unwrap() - .get(table_row.index()) + .get(table_row.as_usize()) .deref() .is_newer_than(fetch.last_run, fetch.this_run), StorageType::SparseSet => { @@ -802,7 +802,7 @@ unsafe impl WorldQuery for Changed { StorageType::Table => fetch .table_ticks .debug_checked_unwrap() - .get(table_row.index()) + .get(table_row.as_usize()) .deref() .is_newer_than(fetch.last_run, fetch.this_run), StorageType::SparseSet => { diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index f5359a25fbf07..af704f97a9f8c 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -517,7 +517,11 @@ impl<'w, 's, Q: WorldQueryData, F: WorldQueryFilter> QueryIterationCursor<'w, 's let index = self.current_row - 1; if Self::IS_DENSE { let entity = self.table_entities.get_unchecked(index); - Some(Q::fetch(&mut self.fetch, *entity, TableRow::new(index))) + Some(Q::fetch( + &mut self.fetch, + *entity, + TableRow::from_usize(index), + )) } else { let archetype_entity = self.archetype_entities.get_unchecked(index); Some(Q::fetch( @@ -578,7 +582,7 @@ impl<'w, 's, Q: WorldQueryData, F: WorldQueryFilter> QueryIterationCursor<'w, 's // SAFETY: set_table was called prior. // `current_row` is a table row in range of the current table, because if it was not, then the if above would have been executed. let entity = self.table_entities.get_unchecked(self.current_row); - let row = TableRow::new(self.current_row); + let row = TableRow::from_usize(self.current_row); if !F::filter_fetch(&mut self.filter, *entity, row) { self.current_row += 1; continue; diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index d4afbfe5ac9c0..9791c9a7aaba9 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -267,7 +267,7 @@ impl QueryState { self.matched_archetypes.set(archetype_index, true); self.matched_archetype_ids.push(archetype.id()); } - let table_index = archetype.table_id().index(); + 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); @@ -1128,7 +1128,7 @@ impl QueryState { let entities = table.entities(); for row in 0..table.entity_count() { let entity = entities.get_unchecked(row); - let row = TableRow::new(row); + let row = TableRow::from_usize(row); if !F::filter_fetch(&mut filter, *entity, row) { continue; } @@ -1221,7 +1221,7 @@ impl QueryState { F::set_table(&mut filter, &self.filter_state, table); for row in offset..offset + len { let entity = entities.get_unchecked(row); - let row = TableRow::new(row); + let row = TableRow::from_usize(row); if !F::filter_fetch(&mut filter, *entity, row) { continue; } diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index c9e771e2aac06..c130e3e232b62 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -40,7 +40,7 @@ impl Drop for ResourceData { impl ResourceData { /// The only row in the underlying column. - const ROW: TableRow = TableRow::new(0); + const ROW: TableRow = TableRow::from_u32(0); /// Validates the access to `!Send` resources is only done on the thread they were created from. /// diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 9779f38d9185b..dfb03f88681d2 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -124,7 +124,7 @@ pub struct ComponentSparseSet { entities: Vec, #[cfg(debug_assertions)] entities: Vec, - sparse: SparseArray, + sparse: SparseArray, } impl ComponentSparseSet { @@ -171,13 +171,13 @@ impl ComponentSparseSet { ) { if let Some(&dense_index) = self.sparse.get(entity.index()) { #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index as usize]); - self.dense - .replace(TableRow::new(dense_index as usize), value, change_tick); + assert_eq!(entity, self.entities[dense_index.as_usize()]); + self.dense.replace(dense_index, value, change_tick); } else { let dense_index = self.dense.len(); self.dense.push(value, ComponentTicks::new(change_tick)); - self.sparse.insert(entity.index(), dense_index as u32); + self.sparse + .insert(entity.index(), TableRow::from_usize(dense_index)); #[cfg(debug_assertions)] assert_eq!(self.entities.len(), dense_index); #[cfg(not(debug_assertions))] @@ -194,7 +194,7 @@ impl ComponentSparseSet { { if let Some(&dense_index) = self.sparse.get(entity.index()) { #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index as usize]); + assert_eq!(entity, self.entities[dense_index.as_usize()]); true } else { false @@ -209,12 +209,11 @@ impl ComponentSparseSet { /// Returns `None` if `entity` does not have a component in the sparse set. #[inline] pub fn get(&self, entity: Entity) -> Option> { - self.sparse.get(entity.index()).map(|dense_index| { - let dense_index = (*dense_index) as usize; + self.sparse.get(entity.index()).map(|&dense_index| { #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index]); + assert_eq!(entity, self.entities[dense_index.as_usize()]); // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.get_data_unchecked(TableRow::new(dense_index)) } + unsafe { self.dense.get_data_unchecked(dense_index) } }) } @@ -223,9 +222,9 @@ impl ComponentSparseSet { /// Returns `None` if `entity` does not have a component in the sparse set. #[inline] pub fn get_with_ticks(&self, entity: Entity) -> Option<(Ptr<'_>, TickCells<'_>)> { - let dense_index = TableRow::new(*self.sparse.get(entity.index())? as usize); + let dense_index = *self.sparse.get(entity.index())?; #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index.index()]); + assert_eq!(entity, self.entities[dense_index.as_usize()]); // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { Some(( @@ -243,16 +242,11 @@ impl ComponentSparseSet { /// Returns `None` if `entity` does not have a component in the sparse set. #[inline] pub fn get_added_tick(&self, entity: Entity) -> Option<&UnsafeCell> { - let dense_index = *self.sparse.get(entity.index())? as usize; + let dense_index = *self.sparse.get(entity.index())?; #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index]); + assert_eq!(entity, self.entities[dense_index.as_usize()]); // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { - Some( - self.dense - .get_added_tick_unchecked(TableRow::new(dense_index)), - ) - } + unsafe { Some(self.dense.get_added_tick_unchecked(dense_index)) } } /// Returns a reference to the "changed" tick of the entity's component value. @@ -260,16 +254,11 @@ impl ComponentSparseSet { /// Returns `None` if `entity` does not have a component in the sparse set. #[inline] pub fn get_changed_tick(&self, entity: Entity) -> Option<&UnsafeCell> { - let dense_index = *self.sparse.get(entity.index())? as usize; + let dense_index = *self.sparse.get(entity.index())?; #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index]); + assert_eq!(entity, self.entities[dense_index.as_usize()]); // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { - Some( - self.dense - .get_changed_tick_unchecked(TableRow::new(dense_index)), - ) - } + unsafe { Some(self.dense.get_changed_tick_unchecked(dense_index)) } } /// Returns a reference to the "added" and "changed" ticks of the entity's component value. @@ -277,11 +266,11 @@ impl ComponentSparseSet { /// Returns `None` if `entity` does not have a component in the sparse set. #[inline] pub fn get_ticks(&self, entity: Entity) -> Option { - let dense_index = *self.sparse.get(entity.index())? as usize; + let dense_index = *self.sparse.get(entity.index())?; #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index]); + assert_eq!(entity, self.entities[dense_index.as_usize()]); // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { Some(self.dense.get_ticks_unchecked(TableRow::new(dense_index))) } + unsafe { Some(self.dense.get_ticks_unchecked(dense_index)) } } /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if @@ -289,23 +278,19 @@ impl ComponentSparseSet { #[must_use = "The returned pointer must be used to drop the removed component."] pub(crate) fn remove_and_forget(&mut self, entity: Entity) -> Option> { self.sparse.remove(entity.index()).map(|dense_index| { - let dense_index = dense_index as usize; #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index]); - self.entities.swap_remove(dense_index); - let is_last = dense_index == self.dense.len() - 1; + assert_eq!(entity, self.entities[dense_index.as_usize()]); + self.entities.swap_remove(dense_index.as_usize()); + let is_last = dense_index.as_usize() == self.dense.len() - 1; // SAFETY: dense_index was just removed from `sparse`, which ensures that it is valid - let (value, _) = unsafe { - self.dense - .swap_remove_and_forget_unchecked(TableRow::new(dense_index)) - }; + let (value, _) = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) }; if !is_last { - let swapped_entity = self.entities[dense_index]; + let swapped_entity = self.entities[dense_index.as_usize()]; #[cfg(not(debug_assertions))] let index = swapped_entity; #[cfg(debug_assertions)] let index = swapped_entity.index(); - *self.sparse.get_mut(index).unwrap() = dense_index as u32; + *self.sparse.get_mut(index).unwrap() = dense_index; } value }) @@ -316,20 +301,21 @@ impl ComponentSparseSet { /// Returns `true` if `entity` had a component value in the sparse set. pub(crate) fn remove(&mut self, entity: Entity) -> bool { if let Some(dense_index) = self.sparse.remove(entity.index()) { - let dense_index = dense_index as usize; #[cfg(debug_assertions)] - assert_eq!(entity, self.entities[dense_index]); - self.entities.swap_remove(dense_index); - let is_last = dense_index == self.dense.len() - 1; + assert_eq!(entity, self.entities[dense_index.as_usize()]); + self.entities.swap_remove(dense_index.as_usize()); + let is_last = dense_index.as_usize() == self.dense.len() - 1; // SAFETY: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.swap_remove_unchecked(TableRow::new(dense_index)) } + unsafe { + self.dense.swap_remove_unchecked(dense_index); + } if !is_last { - let swapped_entity = self.entities[dense_index]; + let swapped_entity = self.entities[dense_index.as_usize()]; #[cfg(not(debug_assertions))] let index = swapped_entity; #[cfg(debug_assertions)] let index = swapped_entity.index(); - *self.sparse.get_mut(index).unwrap() = dense_index as u32; + *self.sparse.get_mut(index).unwrap() = dense_index; } true } else { diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index e1955bc598f89..d11d3fcc76768 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -35,25 +35,52 @@ impl TableId { /// Creates a new [`TableId`]. /// - /// `index` *must* be retrieved from calling [`TableId::index`] on a `TableId` you got + /// `index` *must* be retrieved from calling [`TableId::as_u32`] on a `TableId` you got /// from a table of a given [`World`] or the created ID may be invalid. /// /// [`World`]: crate::world::World #[inline] - pub fn new(index: usize) -> Self { + pub const fn from_u32(index: u32) -> Self { + TableId(index) + } + + /// Creates a new [`TableId`]. + /// + /// `index` *must* be retrieved from calling [`TableId::as_usize`] on a `TableId` you got + /// from a table of a given [`World`] or the created ID may be invalid. + /// + /// [`World`]: crate::world::World + /// + /// # Panics + /// + /// Will panic if the provided value does not fit within a [`u32`] with `debug_assertions`. + #[inline] + pub const fn from_usize(index: usize) -> Self { + debug_assert!((index as u128) < (u32::MAX as u128)); TableId(index as u32) } /// Gets the underlying table index from the ID. #[inline] - pub fn index(self) -> usize { + pub const fn as_u32(self) -> u32 { + self.0 + } + + /// Gets the underlying table index from the ID. + /// + /// # Panics + /// + /// Will panic if the inner value does not fit within a [`usize`] with `debug_assertions`. + #[inline] + pub const fn as_usize(self) -> usize { + debug_assert!((self.0 as u128) < (usize::MAX as u128)); self.0 as usize } /// The [`TableId`] of the [`Table`] without any components. #[inline] pub const fn empty() -> TableId { - TableId(0) + Self(0) } } @@ -82,15 +109,37 @@ impl TableRow { /// Creates a `TableRow`. #[inline] - pub const fn new(index: usize) -> Self { + pub const fn from_u32(index: u32) -> Self { + Self(index) + } + + /// Creates a `TableRow` from a [`usize`] index. + /// + /// # Panics + /// + /// Will panic if the provided value does not fit within a [`u32`] with `debug_assertions`. + #[inline] + pub const fn from_usize(index: usize) -> Self { + debug_assert!((index as u128) < (u32::MAX as u128)); Self(index as u32) } - /// Gets the index of the row. + /// Gets the index of the row as a [`usize`]. + /// + /// # Panics + /// + /// Will panic if the inner value does not fit within a [`usize`] with `debug_assertions`. #[inline] - pub const fn index(self) -> usize { + pub const fn as_usize(self) -> usize { + debug_assert!((self.0 as u128) < (usize::MAX as u128)); self.0 as usize } + + /// Gets the index of the row as a [`usize`]. + #[inline] + pub const fn as_u32(self) -> u32 { + self.0 + } } /// A type-erased contiguous container for data of a homogeneous type. @@ -139,10 +188,13 @@ impl Column { /// Assumes data has already been allocated for the given row. #[inline] pub(crate) unsafe fn initialize(&mut self, row: TableRow, data: OwningPtr<'_>, tick: Tick) { - debug_assert!(row.index() < self.len()); - self.data.initialize_unchecked(row.index(), data); - *self.added_ticks.get_unchecked_mut(row.index()).get_mut() = tick; - *self.changed_ticks.get_unchecked_mut(row.index()).get_mut() = tick; + debug_assert!(row.as_usize() < self.len()); + self.data.initialize_unchecked(row.as_usize(), data); + *self.added_ticks.get_unchecked_mut(row.as_usize()).get_mut() = tick; + *self + .changed_ticks + .get_unchecked_mut(row.as_usize()) + .get_mut() = tick; } /// Writes component data to the column at given row. @@ -152,9 +204,12 @@ impl Column { /// Assumes data has already been allocated for the given row. #[inline] pub(crate) unsafe fn replace(&mut self, row: TableRow, data: OwningPtr<'_>, change_tick: Tick) { - debug_assert!(row.index() < self.len()); - self.data.replace_unchecked(row.index(), data); - *self.changed_ticks.get_unchecked_mut(row.index()).get_mut() = change_tick; + debug_assert!(row.as_usize() < self.len()); + self.data.replace_unchecked(row.as_usize(), data); + *self + .changed_ticks + .get_unchecked_mut(row.as_usize()) + .get_mut() = change_tick; } /// Writes component data to the column at given row. @@ -165,8 +220,8 @@ impl Column { /// Assumes data has already been allocated for the given row. #[inline] pub(crate) unsafe fn replace_untracked(&mut self, row: TableRow, data: OwningPtr<'_>) { - debug_assert!(row.index() < self.len()); - self.data.replace_unchecked(row.index(), data); + debug_assert!(row.as_usize() < self.len()); + self.data.replace_unchecked(row.as_usize(), data); } /// Gets the current number of elements stored in the column. @@ -193,9 +248,9 @@ impl Column { /// #[inline] pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: TableRow) { - self.data.swap_remove_and_drop_unchecked(row.index()); - self.added_ticks.swap_remove(row.index()); - self.changed_ticks.swap_remove(row.index()); + self.data.swap_remove_and_drop_unchecked(row.as_usize()); + self.added_ticks.swap_remove(row.as_usize()); + self.changed_ticks.swap_remove(row.as_usize()); } /// Removes an element from the [`Column`] and returns it and its change detection ticks. @@ -214,11 +269,11 @@ impl Column { &mut self, row: TableRow, ) -> Option<(OwningPtr<'_>, ComponentTicks)> { - (row.index() < self.data.len()).then(|| { + (row.as_usize() < self.data.len()).then(|| { // SAFETY: The row was length checked before this. - let data = unsafe { self.data.swap_remove_and_forget_unchecked(row.index()) }; - let added = self.added_ticks.swap_remove(row.index()).into_inner(); - let changed = self.changed_ticks.swap_remove(row.index()).into_inner(); + let data = unsafe { self.data.swap_remove_and_forget_unchecked(row.as_usize()) }; + let added = self.added_ticks.swap_remove(row.as_usize()).into_inner(); + let changed = self.changed_ticks.swap_remove(row.as_usize()).into_inner(); (data, ComponentTicks { added, changed }) }) } @@ -241,9 +296,9 @@ impl Column { &mut self, row: TableRow, ) -> (OwningPtr<'_>, ComponentTicks) { - let data = self.data.swap_remove_and_forget_unchecked(row.index()); - let added = self.added_ticks.swap_remove(row.index()).into_inner(); - let changed = self.changed_ticks.swap_remove(row.index()).into_inner(); + let data = self.data.swap_remove_and_forget_unchecked(row.as_usize()); + let added = self.added_ticks.swap_remove(row.as_usize()).into_inner(); + let changed = self.changed_ticks.swap_remove(row.as_usize()).into_inner(); (data, ComponentTicks { added, changed }) } @@ -266,12 +321,12 @@ impl Column { dst_row: TableRow, ) { debug_assert!(self.data.layout() == other.data.layout()); - let ptr = self.data.get_unchecked_mut(dst_row.index()); - other.data.swap_remove_unchecked(src_row.index(), ptr); - *self.added_ticks.get_unchecked_mut(dst_row.index()) = - other.added_ticks.swap_remove(src_row.index()); - *self.changed_ticks.get_unchecked_mut(dst_row.index()) = - other.changed_ticks.swap_remove(src_row.index()); + let ptr = self.data.get_unchecked_mut(dst_row.as_usize()); + other.data.swap_remove_unchecked(src_row.as_usize(), ptr); + *self.added_ticks.get_unchecked_mut(dst_row.as_usize()) = + other.added_ticks.swap_remove(src_row.as_usize()); + *self.changed_ticks.get_unchecked_mut(dst_row.as_usize()) = + other.changed_ticks.swap_remove(src_row.as_usize()); } /// Pushes a new value onto the end of the [`Column`]. @@ -338,15 +393,15 @@ impl Column { /// Returns `None` if `row` is out of bounds. #[inline] pub fn get(&self, row: TableRow) -> Option<(Ptr<'_>, TickCells<'_>)> { - (row.index() < self.data.len()) + (row.as_usize() < self.data.len()) // SAFETY: The row is length checked before fetching the pointer. This is being // accessed through a read-only reference to the column. .then(|| unsafe { ( - self.data.get_unchecked(row.index()), + self.data.get_unchecked(row.as_usize()), TickCells { - added: self.added_ticks.get_unchecked(row.index()), - changed: self.changed_ticks.get_unchecked(row.index()), + added: self.added_ticks.get_unchecked(row.as_usize()), + changed: self.changed_ticks.get_unchecked(row.as_usize()), }, ) }) @@ -357,9 +412,11 @@ impl Column { /// Returns `None` if `row` is out of bounds. #[inline] pub fn get_data(&self, row: TableRow) -> Option> { - // SAFETY: The row is length checked before fetching the pointer. This is being - // accessed through a read-only reference to the column. - (row.index() < self.data.len()).then(|| unsafe { self.data.get_unchecked(row.index()) }) + (row.as_usize() < self.data.len()).then(|| { + // SAFETY: The row is length checked before fetching the pointer. This is being + // accessed through a read-only reference to the column. + unsafe { self.data.get_unchecked(row.as_usize()) } + }) } /// Fetches a read-only reference to the data at `row`. Unlike [`Column::get`] this does not @@ -370,8 +427,8 @@ impl Column { /// - no other mutable reference to the data of the same row can exist at the same time #[inline] pub unsafe fn get_data_unchecked(&self, row: TableRow) -> Ptr<'_> { - debug_assert!(row.index() < self.data.len()); - self.data.get_unchecked(row.index()) + debug_assert!(row.as_usize() < self.data.len()); + self.data.get_unchecked(row.as_usize()) } /// Fetches a mutable reference to the data at `row`. @@ -379,9 +436,11 @@ impl Column { /// Returns `None` if `row` is out of bounds. #[inline] pub fn get_data_mut(&mut self, row: TableRow) -> Option> { - // SAFETY: The row is length checked before fetching the pointer. This is being - // accessed through an exclusive reference to the column. - (row.index() < self.data.len()).then(|| unsafe { self.data.get_unchecked_mut(row.index()) }) + (row.as_usize() < self.data.len()).then(|| { + // SAFETY: The row is length checked before fetching the pointer. This is being + // accessed through an exclusive reference to the column. + unsafe { self.data.get_unchecked_mut(row.as_usize()) } + }) } /// Fetches a mutable reference to the data at `row`. Unlike [`Column::get_data_mut`] this does not @@ -392,8 +451,8 @@ impl Column { /// - no other reference to the data of the same row can exist at the same time #[inline] pub(crate) unsafe fn get_data_unchecked_mut(&mut self, row: TableRow) -> PtrMut<'_> { - debug_assert!(row.index() < self.data.len()); - self.data.get_unchecked_mut(row.index()) + debug_assert!(row.as_usize() < self.data.len()); + self.data.get_unchecked_mut(row.as_usize()) } /// Fetches the "added" change detection tick for the value at `row`. @@ -405,7 +464,7 @@ impl Column { /// adhere to the safety invariants of [`UnsafeCell`]. #[inline] pub fn get_added_tick(&self, row: TableRow) -> Option<&UnsafeCell> { - self.added_ticks.get(row.index()) + self.added_ticks.get(row.as_usize()) } /// Fetches the "changed" change detection tick for the value at `row`. @@ -417,7 +476,7 @@ impl Column { /// adhere to the safety invariants of [`UnsafeCell`]. #[inline] pub fn get_changed_tick(&self, row: TableRow) -> Option<&UnsafeCell> { - self.changed_ticks.get(row.index()) + self.changed_ticks.get(row.as_usize()) } /// Fetches the change detection ticks for the value at `row`. @@ -425,7 +484,7 @@ impl Column { /// Returns `None` if `row` is out of bounds. #[inline] pub fn get_ticks(&self, row: TableRow) -> Option { - if row.index() < self.data.len() { + if row.as_usize() < self.data.len() { // SAFETY: The size of the column has already been checked. Some(unsafe { self.get_ticks_unchecked(row) }) } else { @@ -440,8 +499,8 @@ impl Column { /// `row` must be within the range `[0, self.len())`. #[inline] pub unsafe fn get_added_tick_unchecked(&self, row: TableRow) -> &UnsafeCell { - debug_assert!(row.index() < self.added_ticks.len()); - self.added_ticks.get_unchecked(row.index()) + debug_assert!(row.as_usize() < self.added_ticks.len()); + self.added_ticks.get_unchecked(row.as_usize()) } /// Fetches the "changed" change detection tick for the value at `row`. Unlike [`Column::get_changed_tick`] @@ -451,8 +510,8 @@ impl Column { /// `row` must be within the range `[0, self.len())`. #[inline] pub unsafe fn get_changed_tick_unchecked(&self, row: TableRow) -> &UnsafeCell { - debug_assert!(row.index() < self.changed_ticks.len()); - self.changed_ticks.get_unchecked(row.index()) + debug_assert!(row.as_usize() < self.changed_ticks.len()); + self.changed_ticks.get_unchecked(row.as_usize()) } /// Fetches the change detection ticks for the value at `row`. Unlike [`Column::get_ticks`] @@ -462,11 +521,11 @@ impl Column { /// `row` must be within the range `[0, self.len())`. #[inline] pub unsafe fn get_ticks_unchecked(&self, row: TableRow) -> ComponentTicks { - debug_assert!(row.index() < self.added_ticks.len()); - debug_assert!(row.index() < self.changed_ticks.len()); + debug_assert!(row.as_usize() < self.added_ticks.len()); + debug_assert!(row.as_usize() < self.changed_ticks.len()); ComponentTicks { - added: self.added_ticks.get_unchecked(row.index()).read(), - changed: self.changed_ticks.get_unchecked(row.index()).read(), + added: self.added_ticks.get_unchecked(row.as_usize()).read(), + changed: self.changed_ticks.get_unchecked(row.as_usize()).read(), } } @@ -565,12 +624,12 @@ impl Table { for column in self.columns.values_mut() { column.swap_remove_unchecked(row); } - let is_last = row.index() == self.entities.len() - 1; - self.entities.swap_remove(row.index()); + let is_last = row.as_usize() == self.entities.len() - 1; + self.entities.swap_remove(row.as_usize()); if is_last { None } else { - Some(self.entities[row.index()]) + Some(self.entities[row.as_usize()]) } } @@ -587,9 +646,9 @@ impl Table { row: TableRow, new_table: &mut Table, ) -> TableMoveResult { - debug_assert!(row.index() < self.entity_count()); - let is_last = row.index() == self.entities.len() - 1; - let new_row = new_table.allocate(self.entities.swap_remove(row.index())); + debug_assert!(row.as_usize() < self.entity_count()); + let is_last = row.as_usize() == self.entities.len() - 1; + let new_row = new_table.allocate(self.entities.swap_remove(row.as_usize())); for (component_id, column) in self.columns.iter_mut() { if let Some(new_column) = new_table.get_column_mut(*component_id) { new_column.initialize_from_unchecked(column, row, new_row); @@ -603,7 +662,7 @@ impl Table { swapped_entity: if is_last { None } else { - Some(self.entities[row.index()]) + Some(self.entities[row.as_usize()]) }, } } @@ -619,9 +678,9 @@ impl Table { row: TableRow, new_table: &mut Table, ) -> TableMoveResult { - debug_assert!(row.index() < self.entity_count()); - let is_last = row.index() == self.entities.len() - 1; - let new_row = new_table.allocate(self.entities.swap_remove(row.index())); + debug_assert!(row.as_usize() < self.entity_count()); + let is_last = row.as_usize() == self.entities.len() - 1; + let new_row = new_table.allocate(self.entities.swap_remove(row.as_usize())); for (component_id, column) in self.columns.iter_mut() { if let Some(new_column) = new_table.get_column_mut(*component_id) { new_column.initialize_from_unchecked(column, row, new_row); @@ -634,7 +693,7 @@ impl Table { swapped_entity: if is_last { None } else { - Some(self.entities[row.index()]) + Some(self.entities[row.as_usize()]) }, } } @@ -650,9 +709,9 @@ impl Table { row: TableRow, new_table: &mut Table, ) -> TableMoveResult { - debug_assert!(row.index() < self.entity_count()); - let is_last = row.index() == self.entities.len() - 1; - let new_row = new_table.allocate(self.entities.swap_remove(row.index())); + debug_assert!(row.as_usize() < self.entity_count()); + let is_last = row.as_usize() == self.entities.len() - 1; + let new_row = new_table.allocate(self.entities.swap_remove(row.as_usize())); for (component_id, column) in self.columns.iter_mut() { new_table .get_column_mut(*component_id) @@ -664,7 +723,7 @@ impl Table { swapped_entity: if is_last { None } else { - Some(self.entities[row.index()]) + Some(self.entities[row.as_usize()]) }, } } @@ -728,7 +787,7 @@ impl Table { column.added_ticks.push(UnsafeCell::new(Tick::new(0))); column.changed_ticks.push(UnsafeCell::new(Tick::new(0))); } - TableRow::new(index) + TableRow::from_usize(index) } /// Gets the number of entities currently being stored in the table. @@ -819,7 +878,7 @@ impl Tables { /// Returns `None` if `id` is invalid. #[inline] pub fn get(&self, id: TableId) -> Option<&Table> { - self.tables.get(id.index()) + self.tables.get(id.as_usize()) } /// Fetches mutable references to two different [`Table`]s. @@ -829,12 +888,12 @@ impl Tables { /// Panics if `a` and `b` are equal. #[inline] pub(crate) fn get_2_mut(&mut self, a: TableId, b: TableId) -> (&mut Table, &mut Table) { - if a.index() > b.index() { - let (b_slice, a_slice) = self.tables.split_at_mut(a.index()); - (&mut a_slice[0], &mut b_slice[b.index()]) + if a.as_usize() > b.as_usize() { + let (b_slice, a_slice) = self.tables.split_at_mut(a.as_usize()); + (&mut a_slice[0], &mut b_slice[b.as_usize()]) } else { - let (a_slice, b_slice) = self.tables.split_at_mut(b.index()); - (&mut a_slice[a.index()], &mut b_slice[0]) + let (a_slice, b_slice) = self.tables.split_at_mut(b.as_usize()); + (&mut a_slice[a.as_usize()], &mut b_slice[0]) } } @@ -859,7 +918,10 @@ impl Tables { table = table.add_column(components.get_info_unchecked(*component_id)); } tables.push(table.build()); - (component_ids.to_vec(), TableId::new(tables.len() - 1)) + ( + component_ids.to_vec(), + TableId::from_usize(tables.len() - 1), + ) }); *value @@ -889,14 +951,14 @@ impl Index for Tables { #[inline] fn index(&self, index: TableId) -> &Self::Output { - &self.tables[index.index()] + &self.tables[index.as_usize()] } } impl IndexMut for Tables { #[inline] fn index_mut(&mut self, index: TableId) -> &mut Self::Output { - &mut self.tables[index.index()] + &mut self.tables[index.as_usize()] } } From 3acff73778731b59a14e7b178b6cbca4b5559cec Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Thu, 30 Nov 2023 15:38:45 +1100 Subject: [PATCH 2/5] Updated `TableRow` `from_*` and `as_*` assertions Using forward-reverse casting to ensure value is invariant. --- crates/bevy_ecs/src/storage/table.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index d11d3fcc76768..4b9ebc293bee9 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -41,7 +41,7 @@ impl TableId { /// [`World`]: crate::world::World #[inline] pub const fn from_u32(index: u32) -> Self { - TableId(index) + Self(index) } /// Creates a new [`TableId`]. @@ -56,8 +56,8 @@ impl TableId { /// Will panic if the provided value does not fit within a [`u32`] with `debug_assertions`. #[inline] pub const fn from_usize(index: usize) -> Self { - debug_assert!((index as u128) < (u32::MAX as u128)); - TableId(index as u32) + debug_assert!(index as u32 as usize == index); + Self(index as u32) } /// Gets the underlying table index from the ID. @@ -73,13 +73,13 @@ impl TableId { /// Will panic if the inner value does not fit within a [`usize`] with `debug_assertions`. #[inline] pub const fn as_usize(self) -> usize { - debug_assert!((self.0 as u128) < (usize::MAX as u128)); + debug_assert!(self.0 as usize as u32 == self.0); self.0 as usize } /// The [`TableId`] of the [`Table`] without any components. #[inline] - pub const fn empty() -> TableId { + pub const fn empty() -> Self { Self(0) } } @@ -120,7 +120,7 @@ impl TableRow { /// Will panic if the provided value does not fit within a [`u32`] with `debug_assertions`. #[inline] pub const fn from_usize(index: usize) -> Self { - debug_assert!((index as u128) < (u32::MAX as u128)); + debug_assert!(index as u32 as usize == index); Self(index as u32) } @@ -131,7 +131,7 @@ impl TableRow { /// Will panic if the inner value does not fit within a [`usize`] with `debug_assertions`. #[inline] pub const fn as_usize(self) -> usize { - debug_assert!((self.0 as u128) < (usize::MAX as u128)); + debug_assert!(self.0 as usize as u32 == self.0); self.0 as usize } From 5a9b2c8a8aed3dd882b4467f6ff2ad752405297f Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Fri, 1 Dec 2023 08:20:49 +1100 Subject: [PATCH 3/5] Updated `TableRow` `from_*` assertions to `assert` Also removed `as_*` assertion since `usize` is guaranteed to be `u32` or larger in Bevy. --- crates/bevy_ecs/src/storage/table.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 4b9ebc293bee9..474e6b2e3489a 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -56,7 +56,7 @@ impl TableId { /// Will panic if the provided value does not fit within a [`u32`] with `debug_assertions`. #[inline] pub const fn from_usize(index: usize) -> Self { - debug_assert!(index as u32 as usize == index); + assert!(index as u32 as usize == index); Self(index as u32) } @@ -67,13 +67,9 @@ impl TableId { } /// Gets the underlying table index from the ID. - /// - /// # Panics - /// - /// Will panic if the inner value does not fit within a [`usize`] with `debug_assertions`. #[inline] pub const fn as_usize(self) -> usize { - debug_assert!(self.0 as usize as u32 == self.0); + // usize is at least u32 in Bevy self.0 as usize } @@ -120,18 +116,14 @@ impl TableRow { /// Will panic if the provided value does not fit within a [`u32`] with `debug_assertions`. #[inline] pub const fn from_usize(index: usize) -> Self { - debug_assert!(index as u32 as usize == index); + assert!(index as u32 as usize == index); Self(index as u32) } /// Gets the index of the row as a [`usize`]. - /// - /// # Panics - /// - /// Will panic if the inner value does not fit within a [`usize`] with `debug_assertions`. #[inline] pub const fn as_usize(self) -> usize { - debug_assert!(self.0 as usize as u32 == self.0); + // usize is at least u32 in Bevy self.0 as usize } From 147bb13be0ccd3c911715ff506fa76ba55b906b2 Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Fri, 1 Dec 2023 08:20:49 +1100 Subject: [PATCH 4/5] Updated `TableRow` `from_*` assertions to `assert` Also removed `as_*` assertion since `usize` is guaranteed to be `u32` or larger in Bevy. --- crates/bevy_ecs/src/storage/table.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 4b9ebc293bee9..f81e6d5f8f7bc 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -53,10 +53,10 @@ impl TableId { /// /// # Panics /// - /// Will panic if the provided value does not fit within a [`u32`] with `debug_assertions`. + /// Will panic if the provided value does not fit within a [`u32`]. #[inline] pub const fn from_usize(index: usize) -> Self { - debug_assert!(index as u32 as usize == index); + assert!(index as u32 as usize == index); Self(index as u32) } @@ -67,13 +67,9 @@ impl TableId { } /// Gets the underlying table index from the ID. - /// - /// # Panics - /// - /// Will panic if the inner value does not fit within a [`usize`] with `debug_assertions`. #[inline] pub const fn as_usize(self) -> usize { - debug_assert!(self.0 as usize as u32 == self.0); + // usize is at least u32 in Bevy self.0 as usize } @@ -117,21 +113,17 @@ impl TableRow { /// /// # Panics /// - /// Will panic if the provided value does not fit within a [`u32`] with `debug_assertions`. + /// Will panic if the provided value does not fit within a [`u32`]. #[inline] pub const fn from_usize(index: usize) -> Self { - debug_assert!(index as u32 as usize == index); + assert!(index as u32 as usize == index); Self(index as u32) } /// Gets the index of the row as a [`usize`]. - /// - /// # Panics - /// - /// Will panic if the inner value does not fit within a [`usize`] with `debug_assertions`. #[inline] pub const fn as_usize(self) -> usize { - debug_assert!(self.0 as usize as u32 == self.0); + // usize is at least u32 in Bevy self.0 as usize } From f8612ca59781630ab4c3bea71248b09733e3a64b Mon Sep 17 00:00:00 2001 From: Zachary Harrold Date: Sat, 2 Dec 2023 12:11:24 +1100 Subject: [PATCH 5/5] Resolve upstream conflicts Added `assert` in `QueryIter::fold_over_table_range` out of the loop to hint compiler on optimisation. --- crates/bevy_ecs/src/query/iter.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 99c710e64b3ab..1d5247423de11 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -106,6 +106,11 @@ impl<'w, 's, Q: WorldQueryData, F: WorldQueryFilter> QueryIter<'w, 's, Q, F> { where Func: FnMut(B, Q::Item<'w>) -> B, { + assert!( + rows.end <= u32::MAX as usize, + "TableRow is only valid up to u32::MAX" + ); + Q::set_table(&mut self.cursor.fetch, &self.query_state.fetch_state, table); F::set_table( &mut self.cursor.filter, @@ -117,7 +122,7 @@ impl<'w, 's, Q: WorldQueryData, F: WorldQueryFilter> QueryIter<'w, 's, Q, F> { for row in rows { // SAFETY: Caller assures `row` in range of the current archetype. let entity = entities.get_unchecked(row); - let row = TableRow::new(row); + let row = TableRow::from_usize(row); // SAFETY: set_table was called prior. // Caller assures `row` in range of the current archetype. if !F::filter_fetch(&mut self.cursor.filter, *entity, row) {