From 4a51aef7eb672cdc2284685a8a1d270595c4dcfb Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 11 May 2022 10:57:43 -0700 Subject: [PATCH 1/3] Use u32 over usize for ComponentSparseSet indicies --- crates/bevy_ecs/src/storage/sparse_set.rs | 46 ++++++++++++----------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index e21a4f4723c14..84d00ab0bde7d 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -6,6 +6,8 @@ use crate::{ use bevy_ptr::{OwningPtr, Ptr}; use std::{cell::UnsafeCell, hash::Hash, marker::PhantomData}; +type EntityId = u32; + #[derive(Debug)] pub struct SparseArray { values: Vec>, @@ -96,8 +98,8 @@ impl SparseArray { pub struct ComponentSparseSet { dense: BlobVec, ticks: Vec>, - entities: Vec, - sparse: SparseArray, + entities: Vec, + sparse: SparseArray, } impl ComponentSparseSet { @@ -137,30 +139,30 @@ impl ComponentSparseSet { /// The `value` pointer must point to a valid address that matches the [`Layout`](std::alloc::Layout) /// inside the [`ComponentInfo`] given when constructing this sparse set. pub unsafe fn insert(&mut self, entity: Entity, value: OwningPtr<'_>, change_tick: u32) { - if let Some(&dense_index) = self.sparse.get(entity) { - self.dense.replace_unchecked(dense_index, value); - *self.ticks.get_unchecked_mut(dense_index) = + if let Some(&dense_index) = self.sparse.get(entity.id()) { + self.dense.replace_unchecked(dense_index as usize, value); + *self.ticks.get_unchecked_mut(dense_index as usize) = UnsafeCell::new(ComponentTicks::new(change_tick)); } else { let dense_index = self.dense.len(); self.dense.push(value); - self.sparse.insert(entity, dense_index); + self.sparse.insert(entity.id(), dense_index as u32); debug_assert_eq!(self.ticks.len(), dense_index); debug_assert_eq!(self.entities.len(), dense_index); self.ticks .push(UnsafeCell::new(ComponentTicks::new(change_tick))); - self.entities.push(entity); + self.entities.push(entity.id()); } } #[inline] pub fn contains(&self, entity: Entity) -> bool { - self.sparse.contains(entity) + self.sparse.contains(entity.id()) } #[inline] pub fn get(&self, entity: Entity) -> Option> { - self.sparse.get(entity).map(|dense_index| { + self.sparse.get(entity.id()).map(|dense_index| { // SAFE: if the sparse index points to something in the dense vec, it exists unsafe { self.dense.get_unchecked(*dense_index) } }) @@ -168,33 +170,33 @@ impl ComponentSparseSet { #[inline] pub fn get_with_ticks(&self, entity: Entity) -> Option<(Ptr<'_>, &UnsafeCell)> { - let dense_index = *self.sparse.get(entity)?; + let dense_index = *self.sparse.get(entity.id())?; // SAFE: if the sparse index points to something in the dense vec, it exists unsafe { Some(( - self.dense.get_unchecked(dense_index), - self.ticks.get_unchecked(dense_index), + self.dense.get_unchecked(dense_index as usize), + self.ticks.get_unchecked(dense_index as usize), )) } } #[inline] pub fn get_ticks(&self, entity: Entity) -> Option<&UnsafeCell> { - let dense_index = *self.sparse.get(entity)?; + let dense_index = *self.sparse.get(entity.id())?; // SAFE: if the sparse index points to something in the dense vec, it exists - unsafe { Some(self.ticks.get_unchecked(dense_index)) } + unsafe { Some(self.ticks.get_unchecked(dense_index as usize)) } } /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if /// it exists). #[must_use = "The returned pointer must be used to drop the removed component."] pub fn remove_and_forget(&mut self, entity: Entity) -> Option> { - self.sparse.remove(entity).map(|dense_index| { - self.ticks.swap_remove(dense_index); - self.entities.swap_remove(dense_index); + self.sparse.remove(entity.id()).map(|dense_index| { + self.ticks.swap_remove(dense_index as usize); + self.entities.swap_remove(dense_index as usize); let is_last = dense_index == self.dense.len() - 1; // SAFE: dense_index was just removed from `sparse`, which ensures that it is valid - let value = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) }; + let value = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index as usize) }; if !is_last { let swapped_entity = self.entities[dense_index]; *self.sparse.get_mut(swapped_entity).unwrap() = dense_index; @@ -204,12 +206,12 @@ impl ComponentSparseSet { } pub fn remove(&mut self, entity: Entity) -> bool { - if let Some(dense_index) = self.sparse.remove(entity) { - self.ticks.swap_remove(dense_index); - self.entities.swap_remove(dense_index); + if let Some(dense_index) = self.sparse.remove(entity.id()) { + self.ticks.swap_remove(dense_index as usize); + self.entities.swap_remove(dense_index as usize); let is_last = dense_index == self.dense.len() - 1; // SAFE: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.swap_remove_and_drop_unchecked(dense_index) } + unsafe { self.dense.swap_remove_and_drop_unchecked(dense_index as usize) } if !is_last { let swapped_entity = self.entities[dense_index]; *self.sparse.get_mut(swapped_entity).unwrap() = dense_index; From ec00802d85769f6197d96401c6adca2fa7f6c309 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 11 May 2022 11:05:59 -0700 Subject: [PATCH 2/3] Get the ones that were missed --- crates/bevy_ecs/src/storage/sparse_set.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 84d00ab0bde7d..aa96d0ba66b21 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -164,7 +164,7 @@ impl ComponentSparseSet { pub fn get(&self, entity: Entity) -> Option> { self.sparse.get(entity.id()).map(|dense_index| { // SAFE: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.get_unchecked(*dense_index) } + unsafe { self.dense.get_unchecked(*dense_index as usize) } }) } @@ -192,14 +192,15 @@ impl ComponentSparseSet { #[must_use = "The returned pointer must be used to drop the removed component."] pub fn remove_and_forget(&mut self, entity: Entity) -> Option> { self.sparse.remove(entity.id()).map(|dense_index| { - self.ticks.swap_remove(dense_index as usize); - self.entities.swap_remove(dense_index as usize); + let dense_index = dense_index as usize; + self.ticks.swap_remove(dense_index); + self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; // SAFE: dense_index was just removed from `sparse`, which ensures that it is valid - let value = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index as usize) }; + let value = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) }; if !is_last { let swapped_entity = self.entities[dense_index]; - *self.sparse.get_mut(swapped_entity).unwrap() = dense_index; + *self.sparse.get_mut(swapped_entity).unwrap() = dense_index as u32; } value }) @@ -207,14 +208,15 @@ impl ComponentSparseSet { pub fn remove(&mut self, entity: Entity) -> bool { if let Some(dense_index) = self.sparse.remove(entity.id()) { - self.ticks.swap_remove(dense_index as usize); - self.entities.swap_remove(dense_index as usize); + let dense_index = dense_index as usize; + self.ticks.swap_remove(dense_index); + self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; // SAFE: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.swap_remove_and_drop_unchecked(dense_index as usize) } + unsafe { self.dense.swap_remove_and_drop_unchecked(dense_index) } if !is_last { let swapped_entity = self.entities[dense_index]; - *self.sparse.get_mut(swapped_entity).unwrap() = dense_index; + *self.sparse.get_mut(swapped_entity).unwrap() = dense_index as u32; } true } else { From d53d503c879206dd9433a2eb5de661ac54c3ffde Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 18 May 2022 12:24:18 -0700 Subject: [PATCH 3/3] Add #[cfg(debug_assertions)] conditional checks --- crates/bevy_ecs/src/storage/sparse_set.rs | 52 +++++++++++++++++++---- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index aa96d0ba66b21..7f441eb1d5971 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -98,7 +98,13 @@ impl SparseArray { pub struct ComponentSparseSet { dense: BlobVec, ticks: Vec>, + // Internally this only relies on the Entity ID to keep track of where the component data is + // stored for entities that are alive. The generation is not required, but is stored + // in debug builds to validate that access is correct. + #[cfg(not(debug_assertions))] entities: Vec, + #[cfg(debug_assertions)] + entities: Vec, sparse: SparseArray, } @@ -140,7 +146,8 @@ impl ComponentSparseSet { /// inside the [`ComponentInfo`] given when constructing this sparse set. pub unsafe fn insert(&mut self, entity: Entity, value: OwningPtr<'_>, change_tick: u32) { if let Some(&dense_index) = self.sparse.get(entity.id()) { - self.dense.replace_unchecked(dense_index as usize, value); + debug_assert_eq!(entity, self.entities[dense_index as usize]); + let _entity = self.dense.replace_unchecked(dense_index as usize, value); *self.ticks.get_unchecked_mut(dense_index as usize) = UnsafeCell::new(ComponentTicks::new(change_tick)); } else { @@ -151,40 +158,57 @@ impl ComponentSparseSet { debug_assert_eq!(self.entities.len(), dense_index); self.ticks .push(UnsafeCell::new(ComponentTicks::new(change_tick))); + #[cfg(not(debug_assertions))] self.entities.push(entity.id()); + #[cfg(debug_assertions)] + self.entities.push(entity); } } #[inline] pub fn contains(&self, entity: Entity) -> bool { + #[cfg(debug_assertions)] + { + if let Some(&dense_index) = self.sparse.get(entity.id()) { + debug_assert_eq!(entity, self.entities[dense_index as usize]); + true + } else { + false + } + } + #[cfg(not(debug_assertions))] self.sparse.contains(entity.id()) } #[inline] pub fn get(&self, entity: Entity) -> Option> { self.sparse.get(entity.id()).map(|dense_index| { + let dense_index = *dense_index as usize; + debug_assert_eq!(entity, self.entities[dense_index]); // SAFE: if the sparse index points to something in the dense vec, it exists - unsafe { self.dense.get_unchecked(*dense_index as usize) } + unsafe { self.dense.get_unchecked(dense_index) } }) } #[inline] pub fn get_with_ticks(&self, entity: Entity) -> Option<(Ptr<'_>, &UnsafeCell)> { - let dense_index = *self.sparse.get(entity.id())?; + let dense_index = *self.sparse.get(entity.id())? as usize; + debug_assert_eq!(entity, self.entities[dense_index]); // SAFE: if the sparse index points to something in the dense vec, it exists unsafe { Some(( - self.dense.get_unchecked(dense_index as usize), - self.ticks.get_unchecked(dense_index as usize), + self.dense.get_unchecked(dense_index), + self.ticks.get_unchecked(dense_index), )) } } #[inline] pub fn get_ticks(&self, entity: Entity) -> Option<&UnsafeCell> { - let dense_index = *self.sparse.get(entity.id())?; + let dense_index = *self.sparse.get(entity.id())? as usize; + debug_assert_eq!(entity, self.entities[dense_index]); // SAFE: if the sparse index points to something in the dense vec, it exists - unsafe { Some(self.ticks.get_unchecked(dense_index as usize)) } + unsafe { Some(self.ticks.get_unchecked(dense_index)) } } /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if @@ -193,6 +217,7 @@ impl ComponentSparseSet { pub fn remove_and_forget(&mut self, entity: Entity) -> Option> { self.sparse.remove(entity.id()).map(|dense_index| { let dense_index = dense_index as usize; + debug_assert_eq!(entity, self.entities[dense_index]); self.ticks.swap_remove(dense_index); self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; @@ -200,7 +225,11 @@ impl ComponentSparseSet { let value = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) }; if !is_last { let swapped_entity = self.entities[dense_index]; - *self.sparse.get_mut(swapped_entity).unwrap() = dense_index as u32; + #[cfg(not(debug_assertions))] + let idx = swapped_entity; + #[cfg(debug_assertions)] + let idx = swapped_entity.id(); + *self.sparse.get_mut(idx).unwrap() = dense_index as u32; } value }) @@ -209,6 +238,7 @@ impl ComponentSparseSet { pub fn remove(&mut self, entity: Entity) -> bool { if let Some(dense_index) = self.sparse.remove(entity.id()) { let dense_index = dense_index as usize; + debug_assert_eq!(entity, self.entities[dense_index]); self.ticks.swap_remove(dense_index); self.entities.swap_remove(dense_index); let is_last = dense_index == self.dense.len() - 1; @@ -216,7 +246,11 @@ impl ComponentSparseSet { unsafe { self.dense.swap_remove_and_drop_unchecked(dense_index) } if !is_last { let swapped_entity = self.entities[dense_index]; - *self.sparse.get_mut(swapped_entity).unwrap() = dense_index as u32; + #[cfg(not(debug_assertions))] + let idx = swapped_entity; + #[cfg(debug_assertions)] + let idx = swapped_entity.id(); + *self.sparse.get_mut(idx).unwrap() = dense_index as u32; } true } else {