Skip to content

Commit

Permalink
Prevent creation of superfluous empty table (#16935)
Browse files Browse the repository at this point in the history
# 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 😱😱😱
  • Loading branch information
omaskery authored Dec 22, 2024
1 parent 6a4e0c8 commit 022c6b1
Showing 1 changed file with 17 additions and 1 deletion.
18 changes: 17 additions & 1 deletion crates/bevy_ecs/src/storage/table/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -816,14 +820,26 @@ 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;

#[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();
Expand Down

0 comments on commit 022c6b1

Please sign in to comment.