From 79fb4683bb915442284d392326f4ec7c97d8f6e2 Mon Sep 17 00:00:00 2001 From: Frizi Date: Fri, 21 May 2021 01:45:04 +0200 Subject: [PATCH] replace UnsafeCell with Cell in ComponentTicks and fix missing drop on component overwrite --- crates/bevy_ecs/src/bundle.rs | 14 ++-- crates/bevy_ecs/src/component/mod.rs | 15 ++-- crates/bevy_ecs/src/lib.rs | 52 +++++++++++++- crates/bevy_ecs/src/query/fetch.rs | 31 ++++---- crates/bevy_ecs/src/query/filter.rs | 10 +-- crates/bevy_ecs/src/reflect.rs | 2 +- crates/bevy_ecs/src/schedule/stage.rs | 2 +- crates/bevy_ecs/src/storage/blob_vec.rs | 14 +++- crates/bevy_ecs/src/storage/sparse_set.rs | 66 ++++++++--------- crates/bevy_ecs/src/storage/table.rs | 72 ++++++++++--------- crates/bevy_ecs/src/system/system_param.rs | 21 +++--- crates/bevy_ecs/src/world/entity_ref.rs | 62 ++++++++-------- crates/bevy_ecs/src/world/mod.rs | 12 ++-- crates/bevy_ecs/src/world/pointer.rs | 2 +- .../src/hierarchy/child_builder.rs | 8 +-- 15 files changed, 221 insertions(+), 162 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 6ec6094ec28649..00f3bac9e527e4 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 3ee166d9c0aeeb..9ecd35f79ac3ae 100644 --- a/crates/bevy_ecs/src/component/mod.rs +++ b/crates/bevy_ecs/src/component/mod.rs @@ -6,6 +6,7 @@ use crate::storage::SparseSetIndex; use std::{ alloc::Layout, any::{Any, TypeId}, + cell::Cell, collections::hash_map::Entry, }; use thiserror::Error; @@ -306,10 +307,10 @@ impl Components { } } -#[derive(Copy, Clone, Debug)] +#[derive(Clone, Debug)] pub struct ComponentTicks { pub(crate) added: u32, - pub(crate) changed: u32, + pub(crate) changed: Cell, } impl ComponentTicks { @@ -326,7 +327,7 @@ impl ComponentTicks { #[inline] pub fn is_changed(&self, last_change_tick: u32, change_tick: u32) -> bool { - let component_delta = change_tick.wrapping_sub(self.changed); + let component_delta = change_tick.wrapping_sub(self.changed.get()); let system_delta = change_tick.wrapping_sub(last_change_tick); component_delta < system_delta @@ -335,13 +336,13 @@ impl ComponentTicks { pub(crate) fn new(change_tick: u32) -> Self { Self { added: change_tick, - changed: change_tick, + changed: Cell::new(change_tick), } } pub(crate) fn check_ticks(&mut self, change_tick: u32) { check_tick(&mut self.added, change_tick); - check_tick(&mut self.changed, change_tick); + check_tick(self.changed.get_mut(), change_tick); } /// Manually sets the change tick. @@ -357,8 +358,8 @@ impl ComponentTicks { /// component_ticks.set_changed(world.read_change_tick()); /// ``` #[inline] - pub fn set_changed(&mut self, change_tick: u32) { - self.changed = change_tick; + pub fn set_changed(&self, change_tick: u32) { + self.changed.set(change_tick); } } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index f45c6790a57805..55854cff36c5c4 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -47,13 +47,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(); @@ -1174,4 +1195,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 af858cb634cc28..bbcacba4e33992 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -342,7 +342,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(), } @@ -353,7 +353,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::(); } @@ -386,7 +386,7 @@ impl WorldQuery for &mut T { pub struct WriteFetch { storage_type: StorageType, table_components: NonNull, - table_ticks: *mut ComponentTicks, + table_ticks: *const ComponentTicks, entities: *const Entity, entity_table_rows: *const usize, sparse_set: *const ComponentSparseSet, @@ -508,8 +508,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(), } @@ -518,8 +518,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] @@ -529,7 +529,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { let table_row = *self.entity_table_rows.add(archetype_index); Mut { value: &mut *self.table_components.as_ptr().add(table_row), - component_ticks: &mut *self.table_ticks.add(table_row), + component_ticks: &*self.table_ticks.add(table_row), change_tick: self.change_tick, last_change_tick: self.last_change_tick, } @@ -540,7 +540,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { (*self.sparse_set).get_with_ticks(entity).unwrap(); Mut { value: &mut *component.cast::(), - component_ticks: &mut *component_ticks, + component_ticks: &*component_ticks, change_tick: self.change_tick, last_change_tick: self.last_change_tick, } @@ -552,7 +552,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { unsafe fn table_fetch(&mut self, table_row: usize) -> Self::Item { Mut { value: &mut *self.table_components.as_ptr().add(table_row), - component_ticks: &mut *self.table_ticks.add(table_row), + component_ticks: &*self.table_ticks.add(table_row), change_tick: self.change_tick, last_change_tick: self.last_change_tick, } @@ -853,7 +853,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_ptr(); } StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), } @@ -864,8 +864,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_ptr(); } #[inline] @@ -874,7 +873,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, @@ -883,7 +882,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, @@ -895,7 +894,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 2d51028311674e..3392fc7ff58f94 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -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 ComponentTicks, 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,7 +665,7 @@ 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(), } @@ -683,7 +683,7 @@ macro_rules! impl_tick_filter { } 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/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 3e79161578a395..d185485e5b85ca 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -116,7 +116,7 @@ impl FromType for ReflectComponent { /// Unique borrow of a Reflected component pub struct ReflectMut<'a> { pub(crate) value: &'a mut dyn Reflect, - pub(crate) component_ticks: &'a mut ComponentTicks, + pub(crate) component_ticks: &'a ComponentTicks, pub(crate) last_change_tick: u32, pub(crate) change_tick: u32, } diff --git a/crates/bevy_ecs/src/schedule/stage.rs b/crates/bevy_ecs/src/schedule/stage.rs index a92ffe7d674750..f264208529f3a8 100644 --- a/crates/bevy_ecs/src/schedule/stage.rs +++ b/crates/bevy_ecs/src/schedule/stage.rs @@ -2050,7 +2050,7 @@ mod tests { assert!(time_since_last_check <= MAX_DELTA); let time_since_last_check = tracker .change_tick - .wrapping_sub(tracker.component_ticks.changed); + .wrapping_sub(tracker.component_ticks.changed.get()); assert!(time_since_last_check <= MAX_DELTA); } let change_tick = world.change_tick.get_mut(); diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 0c0fd4ac2218fc..753ee6e7894b30 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -89,12 +89,22 @@ impl BlobVec { /// `index` must be in bounds /// Allows aliased mutable access to `index`'s data. Caller must ensure this does not happen #[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 + /// + 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 8d6d9edc1825bd..5a1da71d065105 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -3,7 +3,7 @@ use crate::{ entity::Entity, storage::BlobVec, }; -use std::{cell::UnsafeCell, marker::PhantomData}; +use std::marker::PhantomData; #[derive(Debug)] pub struct SparseArray { @@ -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,18 @@ 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) = 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(ComponentTicks::new(change_tick)); + self.entities.push(entity); + } } #[inline] @@ -151,28 +152,20 @@ impl ComponentSparseSet { /// # Safety /// 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, - ) - }) + pub unsafe fn get_with_ticks(&self, entity: Entity) -> Option<(*mut u8, &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), + )) } - /// # 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)) } } /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if @@ -180,7 +173,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 +188,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,8 +204,7 @@ 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 { + for component_ticks in &mut self.ticks { component_ticks.check_ticks(change_tick); } } diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 7b59c80f95bf46..499c5aa0b2206d 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -5,7 +5,6 @@ use crate::{ }; use bevy_utils::{AHasher, HashMap}; use std::{ - cell::UnsafeCell, hash::{Hash, Hasher}, ops::{Index, IndexMut}, ptr::NonNull, @@ -34,7 +33,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,30 +42,41 @@ 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 { - // SAFE: unique access - unsafe { &mut *self.ticks.get() } + 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) = 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).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] @@ -84,7 +94,7 @@ impl Column { #[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) } /// # Safety @@ -92,7 +102,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] @@ -101,7 +111,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); (data, ticks) } @@ -109,28 +119,26 @@ 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(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 ComponentTicks { + self.ticks.as_ptr() } /// # Safety @@ -142,16 +150,16 @@ impl Column { } /// # Safety - /// must ensure rust mutability rules are not violated + /// index must be in-bounds #[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_unchecked(&self, row: usize) -> &ComponentTicks { + debug_assert!(row < self.ticks.len()); + self.ticks.get_unchecked(row) } #[inline] pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { - for component_ticks in self.ticks_mut() { + for component_ticks in &mut self.ticks { component_ticks.check_ticks(change_tick); } } @@ -225,7 +233,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 { @@ -255,7 +263,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); } @@ -287,7 +295,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, @@ -337,7 +345,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(ComponentTicks::new(0)); } index } @@ -494,7 +502,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 bd93e844eb266c..3b5b791b39298c 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -287,8 +287,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().as_ptr().cast::(), + ticks: &*column.get_ticks_ptr(), last_change_tick: system_state.last_change_tick, change_tick, } @@ -325,8 +325,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().as_ptr().cast::(), + ticks: &*column.get_ticks_ptr(), last_change_tick: system_state.last_change_tick, change_tick, }) @@ -342,7 +342,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResState { /// Use `Option>` instead if the resource might not always exist. pub struct ResMut<'w, T: Component> { value: &'w mut T, - ticks: &'w mut ComponentTicks, + ticks: &'w ComponentTicks, last_change_tick: u32, change_tick: u32, } @@ -827,9 +827,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().as_ptr().cast::(), + ticks: column.get_ticks_unchecked(0).clone(), last_change_tick: system_state.last_change_tick, change_tick, } @@ -848,7 +849,7 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendState { /// Panics when used as a `SystemParameter` if the resource does not exist. pub struct NonSendMut<'a, T: 'static> { pub(crate) value: &'a mut T, - ticks: &'a mut ComponentTicks, + ticks: &'a ComponentTicks, last_change_tick: u32, change_tick: u32, } @@ -959,8 +960,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().as_ptr().cast::(), + ticks: column.get_ticks_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 daf5c31d7bb1cc..bffb2a0b44d1e6 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -80,7 +80,7 @@ impl<'w> EntityRef<'w> { get_component_and_ticks_with_type(self.world, TypeId::of::(), self.entity, self.location) .map(|(value, ticks)| Mut { value: &mut *value.cast::(), - component_ticks: &mut *ticks, + component_ticks: &*ticks, last_change_tick, change_tick, }) @@ -161,7 +161,7 @@ impl<'w> EntityMut<'w> { ) .map(|(value, ticks)| Mut { value: &mut *value.cast::(), - component_ticks: &mut *ticks, + component_ticks: &*ticks, last_change_tick: self.world.last_change_tick(), change_tick: self.world.change_tick(), }) @@ -176,7 +176,7 @@ impl<'w> EntityMut<'w> { get_component_and_ticks_with_type(self.world, TypeId::of::(), self.entity, self.location) .map(|(value, ticks)| Mut { value: &mut *value.cast::(), - component_ticks: &mut *ticks, + component_ticks: &*ticks, last_change_tick: self.world.last_change_tick(), change_tick: self.world.read_change_tick(), }) @@ -184,16 +184,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, @@ -262,26 +252,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, @@ -565,7 +561,7 @@ unsafe fn get_component_and_ticks( component_id: ComponentId, entity: Entity, location: EntityLocation, -) -> Option<(*mut u8, *mut ComponentTicks)> { +) -> Option<(*mut u8, *const ComponentTicks)> { let archetype = &world.archetypes[location.archetype_id]; let component_info = world.components.get_info_unchecked(component_id); match component_info.storage_type() { @@ -576,14 +572,20 @@ unsafe fn get_component_and_ticks( // 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_ticks_unchecked(table_row) as *const ComponentTicks, )) } - StorageType::SparseSet => world - .storages - .sparse_sets - .get(component_id) - .and_then(|sparse_set| sparse_set.get_with_ticks(entity)), + StorageType::SparseSet => { + world + .storages + .sparse_sets + .get(component_id) + .and_then(|sparse_set| { + sparse_set + .get_with_ticks(entity) + .map(|(d, t)| (d, t as *const ComponentTicks)) + }) + } } } @@ -647,7 +649,7 @@ pub(crate) unsafe fn get_component_and_ticks_with_type( type_id: TypeId, entity: Entity, location: EntityLocation, -) -> Option<(*mut u8, *mut ComponentTicks)> { +) -> Option<(*mut u8, *const ComponentTicks)> { let component_id = world.components.get_id(type_id)?; get_component_and_ticks(world, component_id, entity, location) } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 9f46b2f355e4b6..82dc4ae1833daf 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -593,14 +593,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: resurces 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: resurces table always have row 0 + let ticks = unsafe { column.get_ticks_unchecked(0) }; ticks.is_changed(self.last_change_tick(), self.read_change_tick()) } @@ -733,7 +735,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 @@ -746,8 +748,8 @@ impl World { ) -> Option> { let column = self.get_populated_resource_column(component_id)?; Some(Mut { - value: &mut *column.get_ptr().as_ptr().cast::(), - component_ticks: &mut *column.get_ticks_mut_ptr(), + value: &mut *column.get_data_ptr().as_ptr().cast::(), + component_ticks: column.get_ticks_unchecked(0), last_change_tick: self.last_change_tick(), change_tick: self.read_change_tick(), }) diff --git a/crates/bevy_ecs/src/world/pointer.rs b/crates/bevy_ecs/src/world/pointer.rs index dbca45b6f93e28..0a2247e1033182 100644 --- a/crates/bevy_ecs/src/world/pointer.rs +++ b/crates/bevy_ecs/src/world/pointer.rs @@ -4,7 +4,7 @@ use std::ops::{Deref, DerefMut}; /// Unique borrow of an entity's component pub struct Mut<'a, T> { pub(crate) value: &'a mut T, - pub(crate) component_ticks: &'a mut ComponentTicks, + pub(crate) component_ticks: &'a ComponentTicks, pub(crate) last_change_tick: u32, pub(crate) change_tick: u32, } diff --git a/crates/bevy_transform/src/hierarchy/child_builder.rs b/crates/bevy_transform/src/hierarchy/child_builder.rs index 0a97c4a8503822..1c1ab208eefa4e 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));