Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundent information and optimize dynamic allocations in Table #12929

Open
wants to merge 64 commits into
base: main
Choose a base branch
from

Conversation

Adamkob12
Copy link
Contributor

@Adamkob12 Adamkob12 commented Apr 11, 2024

Objective

Solution

The PR consists of multiple steps:

  1. For the component data: create a new data-structure that's similar to BlobVec but doesn't store len & capacity inside of it: "BlobArray" (name suggestions welcome)
  2. For the Tick data: create a new data-structure that's similar to ThinSlicePtr but supports dynamic reallocation: "ThinArrayPtr" (name suggestions welcome)
  3. Create a new data-structure that's very similar to Column that doesn't store len & capacity inside of it: "ThinColumn"
  4. Adjust the Table implementation to use ThinColumn instead of Column

The result is that only one set of len & capacity is stored in Table, in Table::entities

Notes Regarding Performance

Apart from shaving off some excess memory in Table, the changes have also brought noteworthy performance improvements:
The previous implementation relied on Vec::reserve & BlobVec::reserve, but that redundantly repeated the same if statement (capacity == len). Now that check could be made at the Table level because the capacity and length of all the columns are synchronized; saving N branches per allocation. The result is a respectable performance improvement per every Table::reserve (and subsequently Table::allocate) call.

I'm hesitant to give exact numbers because I don't have a lot of experience in profiling and benchmarking, but these are the results I got so far:

add_remove_big/table benchmark after the implementation:

after_add_remove_big_table

add_remove_big/table benchmark in main branch (measured in comparison to the implementation):

main_add_remove_big_table

add_remove_very_big/table benchmark after the implementation:

after_add_remove_very_big

add_remove_very_big/table benchmark in main branch (measured in comparison to the implementation):

main_add_remove_very_big

cc @james7132 to verify


Changelog

  • New data-structure that's similar to BlobVec but doesn't store len & capacity inside of it: BlobArray
  • New data-structure that's similar to ThinSlicePtr but supports dynamic allocation:ThinArrayPtr
  • New data-structure that's very similar to Column that doesn't store len & capacity inside of it: ThinColumn
  • Adjust the Table implementation to use ThinColumn instead of Column
  • New benchmark: add_remove_very_big to benchmark the performance of spawning a lot of entities with a lot of components (15) each

Migration Guide

Table now uses ThinColumn instead of Column. That means that methods that previously returned Column, will now return ThinColumn instead.

ThinColumn has a much more limited and low-level API, but you can still achieve the same things in ThinColumn as you did in Column. For example, instead of calling Column::get_added_tick, you'd call ThinColumn::get_added_ticks_slice and index it to get the specific added tick.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix CI, looks like clippy still has the last word, haha. Great job getting this to pass miri!

Only a partial review. Will finish later.

crates/bevy_ecs/src/world/unsafe_world_cell.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/filter.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/filter.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/blob_vec.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/thin_array_ptr.rs Show resolved Hide resolved
crates/bevy_ecs/src/storage/thin_array_ptr.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/thin_array_ptr.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/thin_array_ptr.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/thin_array_ptr.rs Outdated Show resolved Hide resolved
@Adamkob12
Copy link
Contributor Author

Adamkob12 commented Apr 11, 2024

looks like a segfault in run-examples-macos-metal ci? needs investigation
e: fixed

@vertesians
Copy link
Contributor

vertesians commented Apr 11, 2024

For the component data: create a new data-structure that's similar to BlobVec but doesn't store len & capacity inside of it: "BlobArray" (name suggestions welcome) (This type makes a compile-time destinction between ZST types and non-ZST types)

I like UntrackedBlobVec for this concept -- "thin" in Rust contexts typically refers to pointer sizes, but the data structures aren't pointers. And IMO, "untracked" is clearer about the intention. By the same logic, I like UntrackedVec and UntrackedColumn over ThinArrayPtr and ThinColumn (assuming ThinArrayPtr is implemented like a Vec).

@BD103 BD103 added the A-ECS Entities, components, systems, and events label Apr 11, 2024
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to follow this up with a PR for applying ThinColumn for both resources and sparse sets.

crates/bevy_ecs/src/storage/table/mod.rs Show resolved Hide resolved
crates/bevy_ecs/src/storage/table/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/table/mod.rs Outdated Show resolved Hide resolved
@cBournhonesque cBournhonesque added D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Unsafe Touches with unsafe code in some way labels May 12, 2024
Copy link
Contributor

@james-j-obrien james-j-obrien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, it's a lot of code but most of it is fairly straightforward and mirrors the other container implementations pretty closely.

I've left some nitpicks that I don't see as blocking, feel free to ignore them.

/// increased. For better unwind safety, call [`BlobVec::set_len`] _after_ populating a new
/// value.
#[inline]
pub unsafe fn set_len(&mut self, len: usize) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the getters/setters?
The fields are private so it's not a big deal, but we lose the ability to annotate safety and so won't get clippy lints encouraging safety comments when people touch the fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The primary thing is that they're dead code. They're no longer used in any form as these types are entirely internal. We'd need to add some allow(dead_code) annotations.

crates/bevy_ecs/src/storage/blob_array.rs Outdated Show resolved Hide resolved
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 13, 2024
@james7132 james7132 added this to the 0.15 milestone May 18, 2024
@james7132
Copy link
Member

Due to the amount of unsafe in this PR, merging now is a potential risk for 0.14, moving this to merge early on in the 0.15 cycle.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Unsafe Touches with unsafe code in some way S-Blocked This cannot move forward until something else changes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table redundantly stores 3 lengths and capacities for each column
8 participants