From 022c6b1d34e05ce9e4d7066363bfc32d1ee6f254 Mon Sep 17 00:00:00 2001
From: Oliver Maskery <omaskery@googlemail.com>
Date: Sun, 22 Dec 2024 23:04:32 +0000
Subject: [PATCH] Prevent creation of superfluous empty table (#16935)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

# Objective

- To fix a tiny bug in `bevy_ecs::storage::Tables` that, in one case,
means it accidentally allocates an additional "empty" `Table`, resulting
in two "empty" `Table`s:
- The one pre-allocated empty table at index 0 whose index is designed
to match up with `TableId::empty()`
- One extra empty table, at some non-0 index, that does not match up
with `TableId::empty()`.
- This PR aims to prevent this extraneous `Table`, ensuring that
entities with no components in table-storage reliably have their
archetype's table ID be equal to `TableId::empty()`.

## Solution

### Background

The issue occurs because:

- `Tables` contains:
- `tables: Vec<Table>` - The set of all `Table`s allocated in the world.
- `table_ids: HashMap<Box<[ComponentId]>, TableId>` - An index to
rapidly lookup the `Table` in `tables` by a set of `ComponentId`s.
- When `Tables` is constructed it pre-populates the `tables` `Vec` with
an empty `Table`.
- This ensures that the first entry (index 0) is always the `Table` for
entities with no components in table storage.
- In particular, `TableId::empty()` is a utility that returns a
`TableId` of `0`.
- However, the `table_ids` map is not initialised to associate an empty
`[ComponentId]` with `TableId` `0`.
- This means, the first time a structural change tries to access a
`Table` for an archetype with 0 table components:
  - `Tables::get_id_or_insert` is used to retrieve the target `Table`
- The function attempts to lookup the entry in the `table_ids` `HashMap`
whose key is the empty `ComponentId` set
- The empty `Table` created at startup won't be found, because it was
never inserted into `table_ids`
- It will instead create a new table, insert it into the `HashMap`
(preventing further instances of this issue), and return it.

### Changes

- I considered simply initialising the `table_ids` `HashMap` to know
about the pre-allocated `Table`
- However, I ended up using the proposed solution discussed on Discord
[#ecs-dev](https://discord.com/channels/691052431525675048/749335865876021248/1320430933152759958):
- Make `Tables::get_id_or_insert` simply early-exit if the requested
`component_ids` was empty.
- This avoids unnecessarily hashing the empty slice and looking it up in
the `HashMap`.
- The `table_ids` `HashMap` is not exposed outside this struct, and is
only used within `get_id_or_insert`, so it seems wasteful to defensively
populate it with the empty `Table`.

## Testing

This is my first Bevy contribution, so I don't really know the processes
that well. That said:
- I have introduced a little test that exercises the original issue and
shows that it is now resolved.
- I have run the `bevy_ecs` tests locally, so I have reasonable
confidence I haven't broken that.
- I haven't run any further test suites, mostly as when I tried to run
test suites for the whole project it filled my entire SSD with >600GB of
target directory output 😱😱😱
---
 crates/bevy_ecs/src/storage/table/mod.rs | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/crates/bevy_ecs/src/storage/table/mod.rs b/crates/bevy_ecs/src/storage/table/mod.rs
index 1f82642c07..ec77645eac 100644
--- a/crates/bevy_ecs/src/storage/table/mod.rs
+++ b/crates/bevy_ecs/src/storage/table/mod.rs
@@ -743,6 +743,10 @@ impl Tables {
         component_ids: &[ComponentId],
         components: &Components,
     ) -> TableId {
+        if component_ids.is_empty() {
+            return TableId::empty();
+        }
+
         let tables = &mut self.tables;
         let (_key, value) = self
             .table_ids
@@ -816,7 +820,7 @@ mod tests {
         component::{Component, Components, Tick},
         entity::Entity,
         ptr::OwningPtr,
-        storage::{Storages, TableBuilder, TableRow},
+        storage::{Storages, TableBuilder, TableId, TableRow, Tables},
     };
     #[cfg(feature = "track_change_detection")]
     use core::panic::Location;
@@ -824,6 +828,18 @@ mod tests {
     #[derive(Component)]
     struct W<T>(T);
 
+    #[test]
+    fn only_one_empty_table() {
+        let components = Components::default();
+        let mut tables = Tables::default();
+
+        let component_ids = &[];
+        // SAFETY: component_ids is empty, so we know it cannot reference invalid component IDs
+        let table_id = unsafe { tables.get_id_or_insert(component_ids, &components) };
+
+        assert_eq!(table_id, TableId::empty());
+    }
+
     #[test]
     fn table() {
         let mut components = Components::default();