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

Minimize small allocations by dropping the tick Vecs from Resources #11226

Merged

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Jan 5, 2024

Objective

Column unconditionally requires three separate allocations: one for the data, and two for the tick Vecs. The tick Vecs aren't really needed for Resources, so we're allocating a bunch of one-element Vecs, and it costs two extra dereferences when fetching/inserting/removing resources.

Solution

Drop one level lower in ResourceData and directly store a BlobVec and two UnsafeCell<Tick>s. This should significantly shrink ResourceData (exchanging 6 usizes for 2 u32s), removes the need to dereference two separate ticks when inserting/removing/fetching resources, and can significantly decrease the number of small allocations the ECS makes by default.

This tentatively might have a non-insignificant impact on the CPU cost for rendering since we're constantly fetching resources in draw functions, depending on how aggressively inlined the functions are.

This requires reimplementing some of the unsafe functions that Column wraps, but it also allows us to delete a few Column APIs that were only used for Resources, so the total amount of unsafe we're maintaining shouldn't change significantly.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Jan 5, 2024
@james7132 james7132 changed the title Lower small allocations by dropping the tick Vecs from Resources Minimize small allocations by dropping the tick Vecs from Resources Jan 5, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This change makes sense to me, the unsafe is straightforward, and the docs are good. Curious to see if this measurably changes performance.

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Nice, this could be surprisingly impactful.

crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile 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 Jan 7, 2024
james7132 and others added 2 commits January 7, 2024 06:23
Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com>
@james7132
Copy link
Member Author

I'll run a full trace on these later to see if there's any tangible impact on rendering, but I ran a quick sanity check on the assembly output, and the removal of a bunch of allocation related machinery in resource insertion seems promising: james7132/bevy_asm_tests@main...11226-minimize-resource-allocations#diff-f4d513f091332848b18110a4e97729c2d725fef55a135820bc0a0e8cc56b88a7

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 8, 2024
Merged via the queue into bevyengine:main with commit 13570cd Jan 8, 2024
22 checks passed
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 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.

4 participants