[Merged by Bors] - Start running systems while prepare_systems is running #4919
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
james7132
added
A-ECS
alice-i-cecile
requested changes
Jun 4, 2022
There was a problem hiding this comment.
I really like these performance wins; reducing the overhead of running systems is one of the most important areas because it makes doing the right thing compatible with doing the fast thing.
Some cleanup feedback, but the strategy, performance wins and code quality look good to me.
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
hymm
reviewed
Jun 8, 2022
|
Did some additional benchmarking to verify the results here. The results are remarkably similar to #4740's, which makes sense since both underpin how systems are scheduled onto the scheduler's threads. It overall seem to have an identical effect by decreasing the time between scheduling as system and when it's actually started, even if done via a different method. Both do seem to have a negative impact on when all or most systems are not scheduled to run due to run criteria. However, since the methodology for both is largely orthogonal, combining both together seems to compound the positive gains, and dampen the losses. Some of the gains of one is wiped out by the other, but the general cases where scheduling dominates have had their times cut in half or more. Main vs this PRInternalization vs this PR vs Both TogetherMain vs Combined |
|
Added a few other small, probably insignificant improvements:
|
alice-i-cecile
requested changes
Jun 13, 2022
There was a problem hiding this comment.
Some comments on code clarity. I'd like to ensure that this tricky logic stays relatively approachable, so I'm applying a heightened standard of review.
The comment about the "don't queue up tasks if the system can already start" change is the most critical; the logical flow is very convoluted there, and I think we should reshuffle this significantly in order to ensure that it's easy to read the branches.
hymm
reviewed
Jun 13, 2022
alice-i-cecile
approved these changes
Aug 8, 2022
inodentry
approved these changes
Aug 8, 2022
There was a problem hiding this comment.
I read through the code as thoroughly as I could, and familiarized myself with the dependencies fixedbitset and event-listener involved. :)
Everything seems to make sense. I didn't notice anything that seemed wrong.
Got a few small ideas for further potential improvements/tweaks to explore, but I'll keep quiet so we keep this PR well-scoped and don't drag it on. 😆
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
@james7132 if you have a moment to rebase this I can merge it in :) |
inodentry
added a commit
to BroovyJammy/bevy
that referenced
this pull request
Aug 19, 2022
alice-i-cecile
added
the
S-Adopt-Me
|
Adding |
james7132
removed
the
S-Adopt-Me
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Sep 13, 2022
# Objective While using the ParallelExecutor, systems do not actually start until `prepare_systems` completes. In stages where there are large numbers of "empty" systems with very little work to do, this delay adds significant overhead, which can add up over many stages. ## Solution Immediately and synchronously signal the start of systems that can run without dependencies inside `prepare_systems` instead of waiting for the first executor iteration after `prepare_systems` completes. Any system that is dependent on them still cannot run until after `prepare_systems` completes, but there are a large number of unconstrained systems in the base engine where this is a general benefit in almost every case. ## Performance This change was tested against `many_foxes` in the default configuration. As this change is sensitive to the overhead around scheduling systems, the spans for measuring system timing, system overhead, and system commands were all commented out for these measurements. The median stage timings between `main` and this PR are as follows: |stage|main|this PR| |:--|:--|:--| |First|75.54 us|61.61 us| |LoadAssets|51.05 us|42.32 us| |PreUpdate|54.6 us|55.56 us| |Update|61.89 us|51.5 us| |PostUpdate|7.27 ms|6.71 ms| |AssetEvents|47.82 us|35.95 us| |Last|39.19 us|37.71 us| |reserve_and_flush|57.83 us|48.2 us| |Extract|1.41 ms|1.28 ms| |Prepare|554.49 us|502.53 us| |Queue|216.29 us|207.51 us| |Sort|67.03 us|60.99 us| |Render|1.73 ms|1.58 ms| |Cleanup|33.55 us|30.76 us| |Clear Entities|18.56 us|17.05 us| |**full frame**|**11.9 ms**|**10.91 ms**| For the first few stages, the benefit is small but cumulative over each. For PostUpdate in particular, this allows `parent_update` to run while prepare_systems is running, which is required for the animation and transform propagation systems, which dominate the time spent in the stage, but also frontloads the contention as the other "empty" systems are also running while `parent_update` is running. For Render, where there is just a single large exclusive system, the benefit comes from not waiting on a spuriously scheduled task on the task pool to kick off the system: it's immediately scheduled to run.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
james7132
added a commit
to james7132/bevy
that referenced
this pull request
Oct 19, 2022
# Objective While using the ParallelExecutor, systems do not actually start until `prepare_systems` completes. In stages where there are large numbers of "empty" systems with very little work to do, this delay adds significant overhead, which can add up over many stages. ## Solution Immediately and synchronously signal the start of systems that can run without dependencies inside `prepare_systems` instead of waiting for the first executor iteration after `prepare_systems` completes. Any system that is dependent on them still cannot run until after `prepare_systems` completes, but there are a large number of unconstrained systems in the base engine where this is a general benefit in almost every case. ## Performance This change was tested against `many_foxes` in the default configuration. As this change is sensitive to the overhead around scheduling systems, the spans for measuring system timing, system overhead, and system commands were all commented out for these measurements. The median stage timings between `main` and this PR are as follows: |stage|main|this PR| |:--|:--|:--| |First|75.54 us|61.61 us| |LoadAssets|51.05 us|42.32 us| |PreUpdate|54.6 us|55.56 us| |Update|61.89 us|51.5 us| |PostUpdate|7.27 ms|6.71 ms| |AssetEvents|47.82 us|35.95 us| |Last|39.19 us|37.71 us| |reserve_and_flush|57.83 us|48.2 us| |Extract|1.41 ms|1.28 ms| |Prepare|554.49 us|502.53 us| |Queue|216.29 us|207.51 us| |Sort|67.03 us|60.99 us| |Render|1.73 ms|1.58 ms| |Cleanup|33.55 us|30.76 us| |Clear Entities|18.56 us|17.05 us| |**full frame**|**11.9 ms**|**10.91 ms**| For the first few stages, the benefit is small but cumulative over each. For PostUpdate in particular, this allows `parent_update` to run while prepare_systems is running, which is required for the animation and transform propagation systems, which dominate the time spent in the stage, but also frontloads the contention as the other "empty" systems are also running while `parent_update` is running. For Render, where there is just a single large exclusive system, the benefit comes from not waiting on a spuriously scheduled task on the task pool to kick off the system: it's immediately scheduled to run.
james7132
added a commit
to james7132/bevy
that referenced
this pull request
Oct 28, 2022
# Objective While using the ParallelExecutor, systems do not actually start until `prepare_systems` completes. In stages where there are large numbers of "empty" systems with very little work to do, this delay adds significant overhead, which can add up over many stages. ## Solution Immediately and synchronously signal the start of systems that can run without dependencies inside `prepare_systems` instead of waiting for the first executor iteration after `prepare_systems` completes. Any system that is dependent on them still cannot run until after `prepare_systems` completes, but there are a large number of unconstrained systems in the base engine where this is a general benefit in almost every case. ## Performance This change was tested against `many_foxes` in the default configuration. As this change is sensitive to the overhead around scheduling systems, the spans for measuring system timing, system overhead, and system commands were all commented out for these measurements. The median stage timings between `main` and this PR are as follows: |stage|main|this PR| |:--|:--|:--| |First|75.54 us|61.61 us| |LoadAssets|51.05 us|42.32 us| |PreUpdate|54.6 us|55.56 us| |Update|61.89 us|51.5 us| |PostUpdate|7.27 ms|6.71 ms| |AssetEvents|47.82 us|35.95 us| |Last|39.19 us|37.71 us| |reserve_and_flush|57.83 us|48.2 us| |Extract|1.41 ms|1.28 ms| |Prepare|554.49 us|502.53 us| |Queue|216.29 us|207.51 us| |Sort|67.03 us|60.99 us| |Render|1.73 ms|1.58 ms| |Cleanup|33.55 us|30.76 us| |Clear Entities|18.56 us|17.05 us| |**full frame**|**11.9 ms**|**10.91 ms**| For the first few stages, the benefit is small but cumulative over each. For PostUpdate in particular, this allows `parent_update` to run while prepare_systems is running, which is required for the animation and transform propagation systems, which dominate the time spent in the stage, but also frontloads the contention as the other "empty" systems are also running while `parent_update` is running. For Render, where there is just a single large exclusive system, the benefit comes from not waiting on a spuriously scheduled task on the task pool to kick off the system: it's immediately scheduled to run.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective While using the ParallelExecutor, systems do not actually start until `prepare_systems` completes. In stages where there are large numbers of "empty" systems with very little work to do, this delay adds significant overhead, which can add up over many stages. ## Solution Immediately and synchronously signal the start of systems that can run without dependencies inside `prepare_systems` instead of waiting for the first executor iteration after `prepare_systems` completes. Any system that is dependent on them still cannot run until after `prepare_systems` completes, but there are a large number of unconstrained systems in the base engine where this is a general benefit in almost every case. ## Performance This change was tested against `many_foxes` in the default configuration. As this change is sensitive to the overhead around scheduling systems, the spans for measuring system timing, system overhead, and system commands were all commented out for these measurements. The median stage timings between `main` and this PR are as follows: |stage|main|this PR| |:--|:--|:--| |First|75.54 us|61.61 us| |LoadAssets|51.05 us|42.32 us| |PreUpdate|54.6 us|55.56 us| |Update|61.89 us|51.5 us| |PostUpdate|7.27 ms|6.71 ms| |AssetEvents|47.82 us|35.95 us| |Last|39.19 us|37.71 us| |reserve_and_flush|57.83 us|48.2 us| |Extract|1.41 ms|1.28 ms| |Prepare|554.49 us|502.53 us| |Queue|216.29 us|207.51 us| |Sort|67.03 us|60.99 us| |Render|1.73 ms|1.58 ms| |Cleanup|33.55 us|30.76 us| |Clear Entities|18.56 us|17.05 us| |**full frame**|**11.9 ms**|**10.91 ms**| For the first few stages, the benefit is small but cumulative over each. For PostUpdate in particular, this allows `parent_update` to run while prepare_systems is running, which is required for the animation and transform propagation systems, which dominate the time spent in the stage, but also frontloads the contention as the other "empty" systems are also running while `parent_update` is running. For Render, where there is just a single large exclusive system, the benefit comes from not waiting on a spuriously scheduled task on the task pool to kick off the system: it's immediately scheduled to run.
bors bot
pushed a commit
that referenced
this pull request
Feb 27, 2023
# Objective This is a follow-up to #7745. An unbounded `async_channel` occasionally allocates whenever it exceeds the capacity of the current buffer in it's internal linked list. This is avoidable. This also used to be a bounded channel before stageless, which was introduced in #4919. ## Solution Use a bounded channel to avoid allocations on system completion. This shouldn't conflict with #7745, as it's impossible for the scheduler to exceed the channel capacity, even if somehow every system completed at the same time.
bors bot
pushed a commit
that referenced
this pull request
Feb 27, 2023
# Objective This is a follow-up to #7745. An unbounded `async_channel` occasionally allocates whenever it exceeds the capacity of the current buffer in it's internal linked list. This is avoidable. This also used to be a bounded channel before stageless, which was introduced in #4919. ## Solution Use a bounded channel to avoid allocations on system completion. This shouldn't conflict with #7745, as it's impossible for the scheduler to exceed the channel capacity, even if somehow every system completed at the same time.
Shfty
pushed a commit
to shfty-rust/bevy
that referenced
this pull request
Mar 19, 2023
# Objective This is a follow-up to bevyengine#7745. An unbounded `async_channel` occasionally allocates whenever it exceeds the capacity of the current buffer in it's internal linked list. This is avoidable. This also used to be a bounded channel before stageless, which was introduced in bevyengine#4919. ## Solution Use a bounded channel to avoid allocations on system completion. This shouldn't conflict with bevyengine#7745, as it's impossible for the scheduler to exceed the channel capacity, even if somehow every system completed at the same time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
While using the ParallelExecutor, systems do not actually start until
prepare_systemscompletes. In stages where there are large numbers of "empty" systems with very little work to do, this delay adds significant overhead, which can add up over many stages.Solution
Immediately and synchronously signal the start of systems that can run without dependencies inside
prepare_systemsinstead of waiting for the first executor iteration afterprepare_systemscompletes. Any system that is dependent on them still cannot run until afterprepare_systemscompletes, but there are a large number of unconstrained systems in the base engine where this is a general benefit in almost every case.Performance
This change was tested against
many_foxesin the default configuration. As this change is sensitive to the overhead around scheduling systems, the spans for measuring system timing, system overhead, and system commands were all commented out for these measurements.The median stage timings between
mainand this PR are as follows:For the first few stages, the benefit is small but cumulative over each. For PostUpdate in particular, this allows
parent_updateto run while prepare_systems is running, which is required for the animation and transform propagation systems, which dominate the time spent in the stage, but also frontloads the contention as the other "empty" systems are also running whileparent_updateis running. For Render, where there is just a single large exclusive system, the benefit comes from not waiting on a spuriously scheduled task on the task pool to kick off the system: it's immediately scheduled to run.