Skip to content

Commit 022c6b1

Browse files
authored
Prevent creation of superfluous empty table (#16935)
# 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 😱😱😱
1 parent 6a4e0c8 commit 022c6b1

File tree

1 file changed

+17
-1
lines changed
  • crates/bevy_ecs/src/storage/table

1 file changed

+17
-1
lines changed

crates/bevy_ecs/src/storage/table/mod.rs

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -743,6 +743,10 @@ impl Tables {
743743
component_ids: &[ComponentId],
744744
components: &Components,
745745
) -> TableId {
746+
if component_ids.is_empty() {
747+
return TableId::empty();
748+
}
749+
746750
let tables = &mut self.tables;
747751
let (_key, value) = self
748752
.table_ids
@@ -816,14 +820,26 @@ mod tests {
816820
component::{Component, Components, Tick},
817821
entity::Entity,
818822
ptr::OwningPtr,
819-
storage::{Storages, TableBuilder, TableRow},
823+
storage::{Storages, TableBuilder, TableId, TableRow, Tables},
820824
};
821825
#[cfg(feature = "track_change_detection")]
822826
use core::panic::Location;
823827

824828
#[derive(Component)]
825829
struct W<T>(T);
826830

831+
#[test]
832+
fn only_one_empty_table() {
833+
let components = Components::default();
834+
let mut tables = Tables::default();
835+
836+
let component_ids = &[];
837+
// SAFETY: component_ids is empty, so we know it cannot reference invalid component IDs
838+
let table_id = unsafe { tables.get_id_or_insert(component_ids, &components) };
839+
840+
assert_eq!(table_id, TableId::empty());
841+
}
842+
827843
#[test]
828844
fn table() {
829845
let mut components = Components::default();

0 commit comments

Comments
 (0)