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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Optimize rendering slow-down at high entity counts #5509

Closed
wants to merge 13 commits into from
Closed

[Merged by Bors] - Optimize rendering slow-down at high entity counts #5509

wants to merge 13 commits into from

Conversation

TheRawMeatball
Copy link
Member

@TheRawMeatball TheRawMeatball commented Jul 31, 2022 •
edited

Objective

Solution

  • The very specific circumstances under which the render world is reset meant that the flush_as_invalid function could be replaced with one that had a noop as its init method.
  • This removes a double-writing issue leading to greatly increased performance.

Running the reproduction code in the linked issue, this change nearly doubles the framerate.

@TheRawMeatball TheRawMeatball added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times labels Jul 31, 2022
@superdump superdump requested a review from cart July 31, 2022 14:34
@rparrett
Copy link
Contributor

rparrett commented Jul 31, 2022 •
edited

Neat. This is the most dramatic one of these I've seen.

just the reserve_and_flush span (repro from issue)
image

@cart cart added this to the Bevy 0.8.1 milestone Aug 1, 2022
@cart
Copy link
Member

cart commented Aug 1, 2022

Wow yeah thats a big win right there. Pretty sure this is valid. I'll give this a proper think asap.

@TheRawMeatball
Copy link
Member Author

Changed again to a fully custom flush method that directly uses memset, this is getting even larger gains.

@TheRawMeatball
Copy link
Member Author

Currently UB, working on a fix.

@TheRawMeatball
Copy link
Member Author

Reverted the memset, so this works again, but others investigating why the memset version failed via UB would be appreciated.

@TheRawMeatball
Copy link
Member Author

UB fixed with help from @PROMETHIA-27

@rparrett
Copy link
Contributor

rparrett commented Aug 1, 2022 •
edited

b02a1d7 (repro from issue)
just the reserve_and_flush span
Screen Shot 2022-08-01 at 6 41 39 AM

@rparrett
Copy link
Contributor

rparrett commented Aug 1, 2022 •
edited

many_sprites frame time
image

many_spites frame time (run 2)
image

many_cubes sphere frame time
image

In all cases, yellow / this trace is this PR, red is main. Guessing we're just in the noise a bit and this is not a quiet machine at the moment. All traces were ~30 seconds though.

mockersf
mockersf reviewed Aug 1, 2022
View reviewed changes
examples/hello_world.rs Outdated Show resolved Hide resolved
cart
cart reviewed Aug 8, 2022
View reviewed changes
crates/bevy_ecs/src/archetype.rs
@@ -14,6 +14,7 @@ use std::{
};

#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
#[repr(transparent)]
Copy link
Member

Choose a reason for hiding this comment

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

Probably makes sense to add a comment to ArchetypeId::INVALID saying that it must be equivalent to setting all bytes of memory to u8::MAX (and referencing flush_and_reserve_invalid). Don't want anyone changing this here and then implicitly breaking flush_and_reserve_invalid.

@StarArawn StarArawn mentioned this pull request Aug 11, 2022
Closed
@StarArawn
Copy link
Contributor

StarArawn commented Aug 11, 2022 •
edited

I'm excited for this PR as it seems like it fixes part of the performance issues here! I'm still worried that this isn't a complete fix though. This code is still reserving entities even if they have been despawned which leads to memory leakage(increased memory usage over time) and possibly over time slow performance. I can think of quite a few scenarios where users would spawn high entity counts and despawn said entities. A shmup would be one such example where you would be spawning hundreds or even thousands of bullets which get depsawned.
Looking at it closer it looks like it just reserves the number of entities that currently exist. I still think performance could be an issue for me, but I think a better solution might be to have a separate world.

@cart cart removed this from the Bevy 0.8.1 milestone Aug 18, 2022
inodentry pushed a commit to BroovyJammy/bevy that referenced this pull request Aug 19, 2022
@TheRawMeatball @inodentry
PR bevyengine#5509
3df47e1
inodentry added a commit to BroovyJammy/bevy that referenced this pull request Aug 19, 2022
@inodentry
Merge PR bevyengine#5509
a76290d
@mbolt35
Copy link

mbolt35 commented Aug 27, 2022

Just checking on the general status of this PR. I noticed it was removed from the 0.8.1 release. Curious as to when we might expect this to be part of mainline. Thanks!

@alice-i-cecile alice-i-cecile added P-High This is particularly urgent, and deserves immediate attention X-Controversial There is active debate or serious implications around merging this PR labels Sep 3, 2022
@hankjordan
Copy link
Contributor

Noticeable performance improvement for my game as well - uses extremely high entity count, yellow is this PR, red is 0.8.0:
bench

@terhechte terhechte mentioned this pull request Sep 20, 2022
Closed
@cart cart added this to the Bevy 0.9 milestone Sep 22, 2022
@cart
Copy link
Member

cart commented Oct 24, 2022

bors r+

bors bot pushed a commit that referenced this pull request Oct 24, 2022
@TheRawMeatball @cart
Optimize rendering slow-down at high entity counts (#5509)
3c13c75 
# Objective

- Improve #3953

## Solution

- The very specific circumstances under which the render world is reset meant that the flush_as_invalid function could be replaced with one that had a noop as its init method.
- This removes a double-writing issue leading to greatly increased performance.

Running the reproduction code in the linked issue, this change nearly doubles the framerate.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 24, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Optimize rendering slow-down at high entity counts [Merged by Bors] - Optimize rendering slow-down at high entity counts Oct 24, 2022
@bors bors bot closed this Oct 24, 2022
@JMS55 JMS55 mentioned this pull request Oct 26, 2022
Closed
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@TheRawMeatball @cart @james7132
Optimize rendering slow-down at high entity counts (bevyengine#5509)
9a24592 
# Objective

- Improve bevyengine#3953

## Solution

- The very specific circumstances under which the render world is reset meant that the flush_as_invalid function could be replaced with one that had a noop as its init method.
- This removes a double-writing issue leading to greatly increased performance.

Running the reproduction code in the linked issue, this change nearly doubles the framerate.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
@TheRawMeatball @cart @Pietrek14
Optimize rendering slow-down at high entity counts (bevyengine#5509)
a065123 
# Objective

- Improve bevyengine#3953

## Solution

- The very specific circumstances under which the render world is reset meant that the flush_as_invalid function could be replaced with one that had a noop as its init method.
- This removes a double-writing issue leading to greatly increased performance.

Running the reproduction code in the linked issue, this change nearly doubles the framerate.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@TheRawMeatball @cart @ItsDoot
Optimize rendering slow-down at high entity counts (bevyengine#5509)
7f36feb 
# Objective

- Improve bevyengine#3953

## Solution

- The very specific circumstances under which the render world is reset meant that the flush_as_invalid function could be replaced with one that had a noop as its init method.
- This removes a double-writing issue leading to greatly increased performance.

Running the reproduction code in the linked issue, this change nearly doubles the framerate.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

@mockersf mockersf mockersf left review comments

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 P-High This is particularly urgent, and deserves immediate attention X-Controversial There is active debate or serious implications around merging this PR
Projects
@cart's attention
Archived in project
Milestone
Bevy 0.9
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@TheRawMeatball @rparrett @cart @StarArawn @mbolt35 @hankjordan @mockersf @alice-i-cecile