reintroduce UnsafeCell around individual ComponentTicks and get rid o…

…f incorrect Cell in multithreaded context

crates/bevy_ecs/src/component/mod.rs

@@ -6,7 +6,6 @@ use crate::storage::SparseSetIndex;
 use std::{
     alloc::Layout,
     any::{Any, TypeId},
-    cell::Cell,
     collections::hash_map::Entry,
 };
 use thiserror::Error;
@@ -310,7 +309,7 @@ impl Components {
 #[derive(Clone, Debug)]
 pub struct ComponentTicks {
     pub(crate) added: u32,
-    pub(crate) changed: Cell<u32>,
+    pub(crate) changed: u32,
 }
 
 impl ComponentTicks {
@@ -327,7 +326,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.get());
+        let component_delta = change_tick.wrapping_sub(self.changed);
         let system_delta = change_tick.wrapping_sub(last_change_tick);
 
         component_delta < system_delta
@@ -336,13 +335,13 @@ impl ComponentTicks {
     pub(crate) fn new(change_tick: u32) -> Self {
         Self {
             added: change_tick,
-            changed: Cell::new(change_tick),
+            changed: change_tick,
         }
     }
 
     pub(crate) fn check_ticks(&mut self, change_tick: u32) {
         check_tick(&mut self.added, change_tick);
-        check_tick(self.changed.get_mut(), change_tick);
+        check_tick(&mut self.changed, change_tick);
     }
 
     /// Manually sets the change tick.
@@ -358,8 +357,8 @@ impl ComponentTicks {
     /// component_ticks.set_changed(world.read_change_tick());
     /// ```
     #[inline]
-    pub fn set_changed(&self, change_tick: u32) {
-        self.changed.set(change_tick);
+    pub fn set_changed(&mut self, change_tick: u32) {
+        self.changed = change_tick;
     }
 }
 

crates/bevy_ecs/src/query/fetch.rs

@@ -8,6 +8,7 @@ use crate::{
 };
 use bevy_ecs_macros::all_tuples;
 use std::{
+    cell::UnsafeCell,
     marker::PhantomData,
     ptr::{self, NonNull},
 };
@@ -386,7 +387,7 @@ impl<T: Component> WorldQuery for &mut T {
 pub struct WriteFetch<T> {
     storage_type: StorageType,
     table_components: NonNull<T>,
-    table_ticks: *const ComponentTicks,
+    table_ticks: *const UnsafeCell<ComponentTicks>,
     entities: *const Entity,
     entity_table_rows: *const usize,
     sparse_set: *const ComponentSparseSet,
@@ -481,7 +482,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,
         };
@@ -529,7 +530,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
                 let table_row = *self.entity_table_rows.add(archetype_index);
                 Mut {
                     value: &mut *self.table_components.as_ptr().add(table_row),
-                    component_ticks: &*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,
                 }
@@ -540,7 +541,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
                     (*self.sparse_set).get_with_ticks(entity).unwrap();
                 Mut {
                     value: &mut *component.cast::<T>(),
-                    component_ticks: &*component_ticks,
+                    component_ticks: &mut *component_ticks,
                     change_tick: self.change_tick,
                     last_change_tick: self.last_change_tick,
                 }
@@ -552,7 +553,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
     unsafe fn table_fetch(&mut self, table_row: usize) -> Self::Item {
         Mut {
             value: &mut *self.table_components.as_ptr().add(table_row),
-            component_ticks: &*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,
         }
@@ -853,7 +854,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_ptr();
+                self.table_ticks = column.get_ticks_const_ptr();
             }
             StorageType::SparseSet => self.entities = archetype.entities().as_ptr(),
         }
@@ -864,7 +865,7 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch<T> {
         self.table_ticks = table
             .get_column(state.component_id)
             .unwrap()
-            .get_ticks_ptr();
+            .get_ticks_const_ptr();
     }
 
     #[inline]

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: *const 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::<ComponentTicks>(),
+                    table_ticks: ptr::null::<UnsafeCell<ComponentTicks>>(),
                     entities: ptr::null::<Entity>(),
                     entity_table_rows: ptr::null::<usize>(),
                     sparse_set: ptr::null::<ComponentSparseSet>(),
@@ -672,14 +672,14 @@ macro_rules! impl_tick_filter {
             }
 
             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);

crates/bevy_ecs/src/reflect.rs

@@ -116,7 +116,7 @@ impl<C: Component + Reflect + FromWorld> FromType<C> for ReflectComponent {
 /// Unique borrow of a Reflected component
 pub struct ReflectMut<'a> {
     pub(crate) value: &'a mut dyn Reflect,
-    pub(crate) component_ticks: &'a ComponentTicks,
+    pub(crate) component_ticks: &'a mut ComponentTicks,
     pub(crate) last_change_tick: u32,
     pub(crate) change_tick: u32,
 }

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.get());
+                    .wrapping_sub(tracker.component_ticks.changed);
                 assert!(time_since_last_check <= MAX_DELTA);
             }
             let change_tick = world.change_tick.get_mut();

crates/bevy_ecs/src/storage/sparse_set.rs

@@ -3,7 +3,7 @@ use crate::{
     entity::Entity,
     storage::BlobVec,
 };
-use std::marker::PhantomData;
+use std::{cell::UnsafeCell, marker::PhantomData};
 
 #[derive(Debug)]
 pub struct SparseArray<I, V = I> {
@@ -87,7 +87,7 @@ impl<I: SparseSetIndex, V> SparseArray<I, V> {
 #[derive(Debug)]
 pub struct ComponentSparseSet {
     dense: BlobVec,
-    ticks: Vec<ComponentTicks>,
+    ticks: Vec<UnsafeCell<ComponentTicks>>,
     entities: Vec<Entity>,
     sparse: SparseArray<Entity, usize>,
 }
@@ -122,14 +122,16 @@ impl ComponentSparseSet {
     pub unsafe fn insert(&mut self, entity: Entity, value: *mut u8, change_tick: u32) {
         if let Some(&dense_index) = self.sparse.get(entity) {
             self.dense.replace_unchecked(dense_index, value);
-            *self.ticks.get_unchecked_mut(dense_index) = ComponentTicks::new(change_tick);
+            *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(ComponentTicks::new(change_tick));
+            self.ticks
+                .push(UnsafeCell::new(ComponentTicks::new(change_tick)));
             self.entities.push(entity);
         }
     }
@@ -152,20 +154,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, &ComponentTicks)> {
+    pub unsafe fn get_with_ticks(&self, entity: Entity) -> Option<(*mut u8, *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),
+            self.ticks.get_unchecked(dense_index).get(),
         ))
     }
 
     #[inline]
     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)) }
+        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
@@ -205,7 +207,7 @@ impl ComponentSparseSet {
 
     pub(crate) fn check_change_ticks(&mut self, change_tick: u32) {
         for component_ticks in &mut self.ticks {
-            component_ticks.check_ticks(change_tick);
+            component_ticks.get_mut().check_ticks(change_tick);
         }
     }
 }

crates/bevy_ecs/src/storage/table.rs

@@ -5,6 +5,7 @@ use crate::{
 };
 use bevy_utils::{AHasher, HashMap};
 use std::{
+    cell::UnsafeCell,
     hash::{Hash, Hasher},
     ops::{Index, IndexMut},
     ptr::NonNull,
@@ -33,7 +34,7 @@ impl TableId {
 pub struct Column {
     pub(crate) component_id: ComponentId,
     pub(crate) data: BlobVec,
-    pub(crate) ticks: Vec<ComponentTicks>,
+    pub(crate) ticks: Vec<UnsafeCell<ComponentTicks>>,
 }
 
 impl Column {
@@ -56,7 +57,7 @@ impl Column {
     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;
+        *self.ticks.get_unchecked_mut(row).get_mut() = ticks;
     }
 
     /// Writes component data to the column at given row.
@@ -68,7 +69,10 @@ impl Column {
     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);
+        self.ticks
+            .get_unchecked_mut(row)
+            .get_mut()
+            .set_changed(change_tick);
     }
 
     /// # Safety
@@ -90,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.get_unchecked_mut(row)
+        self.ticks.get_unchecked_mut(row).get_mut()
     }
 
     /// # Safety
@@ -111,7 +115,7 @@ impl Column {
         row: usize,
     ) -> (*mut u8, ComponentTicks) {
         let data = self.data.swap_remove_and_forget_unchecked(row);
-        let ticks = self.ticks.swap_remove(row);
+        let ticks = self.ticks.swap_remove(row).into_inner();
         (data, ticks)
     }
 
@@ -120,7 +124,7 @@ impl Column {
     pub(crate) unsafe fn push(&mut self, ptr: *mut u8, ticks: ComponentTicks) {
         let row = self.data.push_uninit();
         self.data.initialize_unchecked(row, ptr);
-        self.ticks.push(ticks);
+        self.ticks.push(UnsafeCell::new(ticks));
     }
 
     #[inline]
@@ -137,14 +141,22 @@ impl Column {
     }
 
     #[inline]
-    pub fn get_ticks_ptr(&self) -> *const ComponentTicks {
+    pub fn get_ticks_ptr(&self) -> *const UnsafeCell<ComponentTicks> {
         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)
     }
@@ -154,13 +166,23 @@ impl Column {
     #[inline]
     pub unsafe fn get_ticks_unchecked(&self, row: usize) -> &ComponentTicks {
         debug_assert!(row < self.ticks.len());
-        self.ticks.get_unchecked(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 &mut self.ticks {
-            component_ticks.check_ticks(change_tick);
+            component_ticks.get_mut().check_ticks(change_tick);
         }
     }
 }
@@ -345,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.push(ComponentTicks::new(0));
+            column.ticks.push(UnsafeCell::new(ComponentTicks::new(0)));
         }
         index
     }