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

Enable queue phase parallelization by using Parallel to back RenderPhase #11984

Closed

Conversation

james7132
Copy link
Member

Objective

Part 2 on the quest to resolve #3548. Many of the queue and prepare systems are unable to run in parallel, creating a potential CPU bottleneck when dense and varied scenes.

Solution

Use Parallel, introduced in #7348, to provide a cleaner implementation of #4899's thread-local queues, so systems can add phase items to the queue without needing a mutable reference to the render phase. As multiple systems with read-only access can parallelize under Bevy's system executor, many of these systems are "read-only" in the eyes of the scheduler, and thus do not block them from running in parallel with each other.

This potentially adds a bit of additional overhead on a per item basis, due to needing to fetch the thread-local queue for each item, though it may be mitigable by providing a RenderPhase::extend option that takes an iterator instead of adding items one by one.

As a additional optimization, this PR includes a change to Parallel::drain_into to avoid copying if the output Vec is known to be empty to avoid the copying overhead when there's only one system that enqueued items before sorting.

IMO, this design is much cleaner than the one in #4899, though it may be a bit rough to grok for people seeing it for the first time.

Performance

TODO


Changelog

Changed: RenderPhase::add now only requires a Changed:RenderPhase`'s fields are now private

Migration Guide

TODO

@james7132 james7132 added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Feb 19, 2024
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

LGTM, pending benchmark results.

@rparrett rparrett added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 19, 2024
@james7132 james7132 changed the title Enable queue phase parallelization by use Parallel to back RenderPhase Enable queue phase parallelization by using Parallel to back RenderPhase Feb 20, 2024
@IceSentry
Copy link
Contributor

Currently, these changes would only be applicable to the SortedRenderPhase. I'm not sure if it's possible to do the same thing for BinnedRenderPhase. The way RenderPhase are used has also changed quite a bit since this PR. Fixing the conflicts and making it work with BinnedRenderPhase seems like non trivial work and seems to me would require a new PR built from scratch.

So closing for now, but if you disagree with me feel free to reopen it.

@IceSentry IceSentry closed this Aug 21, 2024
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Requiring mutable access to several common shared types prevents queue phase parallelization.
4 participants