diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index 8e8e2702a413d..5a718bd6baf8d 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -317,8 +317,8 @@ pub struct ArchetypeGeneration(usize); impl ArchetypeGeneration { #[inline] - pub fn new(generation: usize) -> Self { - ArchetypeGeneration(generation) + pub const fn initial() -> Self { + ArchetypeGeneration(0) } #[inline] diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index e71188a8cd94e..d8a12f3a7b2a5 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -54,7 +54,7 @@ where let mut state = Self { world_id: world.id(), - archetype_generation: ArchetypeGeneration::new(usize::MAX), + archetype_generation: ArchetypeGeneration::initial(), matched_table_ids: Vec::new(), matched_archetype_ids: Vec::new(), fetch_state, @@ -74,17 +74,10 @@ where std::any::type_name::()); } let archetypes = world.archetypes(); - let old_generation = self.archetype_generation; - let archetype_index_range = if old_generation == archetypes.generation() { - 0..0 - } else { - self.archetype_generation = archetypes.generation(); - if old_generation.value() == usize::MAX { - 0..archetypes.len() - } else { - old_generation.value()..archetypes.len() - } - }; + let new_generation = archetypes.generation(); + let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation); + let archetype_index_range = old_generation.value()..new_generation.value(); + for archetype_index in archetype_index_range { self.new_archetype(&archetypes[ArchetypeId::new(archetype_index)]); } diff --git a/crates/bevy_ecs/src/schedule/executor.rs b/crates/bevy_ecs/src/schedule/executor.rs index 9f35674af1da8..6b48f4b40f1e0 100644 --- a/crates/bevy_ecs/src/schedule/executor.rs +++ b/crates/bevy_ecs/src/schedule/executor.rs @@ -18,8 +18,7 @@ pub struct SingleThreadedExecutor { impl Default for SingleThreadedExecutor { fn default() -> Self { Self { - // MAX ensures access information will be initialized on first run. - archetype_generation: ArchetypeGeneration::new(usize::MAX), + archetype_generation: ArchetypeGeneration::initial(), } } } @@ -46,24 +45,15 @@ impl SingleThreadedExecutor { /// [update_archetypes] and updates cached archetype_component_access. fn update_archetypes(&mut self, systems: &mut [ParallelSystemContainer], world: &World) { let archetypes = world.archetypes(); - let old_generation = self.archetype_generation; let new_generation = archetypes.generation(); - if old_generation == new_generation { - return; - } + let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation); + let archetype_index_range = old_generation.value()..new_generation.value(); - let archetype_index_range = if old_generation.value() == usize::MAX { - 0..archetypes.len() - } else { - old_generation.value()..archetypes.len() - }; for archetype in archetypes.archetypes[archetype_index_range].iter() { for container in systems.iter_mut() { let system = container.system_mut(); system.new_archetype(archetype); } } - - self.archetype_generation = new_generation; } } diff --git a/crates/bevy_ecs/src/schedule/executor_parallel.rs b/crates/bevy_ecs/src/schedule/executor_parallel.rs index 8db225759b2e4..af989ef9eecc1 100644 --- a/crates/bevy_ecs/src/schedule/executor_parallel.rs +++ b/crates/bevy_ecs/src/schedule/executor_parallel.rs @@ -58,8 +58,7 @@ impl Default for ParallelExecutor { fn default() -> Self { let (finish_sender, finish_receiver) = async_channel::unbounded(); Self { - // MAX ensures access information will be initialized on first run. - archetype_generation: ArchetypeGeneration::new(usize::MAX), + archetype_generation: ArchetypeGeneration::initial(), system_metadata: Default::default(), finish_sender, finish_receiver, @@ -152,17 +151,10 @@ impl ParallelExecutor { /// [update_archetypes] and updates cached archetype_component_access. fn update_archetypes(&mut self, systems: &mut [ParallelSystemContainer], world: &World) { let archetypes = world.archetypes(); - let old_generation = self.archetype_generation; let new_generation = archetypes.generation(); - if old_generation == new_generation { - return; - } + let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation); + let archetype_index_range = old_generation.value()..new_generation.value(); - let archetype_index_range = if old_generation.value() == usize::MAX { - 0..archetypes.len() - } else { - old_generation.value()..archetypes.len() - }; for archetype in archetypes.archetypes[archetype_index_range].iter() { for (index, container) in systems.iter_mut().enumerate() { let meta = &mut self.system_metadata[index]; @@ -172,8 +164,6 @@ impl ParallelExecutor { .extend(system.archetype_component_access()); } } - - self.archetype_generation = new_generation; } /// Populates `should_run` bitset, spawns tasks for systems that should run this iteration, diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index b236d8882735c..dddd8fcecba4a 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -1,6 +1,5 @@ use std::{ alloc::{handle_alloc_error, Layout}, - cell::UnsafeCell, ptr::NonNull, }; @@ -9,8 +8,8 @@ pub struct BlobVec { item_layout: Layout, capacity: usize, len: usize, - data: UnsafeCell>, - swap_scratch: UnsafeCell>, + data: NonNull, + swap_scratch: NonNull, drop: unsafe fn(*mut u8), } @@ -18,8 +17,8 @@ impl BlobVec { pub fn new(item_layout: Layout, drop: unsafe fn(*mut u8), capacity: usize) -> BlobVec { if item_layout.size() == 0 { BlobVec { - swap_scratch: UnsafeCell::new(NonNull::dangling()), - data: UnsafeCell::new(NonNull::dangling()), + swap_scratch: NonNull::dangling(), + data: NonNull::dangling(), capacity: usize::MAX, len: 0, item_layout, @@ -29,8 +28,8 @@ impl BlobVec { let swap_scratch = NonNull::new(unsafe { std::alloc::alloc(item_layout) }) .unwrap_or_else(|| std::alloc::handle_alloc_error(item_layout)); let mut blob_vec = BlobVec { - swap_scratch: UnsafeCell::new(swap_scratch), - data: UnsafeCell::new(NonNull::dangling()), + swap_scratch, + data: NonNull::dangling(), capacity: 0, len: 0, item_layout, @@ -81,9 +80,7 @@ impl BlobVec { ) }; - self.data = UnsafeCell::new( - NonNull::new(new_data).unwrap_or_else(|| handle_alloc_error(new_layout)), - ); + self.data = NonNull::new(new_data).unwrap_or_else(|| handle_alloc_error(new_layout)); } self.capacity = new_capacity; } @@ -132,7 +129,7 @@ impl BlobVec { pub unsafe fn swap_remove_and_forget_unchecked(&mut self, index: usize) -> *mut u8 { debug_assert!(index < self.len()); let last = self.len - 1; - let swap_scratch = (*self.swap_scratch.get()).as_ptr(); + let swap_scratch = self.swap_scratch.as_ptr(); std::ptr::copy_nonoverlapping( self.get_unchecked(index), swap_scratch, @@ -170,7 +167,7 @@ impl BlobVec { /// must ensure rust mutability rules are not violated #[inline] pub unsafe fn get_ptr(&self) -> NonNull { - *self.data.get() + self.data } pub fn clear(&mut self) { @@ -199,7 +196,7 @@ impl Drop for BlobVec { array_layout(&self.item_layout, self.capacity) .expect("array layout should be valid"), ); - std::alloc::dealloc((*self.swap_scratch.get()).as_ptr(), self.item_layout); + std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout); } } } diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 280c73c06de4f..8d6d9edc1825b 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -180,10 +180,7 @@ impl ComponentSparseSet { /// returned). pub fn remove_and_forget(&mut self, entity: Entity) -> Option<*mut u8> { self.sparse.remove(entity).map(|dense_index| { - // SAFE: unique access to ticks - unsafe { - (*self.ticks.get()).swap_remove(dense_index); - } + self.ticks.get_mut().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 diff --git a/crates/bevy_ecs/src/system/commands.rs b/crates/bevy_ecs/src/system/commands.rs index 627b9e4e73531..90adfb8dc510c 100644 --- a/crates/bevy_ecs/src/system/commands.rs +++ b/crates/bevy_ecs/src/system/commands.rs @@ -417,9 +417,29 @@ impl Command for RemoveResource { #[allow(clippy::float_cmp, clippy::approx_constant)] mod tests { use crate::{ + component::{ComponentDescriptor, StorageType}, system::{CommandQueue, Commands}, world::World, }; + use std::sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, + }; + + #[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 commands() { @@ -454,10 +474,20 @@ mod tests { #[test] fn remove_components() { let mut world = World::default(); + + struct DenseDropCk(DropCk); + world + .register_component(ComponentDescriptor::new::(StorageType::SparseSet)) + .unwrap(); + let mut command_queue = CommandQueue::default(); + let (dense_dropck, dense_is_dropped) = DropCk::new_pair(); + let dense_dropck = DenseDropCk(dense_dropck); + let (sparse_dropck, sparse_is_dropped) = DropCk::new_pair(); + let entity = Commands::new(&mut command_queue, &world) .spawn() - .insert_bundle((1u32, 2u64)) + .insert_bundle((1u32, 2u64, dense_dropck, sparse_dropck)) .id(); command_queue.apply(&mut world); let results_before = world @@ -471,8 +501,14 @@ mod tests { Commands::new(&mut command_queue, &world) .entity(entity) .remove::() - .remove_bundle::<(u32, u64)>(); + .remove_bundle::<(u32, u64, DenseDropCk, DropCk)>(); + + assert_eq!(dense_is_dropped.load(Ordering::Relaxed), 0); + assert_eq!(sparse_is_dropped.load(Ordering::Relaxed), 0); command_queue.apply(&mut world); + assert_eq!(dense_is_dropped.load(Ordering::Relaxed), 1); + assert_eq!(sparse_is_dropped.load(Ordering::Relaxed), 1); + let results_after = world .query::<(&u32, &u64)>() .iter(&world) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index f8cb1dc60a183..f2c399e9bb020 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -325,7 +325,7 @@ impl<'w> EntityMut<'w> { T::from_components(|| { let component_id = bundle_components.next().unwrap(); // SAFE: entity location is valid and table row is removed below - remove_component( + take_component( components, storages, old_archetype, @@ -406,17 +406,18 @@ impl<'w> EntityMut<'w> { let entity = self.entity; for component_id in bundle_info.component_ids.iter().cloned() { if old_archetype.contains(component_id) { - // SAFE: entity location is valid and table row is removed below - unsafe { - remove_component( - components, - storages, - old_archetype, - removed_components, - component_id, - entity, - old_location, - ); + removed_components + .get_or_insert_with(component_id, Vec::new) + .push(entity); + + // Make sure to drop components stored in sparse sets. + // Dense components are dropped later in `move_to_and_drop_missing_unchecked`. + if let Some(StorageType::SparseSet) = old_archetype.get_storage_type(component_id) { + storages + .sparse_sets + .get_mut(component_id) + .unwrap() + .remove(entity); } } } @@ -586,13 +587,18 @@ unsafe fn get_component_and_ticks( } } +/// Moves component data out of storage. +/// +/// This function leaves the underlying memory unchanged, but the component behind +/// returned pointer is semantically owned by the caller and will not be dropped in its original location. +/// Caller is responsible to drop component data behind returned pointer. +/// /// # Safety -// `entity_location` must be within bounds of the given archetype and `entity` must exist inside the -// archetype -/// The relevant table row must be removed separately -/// `component_id` must be valid +/// - `entity_location` must be within bounds of the given archetype and `entity` must exist inside the archetype +/// - `component_id` must be valid +/// - The relevant table row **must be removed** by the caller once all components are taken #[inline] -unsafe fn remove_component( +unsafe fn take_component( components: &Components, storages: &mut Storages, archetype: &Archetype,