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

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

Conversation

TheRawMeatball
Copy link
Member

@TheRawMeatball TheRawMeatball commented Jul 31, 2022

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

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 TheRawMeatball force-pushed the optimize-large-entity-counts branch from 038f2a5 to a5f8c03 Compare August 1, 2022 06:16
@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

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

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.

examples/hello_world.rs Outdated Show resolved Hide resolved
@@ -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
Copy link
Contributor

StarArawn commented Aug 11, 2022

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
inodentry added a commit to BroovyJammy/bevy that referenced this pull request Aug 19, 2022
@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

@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
# 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 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
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# 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
# 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
# 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
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
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants