From 1214ddabb709de00c2a360644f6b37cb2114d50e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Grabarz?= Date: Sun, 30 May 2021 20:15:40 +0000 Subject: [PATCH] drop overwritten component data on double insert (#2227) Continuing the work on reducing the safety footguns in the code, I've removed one extra `UnsafeCell` in favour of safe `Cell` usage inisde `ComponentTicks`. That change led to discovery of misbehaving component insert logic, where data wasn't properly dropped when overwritten. Apart from that being fixed, some method names were changed to better convey the "initialize new allocation" and "replace existing allocation" semantic. Depends on #2221, I will rebase this PR after the dependency is merged. For now, review just the last commit. Co-authored-by: Carter Anderson --- crates/bevy_ecs/src/bundle.rs | 14 +-- crates/bevy_ecs/src/component/mod.rs | 2 +- crates/bevy_ecs/src/lib.rs | 52 +++++++++- crates/bevy_ecs/src/query/fetch.rs | 32 +++--- crates/bevy_ecs/src/query/filter.rs | 16 +-- crates/bevy_ecs/src/storage/blob_vec.rs | 18 +++- crates/bevy_ecs/src/storage/sparse_set.rs | 66 ++++++------- crates/bevy_ecs/src/storage/table.rs | 99 ++++++++++++------- crates/bevy_ecs/src/system/system_param.rs | 17 ++-- crates/bevy_ecs/src/world/entity_ref.rs | 42 ++++---- crates/bevy_ecs/src/world/mod.rs | 14 +-- .../src/hierarchy/child_builder.rs | 8 +- 12 files changed, 229 insertions(+), 151 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 6ec6094ec2864..00f3bac9e527e 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -140,20 +140,20 @@ impl BundleInfo { // bundle_info.component_ids are also in "bundle order" let mut bundle_component = 0; bundle.get_components(|component_ptr| { - // SAFE: component_id was initialized by get_dynamic_bundle_info let component_id = *self.component_ids.get_unchecked(bundle_component); - let component_status = bundle_status.get_unchecked(bundle_component); match self.storage_types[bundle_component] { StorageType::Table => { let column = table.get_column_mut(component_id).unwrap(); - column.set_data_unchecked(table_row, component_ptr); - let column_status = column.get_ticks_unchecked_mut(table_row); - match component_status { + match bundle_status.get_unchecked(bundle_component) { ComponentStatus::Added => { - *column_status = ComponentTicks::new(change_tick); + column.initialize( + table_row, + component_ptr, + ComponentTicks::new(change_tick), + ); } ComponentStatus::Mutated => { - column_status.set_changed(change_tick); + column.replace(table_row, component_ptr, change_tick); } } } diff --git a/crates/bevy_ecs/src/component/mod.rs b/crates/bevy_ecs/src/component/mod.rs index 3ee166d9c0aee..4a392bbe3a898 100644 --- a/crates/bevy_ecs/src/component/mod.rs +++ b/crates/bevy_ecs/src/component/mod.rs @@ -306,7 +306,7 @@ impl Components { } } -#[derive(Copy, Clone, Debug)] +#[derive(Clone, Debug)] pub struct ComponentTicks { pub(crate) added: u32, pub(crate) changed: u32, diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index f5e01120c1ba7..a6fdbced0894d 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -50,13 +50,34 @@ mod tests { }; use bevy_tasks::TaskPool; use parking_lot::Mutex; - use std::{any::TypeId, sync::Arc}; + use std::{ + any::TypeId, + sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, + }, + }; #[derive(Debug, PartialEq, Eq)] struct A(usize); struct B(usize); struct C; + #[derive(Clone, Debug)] + struct DropCk(Arc); + impl DropCk { + fn new_pair() -> (Self, Arc) { + let atomic = Arc::new(AtomicUsize::new(0)); + (DropCk(atomic.clone()), atomic) + } + } + + impl Drop for DropCk { + fn drop(&mut self) { + self.0.as_ref().fetch_add(1, Ordering::Relaxed); + } + } + #[test] fn random_access() { let mut world = World::new(); @@ -1176,4 +1197,33 @@ mod tests { }); assert_eq!(*world.get_resource::().unwrap(), 1); } + + #[test] + fn insert_overwrite_drop() { + let (dropck1, dropped1) = DropCk::new_pair(); + let (dropck2, dropped2) = DropCk::new_pair(); + let mut world = World::default(); + world.spawn().insert(dropck1).insert(dropck2); + assert_eq!(dropped1.load(Ordering::Relaxed), 1); + assert_eq!(dropped2.load(Ordering::Relaxed), 0); + drop(world); + assert_eq!(dropped1.load(Ordering::Relaxed), 1); + assert_eq!(dropped2.load(Ordering::Relaxed), 1); + } + + #[test] + fn insert_overwrite_drop_sparse() { + let (dropck1, dropped1) = DropCk::new_pair(); + let (dropck2, dropped2) = DropCk::new_pair(); + let mut world = World::default(); + world + .register_component(ComponentDescriptor::new::(StorageType::SparseSet)) + .unwrap(); + world.spawn().insert(dropck1).insert(dropck2); + assert_eq!(dropped1.load(Ordering::Relaxed), 1); + assert_eq!(dropped2.load(Ordering::Relaxed), 0); + drop(world); + assert_eq!(dropped1.load(Ordering::Relaxed), 1); + assert_eq!(dropped2.load(Ordering::Relaxed), 1); + } } diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index b24a9e8b90009..2764f897310f2 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -9,6 +9,7 @@ use crate::{ }; use bevy_ecs_macros::all_tuples; use std::{ + cell::UnsafeCell, marker::PhantomData, ptr::{self, NonNull}, }; @@ -343,7 +344,7 @@ impl<'w, T: Component> Fetch<'w> for ReadFetch { let column = tables[archetype.table_id()] .get_column(state.component_id) .unwrap(); - self.table_components = column.get_ptr().cast::(); + self.table_components = column.get_data_ptr().cast::(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } @@ -354,7 +355,7 @@ impl<'w, T: Component> Fetch<'w> for ReadFetch { self.table_components = table .get_column(state.component_id) .unwrap() - .get_ptr() + .get_data_ptr() .cast::(); } @@ -387,7 +388,7 @@ impl WorldQuery for &mut T { pub struct WriteFetch { storage_type: StorageType, table_components: NonNull, - table_ticks: *mut ComponentTicks, + table_ticks: *const UnsafeCell, entities: *const Entity, entity_table_rows: *const usize, sparse_set: *const ComponentSparseSet, @@ -482,7 +483,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { entities: ptr::null::(), entity_table_rows: ptr::null::(), sparse_set: ptr::null::(), - table_ticks: ptr::null_mut::(), + table_ticks: ptr::null::>(), last_change_tick, change_tick, }; @@ -509,8 +510,8 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { let column = tables[archetype.table_id()] .get_column(state.component_id) .unwrap(); - self.table_components = column.get_ptr().cast::(); - self.table_ticks = column.get_ticks_mut_ptr(); + self.table_components = column.get_data_ptr().cast::(); + self.table_ticks = column.get_ticks_ptr(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } @@ -519,8 +520,8 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { #[inline] unsafe fn set_table(&mut self, state: &Self::State, table: &Table) { let column = table.get_column(state.component_id).unwrap(); - self.table_components = column.get_ptr().cast::(); - self.table_ticks = column.get_ticks_mut_ptr(); + self.table_components = column.get_data_ptr().cast::(); + self.table_ticks = column.get_ticks_ptr(); } #[inline] @@ -531,7 +532,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { Mut { value: &mut *self.table_components.as_ptr().add(table_row), ticks: Ticks { - component_ticks: &mut *self.table_ticks.add(table_row), + component_ticks: &mut *(&*self.table_ticks.add(table_row)).get(), change_tick: self.change_tick, last_change_tick: self.last_change_tick, }, @@ -558,7 +559,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { Mut { value: &mut *self.table_components.as_ptr().add(table_row), ticks: Ticks { - component_ticks: &mut *self.table_ticks.add(table_row), + component_ticks: &mut *(&*self.table_ticks.add(table_row)).get(), change_tick: self.change_tick, last_change_tick: self.last_change_tick, }, @@ -860,7 +861,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { let column = tables[archetype.table_id()] .get_column(state.component_id) .unwrap(); - self.table_ticks = column.get_ticks_mut_ptr().cast::(); + self.table_ticks = column.get_ticks_const_ptr(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } @@ -871,8 +872,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { self.table_ticks = table .get_column(state.component_id) .unwrap() - .get_ticks_mut_ptr() - .cast::(); + .get_ticks_const_ptr(); } #[inline] @@ -881,7 +881,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { StorageType::Table => { let table_row = *self.entity_table_rows.add(archetype_index); ChangeTrackers { - component_ticks: *self.table_ticks.add(table_row), + component_ticks: (&*self.table_ticks.add(table_row)).clone(), marker: PhantomData, last_change_tick: self.last_change_tick, change_tick: self.change_tick, @@ -890,7 +890,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { StorageType::SparseSet => { let entity = *self.entities.add(archetype_index); ChangeTrackers { - component_ticks: *(*self.sparse_set).get_ticks(entity).unwrap(), + component_ticks: (&*self.sparse_set).get_ticks(entity).cloned().unwrap(), marker: PhantomData, last_change_tick: self.last_change_tick, change_tick: self.change_tick, @@ -902,7 +902,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch { #[inline] unsafe fn table_fetch(&mut self, table_row: usize) -> Self::Item { ChangeTrackers { - component_ticks: *self.table_ticks.add(table_row), + component_ticks: (&*self.table_ticks.add(table_row)).clone(), marker: PhantomData, last_change_tick: self.last_change_tick, change_tick: self.change_tick, diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 2d51028311674..77d23c2cb3986 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -8,7 +8,7 @@ use crate::{ world::World, }; use bevy_ecs_macros::all_tuples; -use std::{marker::PhantomData, ptr}; +use std::{cell::UnsafeCell, marker::PhantomData, ptr}; // TODO: uncomment this and use as shorthand (remove where F::Fetch: FilterFetch everywhere) when // this bug is fixed in Rust 1.51: https://github.com/rust-lang/rust/pull/81671 @@ -561,7 +561,7 @@ macro_rules! impl_tick_filter { $(#[$fetch_meta])* pub struct $fetch_name { storage_type: StorageType, - table_ticks: *mut ComponentTicks, + table_ticks: *const UnsafeCell, entity_table_rows: *const usize, marker: PhantomData, entities: *const Entity, @@ -630,7 +630,7 @@ macro_rules! impl_tick_filter { unsafe fn init(world: &World, state: &Self::State, last_change_tick: u32, change_tick: u32) -> Self { let mut value = Self { storage_type: state.storage_type, - table_ticks: ptr::null_mut::(), + table_ticks: ptr::null::>(), entities: ptr::null::(), entity_table_rows: ptr::null::(), sparse_set: ptr::null::(), @@ -655,7 +655,7 @@ macro_rules! impl_tick_filter { unsafe fn set_table(&mut self, state: &Self::State, table: &Table) { self.table_ticks = table .get_column(state.component_id).unwrap() - .get_ticks_mut_ptr(); + .get_ticks_ptr(); } unsafe fn set_archetype(&mut self, state: &Self::State, archetype: &Archetype, tables: &Tables) { @@ -665,25 +665,25 @@ macro_rules! impl_tick_filter { let table = &tables[archetype.table_id()]; self.table_ticks = table .get_column(state.component_id).unwrap() - .get_ticks_mut_ptr(); + .get_ticks_ptr(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } } unsafe fn table_fetch(&mut self, table_row: usize) -> bool { - $is_detected(&*self.table_ticks.add(table_row), self.last_change_tick, self.change_tick) + $is_detected(&*(&*self.table_ticks.add(table_row)).get(), self.last_change_tick, self.change_tick) } unsafe fn archetype_fetch(&mut self, archetype_index: usize) -> bool { match self.storage_type { StorageType::Table => { let table_row = *self.entity_table_rows.add(archetype_index); - $is_detected(&*self.table_ticks.add(table_row), self.last_change_tick, self.change_tick) + $is_detected(&*(&*self.table_ticks.add(table_row)).get(), self.last_change_tick, self.change_tick) } StorageType::SparseSet => { let entity = *self.entities.add(archetype_index); - let ticks = (*(*self.sparse_set).get_ticks(entity).unwrap()); + let ticks = (&*self.sparse_set).get_ticks(entity).cloned().unwrap(); $is_detected(&ticks, self.last_change_tick, self.change_tick) } } diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 0c0fd4ac2218f..7469355cb78c6 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -86,15 +86,25 @@ impl BlobVec { } /// # Safety - /// `index` must be in bounds - /// Allows aliased mutable access to `index`'s data. Caller must ensure this does not happen + /// - index must be in bounds + /// - memory must be reserved and uninitialized #[inline] - pub unsafe fn set_unchecked(&self, index: usize, value: *mut u8) { + pub unsafe fn initialize_unchecked(&mut self, index: usize, value: *mut u8) { debug_assert!(index < self.len()); let ptr = self.get_unchecked(index); std::ptr::copy_nonoverlapping(value, ptr, self.item_layout.size()); } + /// # Safety + /// - index must be in-bounds + // - memory must be previously initialized + pub unsafe fn replace_unchecked(&mut self, index: usize, value: *mut u8) { + debug_assert!(index < self.len()); + let ptr = self.get_unchecked(index); + (self.drop)(ptr); + std::ptr::copy_nonoverlapping(value, ptr, self.item_layout.size()); + } + /// increases the length by one (and grows the vec if needed) with uninitialized memory and /// returns the index /// @@ -267,7 +277,7 @@ mod tests { /// `blob_vec` must have a layout that matches Layout::new::() unsafe fn push(blob_vec: &mut BlobVec, mut value: T) { let index = blob_vec.push_uninit(); - blob_vec.set_unchecked(index, (&mut value as *mut T).cast::()); + blob_vec.initialize_unchecked(index, (&mut value as *mut T).cast::()); std::mem::forget(value); } diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 8d6d9edc1825b..6d0b85ffbbe90 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -87,7 +87,7 @@ impl SparseArray { #[derive(Debug)] pub struct ComponentSparseSet { dense: BlobVec, - ticks: UnsafeCell>, + ticks: Vec>, entities: Vec, sparse: SparseArray, } @@ -96,7 +96,7 @@ impl ComponentSparseSet { pub fn new(component_info: &ComponentInfo, capacity: usize) -> Self { Self { dense: BlobVec::new(component_info.layout(), component_info.drop(), capacity), - ticks: UnsafeCell::new(Vec::with_capacity(capacity)), + ticks: Vec::with_capacity(capacity), entities: Vec::with_capacity(capacity), sparse: Default::default(), } @@ -120,17 +120,20 @@ impl ComponentSparseSet { /// The `value` pointer must point to a valid address that matches the `Layout` /// inside the `ComponentInfo` given when constructing this sparse set. pub unsafe fn insert(&mut self, entity: Entity, value: *mut u8, change_tick: u32) { - let dense = &mut self.dense; - let entities = &mut self.entities; - let ticks_list = self.ticks.get_mut(); - let dense_index = *self.sparse.get_or_insert_with(entity, move || { - ticks_list.push(ComponentTicks::new(change_tick)); - entities.push(entity); - dense.push_uninit() - }); - // SAFE: dense_index exists thanks to the call above - self.dense.set_unchecked(dense_index, value); - ((*self.ticks.get()).get_unchecked_mut(dense_index)).set_changed(change_tick); + if let Some(&dense_index) = self.sparse.get(entity) { + self.dense.replace_unchecked(dense_index, value); + *self.ticks.get_unchecked_mut(dense_index) = + UnsafeCell::new(ComponentTicks::new(change_tick)); + } else { + let dense_index = self.dense.push_uninit(); + self.dense.initialize_unchecked(dense_index, value); + self.sparse.insert(entity, dense_index); + 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); + } } #[inline] @@ -152,27 +155,19 @@ impl ComponentSparseSet { /// ensure the same entity is not accessed twice at the same time #[inline] pub unsafe fn get_with_ticks(&self, entity: Entity) -> Option<(*mut u8, *mut ComponentTicks)> { - let ticks = &mut *self.ticks.get(); - self.sparse.get(entity).map(move |dense_index| { - let dense_index = *dense_index; - // SAFE: if the sparse index points to something in the dense vec, it exists - ( - self.dense.get_unchecked(dense_index), - ticks.get_unchecked_mut(dense_index) as *mut ComponentTicks, - ) - }) + let dense_index = *self.sparse.get(entity)?; + // SAFE: if the sparse index points to something in the dense vec, it exists + Some(( + self.dense.get_unchecked(dense_index), + self.ticks.get_unchecked(dense_index).get(), + )) } - /// # Safety - /// ensure the same entity is not accessed twice at the same time #[inline] - pub unsafe fn get_ticks(&self, entity: Entity) -> Option<&mut ComponentTicks> { - let ticks = &mut *self.ticks.get(); - self.sparse.get(entity).map(move |dense_index| { - let dense_index = *dense_index; - // SAFE: if the sparse index points to something in the dense vec, it exists - ticks.get_unchecked_mut(dense_index) - }) + pub fn get_ticks(&self, entity: Entity) -> Option<&ComponentTicks> { + let dense_index = *self.sparse.get(entity)?; + // SAFE: if the sparse index points to something in the dense vec, it exists + unsafe { Some(&*self.ticks.get_unchecked(dense_index).get()) } } /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if @@ -180,7 +175,7 @@ impl ComponentSparseSet { /// returned). pub fn remove_and_forget(&mut self, entity: Entity) -> Option<*mut u8> { self.sparse.remove(entity).map(|dense_index| { - self.ticks.get_mut().swap_remove(dense_index); + 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 @@ -195,7 +190,7 @@ impl ComponentSparseSet { pub fn remove(&mut self, entity: Entity) -> bool { if let Some(dense_index) = self.sparse.remove(entity) { - self.ticks.get_mut().swap_remove(dense_index); + 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 @@ -211,9 +206,8 @@ impl ComponentSparseSet { } pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { - let ticks = self.ticks.get_mut().iter_mut(); - for component_ticks in ticks { - component_ticks.check_ticks(change_tick); + for component_ticks in &mut self.ticks { + component_ticks.get_mut().check_ticks(change_tick); } } } diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 9d95d234cb394..8fc334b56975a 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -34,7 +34,7 @@ impl TableId { pub struct Column { pub(crate) component_id: ComponentId, pub(crate) data: BlobVec, - pub(crate) ticks: UnsafeCell>, + pub(crate) ticks: Vec>, } impl Column { @@ -43,29 +43,44 @@ impl Column { Column { component_id: component_info.id(), data: BlobVec::new(component_info.layout(), component_info.drop(), capacity), - ticks: UnsafeCell::new(Vec::with_capacity(capacity)), + ticks: Vec::with_capacity(capacity), } } + /// Writes component data to the column at given row. + /// Assumes the slot is uninitialized, drop is not called. + /// To overwrite existing initialized value, use `replace` instead. + /// + /// # Safety + /// Assumes data has already been allocated for the given row. #[inline] - fn ticks_mut(&mut self) -> &mut Vec { - self.ticks.get_mut() + pub unsafe fn initialize(&mut self, row: usize, data: *mut u8, ticks: ComponentTicks) { + debug_assert!(row < self.len()); + self.data.initialize_unchecked(row, data); + *self.ticks.get_unchecked_mut(row).get_mut() = ticks; } + /// Writes component data to the column at given row. + /// Assumes the slot is initialized, calls drop. + /// /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub unsafe fn set_unchecked(&mut self, row: usize, data: *mut u8, ticks: ComponentTicks) { - self.set_data_unchecked(row, data); - *self.ticks_mut().get_unchecked_mut(row) = ticks; + pub unsafe fn replace(&mut self, row: usize, data: *mut u8, change_tick: u32) { + debug_assert!(row < self.len()); + self.data.replace_unchecked(row, data); + self.ticks + .get_unchecked_mut(row) + .get_mut() + .set_changed(change_tick); } /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub unsafe fn set_data_unchecked(&mut self, row: usize, data: *mut u8) { + pub unsafe fn initialize_data(&mut self, row: usize, data: *mut u8) { debug_assert!(row < self.len()); - self.data.set_unchecked(row, data); + self.data.initialize_unchecked(row, data); } #[inline] @@ -79,11 +94,11 @@ impl Column { } /// # Safety - /// Assumes data has already been allocated for the given row. + /// index must be in-bounds #[inline] pub unsafe fn get_ticks_unchecked_mut(&mut self, row: usize) -> &mut ComponentTicks { debug_assert!(row < self.len()); - self.ticks_mut().get_unchecked_mut(row) + self.ticks.get_unchecked_mut(row).get_mut() } /// # Safety @@ -91,7 +106,7 @@ impl Column { #[inline] pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: usize) { self.data.swap_remove_and_drop_unchecked(row); - self.ticks_mut().swap_remove(row); + self.ticks.swap_remove(row); } #[inline] @@ -100,7 +115,7 @@ impl Column { row: usize, ) -> (*mut u8, ComponentTicks) { let data = self.data.swap_remove_and_forget_unchecked(row); - let ticks = self.ticks_mut().swap_remove(row); + let ticks = self.ticks.swap_remove(row).into_inner(); (data, ticks) } @@ -108,50 +123,66 @@ impl Column { // - ptr must point to valid data of this column's component type pub(crate) unsafe fn push(&mut self, ptr: *mut u8, ticks: ComponentTicks) { let row = self.data.push_uninit(); - self.data.set_unchecked(row, ptr); - self.ticks_mut().push(ticks); + self.data.initialize_unchecked(row, ptr); + self.ticks.push(UnsafeCell::new(ticks)); } #[inline] pub(crate) fn reserve_exact(&mut self, additional: usize) { self.data.reserve_exact(additional); - self.ticks_mut().reserve_exact(additional); + self.ticks.reserve_exact(additional); } /// # Safety /// must ensure rust mutability rules are not violated #[inline] - pub unsafe fn get_ptr(&self) -> NonNull { + pub unsafe fn get_data_ptr(&self) -> NonNull { self.data.get_ptr() } - /// # Safety - /// must ensure rust mutability rules are not violated #[inline] - pub unsafe fn get_ticks_mut_ptr(&self) -> *mut ComponentTicks { - (*self.ticks.get()).as_mut_ptr() + pub fn get_ticks_ptr(&self) -> *const UnsafeCell { + self.ticks.as_ptr() + } + + #[inline] + pub fn get_ticks_const_ptr(&self) -> *const ComponentTicks { + // cast is valid, because UnsafeCell is repr(transparent) + self.get_ticks_ptr() as *const ComponentTicks } /// # Safety - /// must ensure rust mutability rules are not violated + /// - index must be in-bounds + /// - no other reference to the data of the same row can exist at the same time + /// - pointer cannot be dereferenced after mutable reference to this `Column` was live #[inline] - pub unsafe fn get_unchecked(&self, row: usize) -> *mut u8 { + pub unsafe fn get_data_unchecked(&self, row: usize) -> *mut u8 { debug_assert!(row < self.data.len()); self.data.get_unchecked(row) } /// # Safety - /// must ensure rust mutability rules are not violated + /// index must be in-bounds + #[inline] + pub unsafe fn get_ticks_unchecked(&self, row: usize) -> &ComponentTicks { + debug_assert!(row < self.ticks.len()); + &*self.ticks.get_unchecked(row).get() + } + + /// # Safety + /// - index must be in-bounds + /// - no other reference to the ticks of the same row can exist at the same time + /// - pointer cannot be dereferenced after mutable reference to this column was live #[inline] - pub unsafe fn get_ticks_unchecked(&self, row: usize) -> *mut ComponentTicks { - debug_assert!(row < (*self.ticks.get()).len()); - self.get_ticks_mut_ptr().add(row) + pub unsafe fn get_ticks_mut_ptr_unchecked(&self, row: usize) -> *mut ComponentTicks { + debug_assert!(row < self.ticks.len()); + self.ticks.get_unchecked(row).get() } #[inline] pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { - for component_ticks in self.ticks_mut() { - component_ticks.check_ticks(change_tick); + for component_ticks in &mut self.ticks { + component_ticks.get_mut().check_ticks(change_tick); } } } @@ -224,7 +255,7 @@ impl Table { for column in self.columns.values_mut() { let (data, ticks) = column.swap_remove_and_forget_unchecked(row); if let Some(new_column) = new_table.get_column_mut(column.component_id) { - new_column.set_unchecked(new_row, data, ticks); + new_column.initialize(new_row, data, ticks); } } TableMoveResult { @@ -254,7 +285,7 @@ impl Table { for column in self.columns.values_mut() { if let Some(new_column) = new_table.get_column_mut(column.component_id) { let (data, ticks) = column.swap_remove_and_forget_unchecked(row); - new_column.set_unchecked(new_row, data, ticks); + new_column.initialize(new_row, data, ticks); } else { column.swap_remove_unchecked(row); } @@ -286,7 +317,7 @@ impl Table { for column in self.columns.values_mut() { let new_column = new_table.get_column_mut(column.component_id).unwrap(); let (data, ticks) = column.swap_remove_and_forget_unchecked(row); - new_column.set_unchecked(new_row, data, ticks); + new_column.initialize(new_row, data, ticks); } TableMoveResult { new_row, @@ -336,7 +367,7 @@ impl Table { self.entities.push(entity); for column in self.columns.values_mut() { column.data.set_len(self.entities.len()); - (*column.ticks.get()).push(ComponentTicks::new(0)); + column.ticks.push(UnsafeCell::new(ComponentTicks::new(0))); } index } @@ -493,7 +524,7 @@ mod tests { table .get_column_mut(component_id) .unwrap() - .set_data_unchecked(row, value_ptr); + .initialize_data(row, value_ptr); }; } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index eb7bbcc1a64c8..07a8499971396 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -289,8 +289,8 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResState { ) }); Res { - value: &*column.get_ptr().as_ptr().cast::(), - ticks: &*column.get_ticks_mut_ptr(), + value: &*column.get_data_ptr().cast::().as_ptr(), + ticks: column.get_ticks_unchecked(0), last_change_tick: system_state.last_change_tick, change_tick, } @@ -327,8 +327,8 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResState { world .get_populated_resource_column(state.0.component_id) .map(|column| Res { - value: &*column.get_ptr().as_ptr().cast::(), - ticks: &*column.get_ticks_mut_ptr(), + value: &*column.get_data_ptr().cast::().as_ptr(), + ticks: column.get_ticks_unchecked(0), last_change_tick: system_state.last_change_tick, change_tick, }) @@ -756,9 +756,10 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendState { std::any::type_name::() ) }); + NonSend { - value: &*column.get_ptr().as_ptr().cast::(), - ticks: *column.get_ticks_mut_ptr(), + value: &*column.get_data_ptr().cast::().as_ptr(), + ticks: column.get_ticks_unchecked(0).clone(), last_change_tick: system_state.last_change_tick, change_tick, } @@ -888,8 +889,8 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendMutState { ) }); NonSendMut { - value: &mut *column.get_ptr().as_ptr().cast::(), - ticks: &mut *column.get_ticks_mut_ptr(), + value: &mut *column.get_data_ptr().cast::().as_ptr(), + ticks: &mut *column.get_ticks_mut_ptr_unchecked(0), last_change_tick: system_state.last_change_tick, change_tick, } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index cb4a10b66b118..1815d15798f43 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -191,16 +191,6 @@ impl<'w> EntityMut<'w> { // TODO: move relevant methods to World (add/remove bundle) pub fn insert_bundle(&mut self, bundle: T) -> &mut Self { - let entity = self.entity; - let change_tick = self.world.change_tick(); - let entities = &mut self.world.entities; - let archetypes = &mut self.world.archetypes; - let components = &mut self.world.components; - let storages = &mut self.world.storages; - - let bundle_info = self.world.bundles.init_info::(components); - let current_location = self.location; - // Use a non-generic function to cut down on monomorphization unsafe fn get_insert_bundle_info<'a>( entities: &mut Entities, @@ -269,26 +259,32 @@ impl<'w> EntityMut<'w> { } } + let change_tick = self.world.change_tick(); + let bundle_info = self + .world + .bundles + .init_info::(&mut self.world.components); + let (archetype, bundle_status, new_location) = unsafe { get_insert_bundle_info( - entities, - archetypes, - components, - storages, + &mut self.world.entities, + &mut self.world.archetypes, + &mut self.world.components, + &mut self.world.storages, bundle_info, - current_location, - entity, + self.location, + self.entity, ) }; self.location = new_location; - let table = &mut storages.tables[archetype.table_id()]; + let table = &mut self.world.storages.tables[archetype.table_id()]; let table_row = archetype.entity_table_row(new_location.index); // SAFE: table row is valid unsafe { bundle_info.write_components( - &mut storages.sparse_sets, - entity, + &mut self.world.storages.sparse_sets, + self.entity, table, table_row, bundle_status, @@ -554,7 +550,7 @@ unsafe fn get_component( let components = table.get_column(component_id)?; let table_row = archetype.entity_table_row(location.index); // SAFE: archetypes only store valid table_rows and the stored component type is T - Some(components.get_unchecked(table_row)) + Some(components.get_data_unchecked(table_row)) } StorageType::SparseSet => world .storages @@ -582,8 +578,8 @@ unsafe fn get_component_and_ticks( let table_row = archetype.entity_table_row(location.index); // SAFE: archetypes only store valid table_rows and the stored component type is T Some(( - components.get_unchecked(table_row), - components.get_ticks_unchecked(table_row), + components.get_data_unchecked(table_row), + components.get_ticks_mut_ptr_unchecked(table_row), )) } StorageType::SparseSet => world @@ -624,7 +620,7 @@ unsafe fn take_component( let components = table.get_column(component_id).unwrap(); let table_row = archetype.entity_table_row(location.index); // SAFE: archetypes only store valid table_rows and the stored component type is T - components.get_unchecked(table_row) + components.get_data_unchecked(table_row) } StorageType::SparseSet => storages .sparse_sets diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index f549ff791afa6..5068fdc4e5074 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -594,14 +594,16 @@ impl World { pub fn is_resource_added(&self) -> bool { let component_id = self.components.get_resource_id(TypeId::of::()).unwrap(); let column = self.get_populated_resource_column(component_id).unwrap(); - let ticks = unsafe { &*column.get_ticks_mut_ptr() }; + // SAFE: resources table always have row 0 + let ticks = unsafe { column.get_ticks_unchecked(0) }; ticks.is_added(self.last_change_tick(), self.read_change_tick()) } pub fn is_resource_changed(&self) -> bool { let component_id = self.components.get_resource_id(TypeId::of::()).unwrap(); let column = self.get_populated_resource_column(component_id).unwrap(); - let ticks = unsafe { &*column.get_ticks_mut_ptr() }; + // SAFE: resources table always have row 0 + let ticks = unsafe { column.get_ticks_unchecked(0) }; ticks.is_changed(self.last_change_tick(), self.read_change_tick()) } @@ -736,7 +738,7 @@ impl World { component_id: ComponentId, ) -> Option<&T> { let column = self.get_populated_resource_column(component_id)?; - Some(&*column.get_ptr().as_ptr().cast::()) + Some(&*column.get_data_ptr().as_ptr().cast::()) } /// # Safety @@ -749,9 +751,9 @@ impl World { ) -> Option> { let column = self.get_populated_resource_column(component_id)?; Some(Mut { - value: &mut *column.get_ptr().as_ptr().cast::(), + value: &mut *column.get_data_ptr().cast::().as_ptr(), ticks: Ticks { - component_ticks: &mut *column.get_ticks_mut_ptr(), + component_ticks: &mut *column.get_ticks_mut_ptr_unchecked(0), last_change_tick: self.last_change_tick(), change_tick: self.read_change_tick(), }, @@ -794,7 +796,7 @@ impl World { column.push(data, ComponentTicks::new(change_tick)); } else { // SAFE: column is of type T and has already been allocated - *column.get_unchecked(0).cast::() = value; + *column.get_data_unchecked(0).cast::() = value; column.get_ticks_unchecked_mut(0).set_changed(change_tick); } } diff --git a/crates/bevy_transform/src/hierarchy/child_builder.rs b/crates/bevy_transform/src/hierarchy/child_builder.rs index 0a97c4a850382..1c1ab208eefa4 100644 --- a/crates/bevy_transform/src/hierarchy/child_builder.rs +++ b/crates/bevy_transform/src/hierarchy/child_builder.rs @@ -23,15 +23,9 @@ impl Command for InsertChildren { .insert_bundle((Parent(self.parent), PreviousParent(self.parent))); } { - let mut added = false; if let Some(mut children) = world.get_mut::(self.parent) { children.0.insert_from_slice(self.index, &self.children); - added = true; - } - - // NOTE: ideally this is just an else statement, but currently that _incorrectly_ fails - // borrow-checking - if !added { + } else { world .entity_mut(self.parent) .insert(Children(self.children));