Make entity::index non max (#18704)

# Objective

There are two problems this aims to solve. 

First, `Entity::index` is currently a `u32`. That means there are
`u32::MAX + 1` possible entities. Not only is that awkward, but it also
make `Entity` allocation more difficult. I discovered this while working
on remote entity reservation, but even on main, `Entities` doesn't
handle the `u32::MAX + 1` entity very well. It can not be batch reserved
because that iterator uses exclusive ranges, which has a maximum upper
bound of `u32::MAX - 1`. In other words, having `u32::MAX` as a valid
index can be thought of as a bug right now. We either need to make that
invalid (this PR), which makes Entity allocation cleaner and makes
remote reservation easier (because the length only needs to be u32
instead of u64, which, in atomics is a big deal), or we need to take
another pass at `Entities` to make it handle the `u32::MAX` index
properly.

Second, `TableRow`, `ArchetypeRow` and `EntityIndex` (a type alias for
u32) all have `u32` as the underlying type. That means using these as
the index type in a `SparseSet` uses 64 bits for the sparse list because
it stores `Option<IndexType>`. By using `NonMaxU32` here, we cut the
memory of that list in half. To my knowledge, `EntityIndex` is the only
thing that would really benefit from this niche. `TableRow` and
`ArchetypeRow` I think are not stored in an `Option` in bulk. But if
they ever are, this would help. Additionally this ensures
`TableRow::INVALID` and `ArchetypeRow::INVALID` never conflict with an
actual row, which in a nice bonus.

As a related note, if we do components as entities where `ComponentId`
becomes `Entity`, the the `SparseSet<ComponentId>` will see a similar
memory improvement too.

## Solution

Create a new type `EntityRow` that wraps `NonMaxU32`, similar to
`TableRow` and `ArchetypeRow`.
Change `Entity::index` to this type.

## Downsides

`NonMax` is implemented as a `NonZero` with a binary inversion. That
means accessing and storing the value takes one more instruction. I
don't think that's a big deal, but it's worth mentioning.

As a consequence, `to_bits` uses `transmute` to skip the inversion which
keeps it a nop. But that also means that ordering has now flipped. In
other words, higher indices are considered less than lower indices. I
don't think that's a problem, but it's also worth mentioning.

## Alternatives

We could keep the index as a u32 type and just document that `u32::MAX`
is invalid, modifying `Entities` to ensure it never gets handed out.
(But that's not enforced by the type system.) We could still take
advantage of the niche here in `ComponentSparseSet`. We'd just need some
unsafe manual conversions, which is probably fine, but opens up the
possibility for correctness problems later.

We could change `Entities` to fully support the `u32::MAX` index. (But
that makes `Entities` more complex and potentially slightly slower.)

## Testing

- CI
- A few tests were changed because they depend on different ordering and
`to_bits` values.

## Future Work

- It might be worth removing the niche on `Entity::generation` since
there is now a different niche.
- We could move `Entity::generation` into it's own type too for clarity.
- We should change `ComponentSparseSet` to take advantage of the new
niche. (This PR doesn't change that yet.)
- Consider removing or updating `Identifier`. This is only used for
`Entity`, so it might be worth combining since `Entity` is now more
unique.

---------

Co-authored-by: atlv <email@atlasdostal.com>
Co-authored-by: Zachary Harrold <zac@harrold.com.au>

Author: 3 people

assets/scenes/load_scene_example.scn.ron

@@ -5,7 +5,7 @@
     ),
   },
   entities: {
-    4294967296: (
+    4294967297: (
       components: {
         "bevy_ecs::name::Name": "joe",
         "bevy_transform::components::global_transform::GlobalTransform": ((1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0)),
@@ -23,7 +23,7 @@
         ),
       },
     ),
-    4294967297: (
+    4294967298: (
       components: {
         "scene::ComponentA": (
           x: 3.0,
@@ -32,4 +32,4 @@
       },
     ),
   },
-)
+)

benches/Cargo.toml

@@ -32,6 +32,7 @@ bevy_platform = { path = "../crates/bevy_platform", default-features = false, fe
 glam = "0.29"
 rand = "0.8"
 rand_chacha = "0.3"
+nonmax = { version = "0.5", default-features = false }
 
 # Make `bevy_render` compile on Linux with x11 windowing. x11 vs. Wayland does not matter here
 # because the benches do not actually open any windows.

benches/benches/bevy_ecs/world/entity_hash.rs

@@ -17,9 +17,10 @@ fn make_entity(rng: &mut impl Rng, size: usize) -> Entity {
     let generation = 1.0 + -(1.0 - x).log2() * 2.0;
 
     // this is not reliable, but we're internal so a hack is ok
-    let bits = ((generation as u64) << 32) | (id as u64);
+    let id = id as u64 + 1;
+    let bits = ((generation as u64) << 32) | id;
     let e = Entity::from_bits(bits);
-    assert_eq!(e.index(), id as u32);
+    assert_eq!(e.index(), !(id as u32));
     assert_eq!(e.generation(), generation as u32);
     e
 }

benches/benches/bevy_ecs/world/world_get.rs

@@ -1,9 +1,10 @@
 use core::hint::black_box;
+use nonmax::NonMaxU32;
 
 use bevy_ecs::{
     bundle::{Bundle, NoBundleEffect},
     component::Component,
-    entity::Entity,
+    entity::{Entity, EntityRow},
     system::{Query, SystemState},
     world::World,
 };
@@ -53,7 +54,9 @@ pub fn world_entity(criterion: &mut Criterion) {
 
             bencher.iter(|| {
                 for i in 0..entity_count {
-                    let entity = Entity::from_raw(i);
+                    let entity =
+                        // SAFETY: Range is exclusive.
+                        Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) }));
                     black_box(world.entity(entity));
                 }
             });
@@ -74,7 +77,9 @@ pub fn world_get(criterion: &mut Criterion) {
 
             bencher.iter(|| {
                 for i in 0..entity_count {
-                    let entity = Entity::from_raw(i);
+                    let entity =
+                        // SAFETY: Range is exclusive.
+                        Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) }));
                     assert!(world.get::<Table>(entity).is_some());
                 }
             });
@@ -84,7 +89,9 @@ pub fn world_get(criterion: &mut Criterion) {
 
             bencher.iter(|| {
                 for i in 0..entity_count {
-                    let entity = Entity::from_raw(i);
+                    let entity =
+                        // SAFETY: Range is exclusive.
+                        Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) }));
                     assert!(world.get::<Sparse>(entity).is_some());
                 }
             });
@@ -106,7 +113,9 @@ pub fn world_query_get(criterion: &mut Criterion) {
 
             bencher.iter(|| {
                 for i in 0..entity_count {
-                    let entity = Entity::from_raw(i);
+                    let entity =
+                        // SAFETY: Range is exclusive.
+                        Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) }));
                     assert!(query.get(&world, entity).is_ok());
                 }
             });
@@ -131,7 +140,9 @@ pub fn world_query_get(criterion: &mut Criterion) {
 
             bencher.iter(|| {
                 for i in 0..entity_count {
-                    let entity = Entity::from_raw(i);
+                    let entity =
+                        // SAFETY: Range is exclusive.
+                        Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) }));
                     assert!(query.get(&world, entity).is_ok());
                 }
             });
@@ -142,7 +153,9 @@ pub fn world_query_get(criterion: &mut Criterion) {
 
             bencher.iter(|| {
                 for i in 0..entity_count {
-                    let entity = Entity::from_raw(i);
+                    let entity =
+                        // SAFETY: Range is exclusive.
+                        Entity::from_raw(EntityRow::new(unsafe { NonMaxU32::new_unchecked(i) }));
                     assert!(query.get(&world, entity).is_ok());
                 }
             });
@@ -169,7 +182,10 @@ pub fn world_query_get(criterion: &mut Criterion) {
 
                 bencher.iter(|| {
                     for i in 0..entity_count {
-                        let entity = Entity::from_raw(i);
+                        // SAFETY: Range is exclusive.
+                        let entity = Entity::from_raw(EntityRow::new(unsafe {
+                            NonMaxU32::new_unchecked(i)
+                        }));
                         assert!(query.get(&world, entity).is_ok());
                     }
                 });

crates/bevy_ecs/src/entity/map_entities.rs

@@ -171,7 +171,7 @@ impl<T: MapEntities, A: smallvec::Array<Item = T>> MapEntities for SmallVec<A> {
 ///     fn get_mapped(&mut self, entity: Entity) -> Entity {
 ///         self.map.get(&entity).copied().unwrap_or(entity)
 ///     }
-///     
+///
 ///     fn set_mapped(&mut self, source: Entity, target: Entity) {
 ///         self.map.insert(source, target);
 ///     }
@@ -228,7 +228,7 @@ impl EntityMapper for SceneEntityMapper<'_> {
 
         // this new entity reference is specifically designed to never represent any living entity
         let new = Entity::from_raw_and_generation(
-            self.dead_start.index(),
+            self.dead_start.row(),
             IdentifierMask::inc_masked_high_by(self.dead_start.generation, self.generations),
         );
 
@@ -331,21 +331,19 @@ impl<'m> SceneEntityMapper<'m> {
 
 #[cfg(test)]
 mod tests {
+
     use crate::{
         entity::{Entity, EntityHashMap, EntityMapper, SceneEntityMapper},
         world::World,
     };
 
     #[test]
     fn entity_mapper() {
-        const FIRST_IDX: u32 = 1;
-        const SECOND_IDX: u32 = 2;
-
         let mut map = EntityHashMap::default();
         let mut world = World::new();
         let mut mapper = SceneEntityMapper::new(&mut map, &mut world);
 
-        let mapped_ent = Entity::from_raw(FIRST_IDX);
+        let mapped_ent = Entity::from_raw_u32(1).unwrap();
         let dead_ref = mapper.get_mapped(mapped_ent);
 
         assert_eq!(
@@ -354,7 +352,7 @@ mod tests {
             "should persist the allocated mapping from the previous line"
         );
         assert_eq!(
-            mapper.get_mapped(Entity::from_raw(SECOND_IDX)).index(),
+            mapper.get_mapped(Entity::from_raw_u32(2).unwrap()).index(),
             dead_ref.index(),
             "should re-use the same index for further dead refs"
         );
@@ -372,7 +370,7 @@ mod tests {
         let mut world = World::new();
 
         let dead_ref = SceneEntityMapper::world_scope(&mut map, &mut world, |_, mapper| {
-            mapper.get_mapped(Entity::from_raw(0))
+            mapper.get_mapped(Entity::from_raw_u32(0).unwrap())
         });
 
         // Next allocated entity should be a further generation on the same index