Skip to content

Fix sprite performance regression since retained render world#17078

Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom
superdump:fix-sprite-regression
Jan 1, 2025
Merged

Fix sprite performance regression since retained render world#17078
alice-i-cecile merged 2 commits intobevyengine:mainfrom
superdump:fix-sprite-regression

Conversation

@superdump
Copy link
Copy Markdown
Contributor

@superdump superdump commented Jan 1, 2025 •
edited
Loading

Objective

  • Fix sprite rendering performance regression since retained render world changes
    • The retained render world changes moved ExtractedSprites from using the highly-optimised EntityHasher with an Entity to using FixedHasher with (Entity, MainEntity). This was enough to regress framerate in bevymark by 25%.

Solution

  • Move the render world entity into a member of ExtractedSprite and change ExtractedSprites to use MainEntityHashMap for its storage
  • Disable sprite picking in bevymark

Testing

M4 Max. bevymark --waves 100 --per-wave 1000 --benchmark. main in yellow vs PR in red:

Screenshot 2025-01-01 at 16 36 22

20.2% median frame time reduction.

Screenshot 2025-01-01 at 16 38 37

49.7% median extract_sprites execution time reduction.

Comparing 0.14.2 yellow vs PR red:
Screenshot 2025-01-01 at 16 40 06

~6.1% median frame time reduction.

Migration Guide

  • ExtractedSprites is now using MainEntityHashMap for storage, which is keyed on MainEntity.
  • The render world entity corresponding to an ExtractedSprite is now stored in the render_entity member of it.

@superdump
Fix sprite performance regression
42e04fc 
As part of the retained render world changes, ExtractedSprites moved to using a
`HashMap<(Entity, MainEntity), ExtractedSprite>` for its storage. The hashing
of `(Entity, MainEntity)` using `FixedHash` was the source of the regression
and performs much much slower than the highly-optimized `EntityHashMap`.

This change moves the render world entity into a member in `ExtractedSprite`
and then uses `MainEntityHashMap` for `ExtractedSprites` storage.
@superdump superdump changed the title Fix sprite regression Fix sprite performance regression since retained render world Jan 1, 2025
@superdump superdump requested a review from tychedelia January 1, 2025 15:44
@superdump
Copy link
Copy Markdown
Contributor Author

superdump commented Jan 1, 2025 •
edited
Loading

I'm not totally sure this is correct. I don't see why it wouldn't be, but I am unfamiliar with all the ins and outs of the retained render world changes. As such, I would love if @tychedelia could review this. :)

@superdump superdump force-pushed the fix-sprite-regression branch from 1147b26 to 8335a73 Compare January 1, 2025 15:50
tychedelia
tychedelia approved these changes Jan 1, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Thanks @superdump, this fix looks exactly right. We shouldn't ever need to key off the entity pair together. Our policy has pretty much been to make sure we key all collections to MainEntity.

FYI: The reason for the (RenderEntity, MainEntity) shenanigans in the render phase is because the RenderCommand api supports looking up components on the PhaseItem via RenderCommand::ItemQuery, which needs the render world id to function. So while our internal render code almost exclusively looks up things via a MainEntity keyed collection on RenderCommand::Param, user code may not. I believe the exception here in our own code is some UI stuff. It's a bit of a mess!

@tychedelia tychedelia added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 1, 2025
@tychedelia tychedelia added this to the 0.15.2 milestone Jan 1, 2025
@tychedelia tychedelia removed the C-Bug An unexpected or incorrect behavior label Jan 1, 2025
@tychedelia
Copy link
Copy Markdown
Member

tychedelia commented Jan 1, 2025

I added this to 0.15.2 although I realize now it technically has a breaking change in ExtractedSprites. The performance regression is probably not extreme in non-benchmark use cases.

@tychedelia tychedelia modified the milestones: 0.15.2, 0.16 Jan 1, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 1, 2025
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Jan 1, 2025

Very neat to see that we have a net performance gain here!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 1, 2025
Merged via the queue into bevyengine:main with commit fd330c8 Jan 1, 2025
@rparrett rparrett mentioned this pull request Jan 2, 2025
Closed
rparrett added a commit to rparrett/bevy that referenced this pull request Jan 3, 2025
@rparrett
Revert "Fix sprite performance regression since retained render world (…
487a3be 
…bevyengine#17078)"

This reverts commit fd330c8.
@rparrett rparrett mentioned this pull request Jan 3, 2025
Merged
@tychedelia tychedelia mentioned this pull request Jan 3, 2025
Closed
github-merge-queue bot pushed a commit that referenced this pull request Jan 4, 2025
@rparrett
Revert "Fix sprite performance regression since retained render world (…
859c2d7 
…#17078)" (#17123)

# Objective

Fixes #17098

It seems that it's not totally obvious how to fix this, but that
reverting might be part of the solution anyway.

Let's get the repo back into a working state.

## Solution

Revert the [recent
optimization](#17078) that broke
"many-to-one main->render world entities" for 2d.

## Testing

`cargo run --example text2d`
`cargo run --example sprite_slice`
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@superdump @ecoskey
Fix sprite performance regression since retained render world (bevyen…
ab086c5 
…gine#17078)

# Objective

- Fix sprite rendering performance regression since retained render
world changes
- The retained render world changes moved `ExtractedSprites` from using
the highly-optimised `EntityHasher` with an `Entity` to using
`FixedHasher` with `(Entity, MainEntity)`. This was enough to regress
framerate in bevymark by 25%.

## Solution

- Move the render world entity into a member of `ExtractedSprite` and
change `ExtractedSprites` to use `MainEntityHashMap` for its storage
- Disable sprite picking in bevymark

## Testing

M4 Max. `bevymark --waves 100 --per-wave 1000 --benchmark`. main in
yellow vs PR in red:

<img width="590" alt="Screenshot 2025-01-01 at 16 36 22"
src="https://github.com/user-attachments/assets/1e4ed6ec-3811-4abf-8b30-336153737f89"
/>

20.2% median frame time reduction.

<img width="594" alt="Screenshot 2025-01-01 at 16 38 37"
src="https://github.com/user-attachments/assets/157c2022-cda6-4cf2-bc63-d0bc40528cf0"
/>

49.7% median extract_sprites execution time reduction.

Comparing 0.14.2 yellow vs PR red:
<img width="593" alt="Screenshot 2025-01-01 at 16 40 06"
src="https://github.com/user-attachments/assets/abd59b6f-290a-4eb6-8835-ed110af995f3"
/>

~6.1% median frame time reduction.

---

## Migration Guide

- `ExtractedSprites` is now using `MainEntityHashMap` for storage, which
is keyed on `MainEntity`.
- The render world entity corresponding to an `ExtractedSprite` is now
stored in the `render_entity` member of it.
ickshonpe added a commit to ickshonpe/bevy that referenced this pull request Jan 11, 2025
@ickshonpe
Adds the reverted changes from bevyengine#17078 fix-sprite-regression…
a352810 
… and use a `MainEntityHashMap` for storage in `ExtractedSprites`
@ickshonpe ickshonpe mentioned this pull request Jan 11, 2025
Merged
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@superdump @mrchantey
Fix sprite performance regression since retained render world (bevyen…
1e35e52 
…gine#17078)

# Objective

- Fix sprite rendering performance regression since retained render
world changes
- The retained render world changes moved `ExtractedSprites` from using
the highly-optimised `EntityHasher` with an `Entity` to using
`FixedHasher` with `(Entity, MainEntity)`. This was enough to regress
framerate in bevymark by 25%.

## Solution

- Move the render world entity into a member of `ExtractedSprite` and
change `ExtractedSprites` to use `MainEntityHashMap` for its storage
- Disable sprite picking in bevymark

## Testing

M4 Max. `bevymark --waves 100 --per-wave 1000 --benchmark`. main in
yellow vs PR in red:

<img width="590" alt="Screenshot 2025-01-01 at 16 36 22"
src="https://github.com/user-attachments/assets/1e4ed6ec-3811-4abf-8b30-336153737f89"
/>

20.2% median frame time reduction.

<img width="594" alt="Screenshot 2025-01-01 at 16 38 37"
src="https://github.com/user-attachments/assets/157c2022-cda6-4cf2-bc63-d0bc40528cf0"
/>

49.7% median extract_sprites execution time reduction.

Comparing 0.14.2 yellow vs PR red:
<img width="593" alt="Screenshot 2025-01-01 at 16 40 06"
src="https://github.com/user-attachments/assets/abd59b6f-290a-4eb6-8835-ed110af995f3"
/>

~6.1% median frame time reduction.

---

## Migration Guide

- `ExtractedSprites` is now using `MainEntityHashMap` for storage, which
is keyed on `MainEntity`.
- The render world entity corresponding to an `ExtractedSprite` is now
stored in the `render_entity` member of it.
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@rparrett @mrchantey
Revert "Fix sprite performance regression since retained render world (…
f0252f3 
…bevyengine#17078)" (bevyengine#17123)

# Objective

Fixes bevyengine#17098

It seems that it's not totally obvious how to fix this, but that
reverting might be part of the solution anyway.

Let's get the repo back into a working state.

## Solution

Revert the [recent
optimization](bevyengine#17078) that broke
"many-to-one main->render world entities" for 2d.

## Testing

`cargo run --example text2d`
`cargo run --example sprite_slice`
@alice-i-cecile alice-i-cecile mentioned this pull request Apr 2, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+1 more reviewer

@tychedelia tychedelia tychedelia approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen 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

No open projects
Rendering (Old)
Status: Done

Milestone

0.16

Development

Successfully merging this pull request may close these issues.

3 participants

@superdump @tychedelia @alice-i-cecile