Construct and deconstruct entities to improve entity allocation

crates/bevy_app/src/app.rs

@@ -1582,7 +1582,7 @@ mod tests {
         app.add_systems(EnterMainMenu, (foo, bar));
 
         app.world_mut().run_schedule(EnterMainMenu);
-        assert_eq!(app.world().entities().len(), 2);
+        assert_eq!(app.world().entities().count_constructed(), 2);
     }
 
     #[test]

crates/bevy_diagnostic/src/entity_count_diagnostics_plugin.rs

@@ -24,6 +24,6 @@ impl EntityCountDiagnosticsPlugin {
 
     /// Updates entity count measurement.
     pub fn diagnostic_system(mut diagnostics: Diagnostics, entities: &Entities) {
-        diagnostics.add_measurement(&Self::ENTITY_COUNT, || entities.len() as f64);
+        diagnostics.add_measurement(&Self::ENTITY_COUNT, || entities.count_constructed() as f64);
     }
 }

crates/bevy_ecs/src/bundle.rs

@@ -65,7 +65,7 @@ use crate::{
         Component, ComponentId, Components, ComponentsRegistrator, RequiredComponentConstructor,
         RequiredComponents, StorageType, Tick,
     },
-    entity::{Entities, Entity, EntityLocation},
+    entity::{Entities, EntitiesAllocator, Entity, EntityLocation},
     lifecycle::{ADD, INSERT, REMOVE, REPLACE},
     observer::Observers,
     prelude::World,
@@ -1248,9 +1248,9 @@ impl<'w> BundleInserter<'w> {
                 if let Some(swapped_entity) = result.swapped_entity {
                     let swapped_location =
                         // SAFETY: If the swap was successful, swapped_entity must be valid.
-                        unsafe { entities.get(swapped_entity).debug_checked_unwrap() };
-                    entities.set(
-                        swapped_entity.index(),
+                        unsafe { entities.get_constructed(swapped_entity).debug_checked_unwrap() };
+                    entities.update(
+                        swapped_entity.row(),
                         Some(EntityLocation {
                             archetype_id: swapped_location.archetype_id,
                             archetype_row: location.archetype_row,
@@ -1260,7 +1260,7 @@ impl<'w> BundleInserter<'w> {
                     );
                 }
                 let new_location = new_archetype.allocate(entity, result.table_row);
-                entities.set(entity.index(), Some(new_location));
+                entities.update(entity.row(), Some(new_location));
                 let after_effect = bundle_info.write_components(
                     table,
                     sparse_sets,
@@ -1297,9 +1297,9 @@ impl<'w> BundleInserter<'w> {
                 if let Some(swapped_entity) = result.swapped_entity {
                     let swapped_location =
                         // SAFETY: If the swap was successful, swapped_entity must be valid.
-                        unsafe { entities.get(swapped_entity).debug_checked_unwrap() };
-                    entities.set(
-                        swapped_entity.index(),
+                        unsafe { entities.get_constructed(swapped_entity).debug_checked_unwrap() };
+                    entities.update(
+                        swapped_entity.row(),
                         Some(EntityLocation {
                             archetype_id: swapped_location.archetype_id,
                             archetype_row: location.archetype_row,
@@ -1312,16 +1312,16 @@ impl<'w> BundleInserter<'w> {
                 // redundant copies
                 let move_result = table.move_to_superset_unchecked(result.table_row, new_table);
                 let new_location = new_archetype.allocate(entity, move_result.new_row);
-                entities.set(entity.index(), Some(new_location));
+                entities.update(entity.row(), Some(new_location));
 
                 // If an entity was moved into this entity's table spot, update its table row.
                 if let Some(swapped_entity) = move_result.swapped_entity {
                     let swapped_location =
                         // SAFETY: If the swap was successful, swapped_entity must be valid.
-                        unsafe { entities.get(swapped_entity).debug_checked_unwrap() };
+                        unsafe { entities.get_constructed(swapped_entity).debug_checked_unwrap() };
 
-                    entities.set(
-                        swapped_entity.index(),
+                    entities.update(
+                        swapped_entity.row(),
                         Some(EntityLocation {
                             archetype_id: swapped_location.archetype_id,
                             archetype_row: swapped_location.archetype_row,
@@ -1637,10 +1637,10 @@ impl<'w> BundleRemover<'w> {
             .swap_remove(location.archetype_row);
         // if an entity was moved into this entity's archetype row, update its archetype row
         if let Some(swapped_entity) = remove_result.swapped_entity {
-            let swapped_location = world.entities.get(swapped_entity).unwrap();
+            let swapped_location = world.entities.get_constructed(swapped_entity).unwrap();
 
-            world.entities.set(
-                swapped_entity.index(),
+            world.entities.update(
+                swapped_entity.row(),
                 Some(EntityLocation {
                     archetype_id: swapped_location.archetype_id,
                     archetype_row: location.archetype_row,
@@ -1678,10 +1678,10 @@ impl<'w> BundleRemover<'w> {
 
             // if an entity was moved into this entity's table row, update its table row
             if let Some(swapped_entity) = move_result.swapped_entity {
-                let swapped_location = world.entities.get(swapped_entity).unwrap();
+                let swapped_location = world.entities.get_constructed(swapped_entity).unwrap();
 
-                world.entities.set(
-                    swapped_entity.index(),
+                world.entities.update(
+                    swapped_entity.row(),
                     Some(EntityLocation {
                         archetype_id: swapped_location.archetype_id,
                         archetype_row: swapped_location.archetype_row,
@@ -1703,7 +1703,7 @@ impl<'w> BundleRemover<'w> {
 
         // SAFETY: The entity is valid and has been moved to the new location already.
         unsafe {
-            world.entities.set(entity.index(), Some(new_location));
+            world.entities.update(entity.row(), Some(new_location));
         }
 
         (new_location, pre_remove_result)
@@ -1778,10 +1778,14 @@ impl<'w> BundleSpawner<'w> {
     }
 
     /// # Safety
-    /// `entity` must be allocated (but non-existent), `T` must match this [`BundleInfo`]'s type
+    ///  `T` must match this [`BundleInfo`]'s type
+    ///
+    /// # Panics
+    ///
+    /// Panics if the entity has already been constructed.
     #[inline]
     #[track_caller]
-    pub unsafe fn spawn_non_existent<T: DynamicBundle>(
+    pub unsafe fn construct<T: DynamicBundle>(
         &mut self,
         entity: Entity,
         bundle: T,
@@ -1812,8 +1816,17 @@ impl<'w> BundleSpawner<'w> {
                 InsertMode::Replace,
                 caller,
             );
-            entities.set(entity.index(), Some(location));
-            entities.mark_spawn_despawn(entity.index(), caller, self.change_tick);
+
+            let was_at = entities.declare(entity.row(), Some(location));
+            // We need to assert here.
+            // Even if we can ensure that this entity is fresh from an allocator,
+            // it does not prevent users constructing arbitrary rows, which may overlap with the allocator.
+            // One alternative would be making `Entity` creation unsafe, but this is a good safety net anyway.
+            assert!(
+                was_at.is_none(),
+                "Constructing an {entity} that was already constructed is not allowed."
+            );
+            entities.mark_construct_or_destruct(entity.row(), caller, self.change_tick);
             (location, after_effect)
         };
 
@@ -1858,6 +1871,12 @@ impl<'w> BundleSpawner<'w> {
         (location, after_effect)
     }
 
+    #[inline]
+    pub fn allocator(&self) -> &'w EntitiesAllocator {
+        // SAFETY: No outstanding references to self.world, changes to allocator cannot invalidate our internal pointers
+        unsafe { &self.world.world().allocator }
+    }
+
     /// # Safety
     /// `T` must match this [`BundleInfo`]'s type
     #[inline]
@@ -1866,18 +1885,12 @@ impl<'w> BundleSpawner<'w> {
         bundle: T,
         caller: MaybeLocation,
     ) -> (Entity, T::Effect) {
-        let entity = self.entities().alloc();
+        let entity = self.allocator().alloc();
         // SAFETY: entity is allocated (but non-existent), `T` matches this BundleInfo's type
-        let (_, after_effect) = unsafe { self.spawn_non_existent(entity, bundle, caller) };
+        let (_, after_effect) = unsafe { self.construct(entity, bundle, caller) };
         (entity, after_effect)
     }
 
-    #[inline]
-    pub(crate) fn entities(&mut self) -> &mut Entities {
-        // SAFETY: No outstanding references to self.world, changes to entities cannot invalidate our internal pointers
-        unsafe { &mut self.world.world_mut().entities }
-    }
-
     /// # Safety
     /// - `Self` must be dropped after running this function as it may invalidate internal pointers.
     #[inline]

crates/bevy_ecs/src/entity/clone_entities.rs

@@ -1,19 +1,20 @@
-use alloc::{borrow::ToOwned, boxed::Box, collections::VecDeque, vec::Vec};
-use bevy_platform::collections::{HashMap, HashSet};
-use bevy_ptr::{Ptr, PtrMut};
-use bevy_utils::prelude::DebugName;
-use bumpalo::Bump;
-use core::any::TypeId;
-
 use crate::{
     archetype::Archetype,
     bundle::Bundle,
     component::{Component, ComponentCloneBehavior, ComponentCloneFn, ComponentId, ComponentInfo},
-    entity::{hash_map::EntityHashMap, Entities, Entity, EntityMapper},
+    entity::{hash_map::EntityHashMap, Entity, EntityMapper},
     query::DebugCheckedUnwrap,
     relationship::RelationshipHookMode,
     world::World,
 };
+use alloc::{borrow::ToOwned, boxed::Box, collections::VecDeque, vec::Vec};
+use bevy_platform::collections::{HashMap, HashSet};
+use bevy_ptr::{Ptr, PtrMut};
+use bevy_utils::prelude::DebugName;
+use bumpalo::Bump;
+use core::any::TypeId;
+
+use super::EntitiesAllocator;
 
 /// Provides read access to the source component (the component being cloned) in a [`ComponentCloneFn`].
 pub struct SourceComponent<'a> {
@@ -77,7 +78,7 @@ pub struct ComponentCloneCtx<'a, 'b> {
     target_component_written: bool,
     bundle_scratch: &'a mut BundleScratch<'b>,
     bundle_scratch_allocator: &'b Bump,
-    entities: &'a Entities,
+    allocator: &'a EntitiesAllocator,
     source: Entity,
     target: Entity,
     component_info: &'a ComponentInfo,
@@ -103,7 +104,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> {
         target: Entity,
         bundle_scratch_allocator: &'b Bump,
         bundle_scratch: &'a mut BundleScratch<'b>,
-        entities: &'a Entities,
+        allocator: &'a EntitiesAllocator,
         component_info: &'a ComponentInfo,
         entity_cloner: &'a mut EntityCloner,
         mapper: &'a mut dyn EntityMapper,
@@ -117,7 +118,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> {
             bundle_scratch,
             target_component_written: false,
             bundle_scratch_allocator,
-            entities,
+            allocator,
             mapper,
             component_info,
             entity_cloner,
@@ -270,7 +271,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> {
 
     /// Queues the `entity` to be cloned by the current [`EntityCloner`]
     pub fn queue_entity_clone(&mut self, entity: Entity) {
-        let target = self.entities.reserve_entity();
+        let target = self.allocator.alloc();
         self.mapper.set_mapped(entity, target);
         self.entity_cloner.clone_queue.push_back(entity);
     }
@@ -348,6 +349,8 @@ pub struct EntityCloner {
     move_components: bool,
     linked_cloning: bool,
     default_clone_fn: ComponentCloneFn,
+    /// Represents a queue of entities to clone.
+    /// These will have targets in the entity map, which will need to be constructed.
     clone_queue: VecDeque<Entity>,
     deferred_commands: VecDeque<Box<dyn FnOnce(&mut World, &mut dyn EntityMapper)>>,
 }
@@ -458,18 +461,24 @@ impl EntityCloner {
         relationship_hook_insert_mode: RelationshipHookMode,
     ) -> Entity {
         let target = mapper.get_mapped(source);
+        // The target may need to be constructed if it hasn't been already.
+        // If this fails, it either didn't need to be constructed (ok) or doesn't exist (caught better later).
+        let _ = world.construct_empty(target);
+
         // PERF: reusing allocated space across clones would be more efficient. Consider an allocation model similar to `Commands`.
         let bundle_scratch_allocator = Bump::new();
         let mut bundle_scratch: BundleScratch;
         {
             let world = world.as_unsafe_world_cell();
             let source_entity = world.get_entity(source).expect("Source entity must exist");
-            let target_archetype = (!self.filter_required.is_empty()).then(|| {
-                world
-                    .get_entity(target)
-                    .expect("Target entity must exist")
-                    .archetype()
-            });
+            let target_archetype = (!self.filter_required.is_empty())
+                .then(|| {
+                    world
+                        .get_entity(target)
+                        .expect("Target entity must exist")
+                        .archetype()
+                })
+                .flatten();
 
             #[cfg(feature = "bevy_reflect")]
             // SAFETY: we have unique access to `world`, nothing else accesses the registry at this moment, and we clone
@@ -482,7 +491,10 @@ impl EntityCloner {
             #[cfg(not(feature = "bevy_reflect"))]
             let app_registry = Option::<()>::None;
 
-            let archetype = source_entity.archetype();
+            let Some(archetype) = source_entity.archetype() else {
+                // If the source has no archetype, there is nothing to clone.
+                return target;
+            };
             bundle_scratch = BundleScratch::with_capacity(archetype.component_count());
 
             for component in archetype.components() {
@@ -523,7 +535,7 @@ impl EntityCloner {
                         target,
                         &bundle_scratch_allocator,
                         &mut bundle_scratch,
-                        world.entities(),
+                        world.entities_allocator(),
                         info,
                         self,
                         mapper,
@@ -535,16 +547,10 @@ impl EntityCloner {
             }
         }
 
-        world.flush();
-
         for deferred in self.deferred_commands.drain(..) {
             (deferred)(world, mapper);
         }
 
-        if !world.entities.contains(target) {
-            panic!("Target entity does not exist");
-        }
-
         if self.move_components {
             world
                 .entity_mut(source)