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 <mcanders1@gmail.com>

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);
+                    match bundle_status.get_unchecked(bundle_component) {
-                    let column_status = column.get_ticks_unchecked_mut(table_row);
-                    match component_status {
                         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);
                         }
                     }
                 }

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,

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<AtomicUsize>);
+    impl DropCk {
+        fn new_pair() -> (Self, Arc<AtomicUsize>) {
+            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::<i32>().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::<DropCk>(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);
+    }
 }

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<T> {
                 let column = tables[archetype.table_id()]
                     .get_column(state.component_id)
                     .unwrap();
-                self.table_components = column.get_ptr().cast::<T>();
+                self.table_components = column.get_data_ptr().cast::<T>();
             }
             StorageType::SparseSet => self.entities = archetype.entities().as_ptr(),
         }
@@ -354,7 +355,7 @@ impl<'w, T: Component> Fetch<'w> for ReadFetch<T> {
         self.table_components = table
             .get_column(state.component_id)
             .unwrap()
-            .get_ptr()
+            .get_data_ptr()
             .cast::<T>();
     }
 
@@ -387,7 +388,7 @@ impl<T: Component> WorldQuery for &mut T {
 pub struct WriteFetch<T> {
     storage_type: StorageType,
     table_components: NonNull<T>,
-    table_ticks: *mut ComponentTicks,
+    table_ticks: *const UnsafeCell<ComponentTicks>,
     entities: *const Entity,
     entity_table_rows: *const usize,
     sparse_set: *const ComponentSparseSet,
@@ -482,7 +483,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
             entities: ptr::null::<Entity>(),
             entity_table_rows: ptr::null::<usize>(),
             sparse_set: ptr::null::<ComponentSparseSet>(),
-            table_ticks: ptr::null_mut::<ComponentTicks>(),
+            table_ticks: ptr::null::<UnsafeCell<ComponentTicks>>(),
             last_change_tick,
             change_tick,
         };
@@ -509,8 +510,8 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
                 let column = tables[archetype.table_id()]
                     .get_column(state.component_id)
                     .unwrap();
-                self.table_components = column.get_ptr().cast::<T>();
+                self.table_components = column.get_data_ptr().cast::<T>();
-                self.table_ticks = column.get_ticks_mut_ptr();
+                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<T> {
     #[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::<T>();
+        self.table_components = column.get_data_ptr().cast::<T>();
-        self.table_ticks = column.get_ticks_mut_ptr();
+        self.table_ticks = column.get_ticks_ptr();
     }
 
     #[inline]
@@ -531,7 +532,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
                 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<T> {
         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<T> {
                 let column = tables[archetype.table_id()]
                     .get_column(state.component_id)
                     .unwrap();
-                self.table_ticks = column.get_ticks_mut_ptr().cast::<ComponentTicks>();
+                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<T> {
         self.table_ticks = table
             .get_column(state.component_id)
             .unwrap()
-            .get_ticks_mut_ptr()
+            .get_ticks_const_ptr();
-            .cast::<ComponentTicks>();
     }
 
     #[inline]
@@ -881,7 +881,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch<T> {
             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<T> {
             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<T> {
     #[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,

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<T> {
             storage_type: StorageType,
-            table_ticks: *mut ComponentTicks,
+            table_ticks: *const UnsafeCell<ComponentTicks>,
             entity_table_rows: *const usize,
             marker: PhantomData<T>,
             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::<ComponentTicks>(),
+                    table_ticks: ptr::null::<UnsafeCell<ComponentTicks>>(),
                     entities: ptr::null::<Entity>(),
                     entity_table_rows: ptr::null::<usize>(),
                     sparse_set: ptr::null::<ComponentSparseSet>(),
@@ -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)
                     }
                 }

crates/bevy_ecs/src/storage/blob_vec.rs

@@ -86,15 +86,25 @@ impl BlobVec {
     }
 
     /// # Safety
-    /// `index` must be in bounds
+    /// - index must be in bounds
-    /// Allows aliased mutable access to `index`'s data. Caller must ensure this does not happen
+    /// - 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::<T>()
     unsafe fn push<T>(blob_vec: &mut BlobVec, mut value: T) {
         let index = blob_vec.push_uninit();
-        blob_vec.set_unchecked(index, (&mut value as *mut T).cast::<u8>());
+        blob_vec.initialize_unchecked(index, (&mut value as *mut T).cast::<u8>());
         std::mem::forget(value);
     }
 

crates/bevy_ecs/src/storage/sparse_set.rs

@@ -87,7 +87,7 @@ impl<I: SparseSetIndex, V> SparseArray<I, V> {
 #[derive(Debug)]
 pub struct ComponentSparseSet {
     dense: BlobVec,
-    ticks: UnsafeCell<Vec<ComponentTicks>>,
+    ticks: Vec<UnsafeCell<ComponentTicks>>,
     entities: Vec<Entity>,
     sparse: SparseArray<Entity, usize>,
 }
@@ -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;
+        if let Some(&dense_index) = self.sparse.get(entity) {
-        let entities = &mut self.entities;
+            self.dense.replace_unchecked(dense_index, value);
-        let ticks_list = self.ticks.get_mut();
+            *self.ticks.get_unchecked_mut(dense_index) =
-        let dense_index = *self.sparse.get_or_insert_with(entity, move || {
+                UnsafeCell::new(ComponentTicks::new(change_tick));
-            ticks_list.push(ComponentTicks::new(change_tick));
+        } else {
-            entities.push(entity);
+            let dense_index = self.dense.push_uninit();
-            dense.push_uninit()
+            self.dense.initialize_unchecked(dense_index, value);
-        });
+            self.sparse.insert(entity, dense_index);
-        // SAFE: dense_index exists thanks to the call above
+            debug_assert_eq!(self.ticks.len(), dense_index);
-        self.dense.set_unchecked(dense_index, value);
+            debug_assert_eq!(self.entities.len(), dense_index);
-        ((*self.ticks.get()).get_unchecked_mut(dense_index)).set_changed(change_tick);
+            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();
+        let dense_index = *self.sparse.get(entity)?;
-        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
-            (
+        Some((
             self.dense.get_unchecked(dense_index),
-                ticks.get_unchecked_mut(dense_index) as *mut ComponentTicks,
+            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> {
+    pub fn get_ticks(&self, entity: Entity) -> Option<&ComponentTicks> {
-        let ticks = &mut *self.ticks.get();
+        let dense_index = *self.sparse.get(entity)?;
-        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)
+        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 &mut self.ticks {
-        for component_ticks in ticks {
+            component_ticks.get_mut().check_ticks(change_tick);
-            component_ticks.check_ticks(change_tick);
         }
     }
 }

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<Vec<ComponentTicks>>,
+    pub(crate) ticks: Vec<UnsafeCell<ComponentTicks>>,
 }
 
 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),
         }
     }
 
-    #[inline]
+    /// Writes component data to the column at given row.
-    fn ticks_mut(&mut self) -> &mut Vec<ComponentTicks> {
+    /// Assumes the slot is uninitialized, drop is not called.
-        self.ticks.get_mut()
+    /// To overwrite existing initialized value, use `replace` instead.
-    }
+    ///
-
     /// # 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) {
+    pub unsafe fn initialize(&mut self, row: usize, data: *mut u8, ticks: ComponentTicks) {
-        self.set_data_unchecked(row, data);
-        *self.ticks_mut().get_unchecked_mut(row) = ticks;
-    }
-
-    /// # 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) {
         debug_assert!(row < self.len());
-        self.data.set_unchecked(row, data);
+        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 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 initialize_data(&mut self, row: usize, data: *mut u8) {
+        debug_assert!(row < self.len());
+        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.data.initialize_unchecked(row, ptr);
-        self.ticks_mut().push(ticks);
+        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<u8> {
+    pub unsafe fn get_data_ptr(&self) -> NonNull<u8> {
         self.data.get_ptr()
     }
 
-    /// # Safety
-    /// must ensure rust mutability rules are not violated
     #[inline]
-    pub unsafe fn get_ticks_mut_ptr(&self) -> *mut ComponentTicks {
+    pub fn get_ticks_ptr(&self) -> *const UnsafeCell<ComponentTicks> {
-        (*self.ticks.get()).as_mut_ptr()
+        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) -> *mut ComponentTicks {
+    pub unsafe fn get_ticks_unchecked(&self, row: usize) -> &ComponentTicks {
-        debug_assert!(row < (*self.ticks.get()).len());
+        debug_assert!(row < self.ticks.len());
-        self.get_ticks_mut_ptr().add(row)
+        &*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_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() {
+        for component_ticks in &mut self.ticks {
-            component_ticks.check_ticks(change_tick);
+            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);
             };
         }
 

crates/bevy_ecs/src/system/system_param.rs

@@ -289,8 +289,8 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResState<T> {
                 )
             });
         Res {
-            value: &*column.get_ptr().as_ptr().cast::<T>(),
+            value: &*column.get_data_ptr().cast::<T>().as_ptr(),
-            ticks: &*column.get_ticks_mut_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<T> {
         world
             .get_populated_resource_column(state.0.component_id)
             .map(|column| Res {
-                value: &*column.get_ptr().as_ptr().cast::<T>(),
+                value: &*column.get_data_ptr().cast::<T>().as_ptr(),
-                ticks: &*column.get_ticks_mut_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<T> {
                     std::any::type_name::<T>()
                 )
             });
+
         NonSend {
-            value: &*column.get_ptr().as_ptr().cast::<T>(),
+            value: &*column.get_data_ptr().cast::<T>().as_ptr(),
-            ticks: *column.get_ticks_mut_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<T> {
                 )
             });
         NonSendMut {
-            value: &mut *column.get_ptr().as_ptr().cast::<T>(),
+            value: &mut *column.get_data_ptr().cast::<T>().as_ptr(),
-            ticks: &mut *column.get_ticks_mut_ptr(),
+            ticks: &mut *column.get_ticks_mut_ptr_unchecked(0),
             last_change_tick: system_state.last_change_tick,
             change_tick,
         }

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<T: 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::<T>(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::<T>(&mut self.world.components);
+
         let (archetype, bundle_status, new_location) = unsafe {
             get_insert_bundle_info(
-                entities,
+                &mut self.world.entities,
-                archetypes,
+                &mut self.world.archetypes,
-                components,
+                &mut self.world.components,
-                storages,
+                &mut self.world.storages,
                 bundle_info,
-                current_location,
+                self.location,
-                entity,
+                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,
+                &mut self.world.storages.sparse_sets,
-                entity,
+                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_data_unchecked(table_row),
-                components.get_ticks_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

crates/bevy_ecs/src/world/mod.rs

@@ -594,14 +594,16 @@ impl World {
     pub fn is_resource_added<T: Component>(&self) -> bool {
         let component_id = self.components.get_resource_id(TypeId::of::<T>()).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<T: Component>(&self) -> bool {
         let component_id = self.components.get_resource_id(TypeId::of::<T>()).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::<T>())
+        Some(&*column.get_data_ptr().as_ptr().cast::<T>())
     }
 
     /// # Safety
@@ -749,9 +751,9 @@ impl World {
     ) -> Option<Mut<'_, T>> {
         let column = self.get_populated_resource_column(component_id)?;
         Some(Mut {
-            value: &mut *column.get_ptr().as_ptr().cast::<T>(),
+            value: &mut *column.get_data_ptr().cast::<T>().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::<T>() = value;
+            *column.get_data_unchecked(0).cast::<T>() = value;
             column.get_ticks_unchecked_mut(0).set_changed(change_tick);
         }
     }

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::<Children>(self.parent) {
                 children.0.insert_from_slice(self.index, &self.children);
-                added = true;
+            } else {
-            }
-
-            // NOTE: ideally this is just an else statement, but currently that _incorrectly_ fails
-            // borrow-checking
-            if !added {
                 world
                     .entity_mut(self.parent)
                     .insert(Children(self.children));