Allow Mesh-related queue phase systems to parallelize #11804
Merged
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-Rendering
pcwalton
reviewed
Feb 10, 2024
JMS55
approved these changes
Feb 10, 2024
|
Does this have a significant effect on single threaded perf? |
superdump
approved these changes
Feb 19, 2024
|
Merge conflict. Code looks good. Could you test with a heavier single-material-type no-contention load like |
IceSentry
approved these changes
Feb 19, 2024
Checked against main and this PR. There seems to be next to zero impact on the average time spent, which makes sense: uncontended atomics on x86 machines is essentially free. There's a bit more variance, but not by much. Here's |
Most modern platforms (with maybe the exception of wasm) treat all load/store operations as atomic, and so long as there is no contention on the same cache line, it's identical to single threaded performance. The only thing you might be missing out on is the compiler reordering your operations to be more optimal, which isn't a particular concern for these systems. |
msvbg
pushed a commit
to msvbg/bevy
that referenced
this pull request
Feb 26, 2024
# Objective Partially addresses bevyengine#3548. `queue_shadows` and `queue_material_meshes` cannot parallelize because of the `ResMut<RenderMeshInstances>` parameter for `queue_material_meshes`. ## Solution Change the `material_bind_group` field to use atomics instead of needing full mutable access. Change the `ResMut` to a `Res`, which should allow both sets of systems to parallelize without issue. ## Performance Tested against `many_foxes`, this has a significant improvement over the entire render schedule. (Yellow is this PR, red is main)  The use of atomics does seem to have a negative effect on `queue_material_meshes` (roughly a 8.29% increase in time spent in the system).  `queue_shadows` seems to be ever so slightly slower (1.6% more time spent) in the system.  `batch_and_prepare_render_phase` seems to be a mix, but overall seems to be slightly *faster* by about 5%. 
msvbg
pushed a commit
to msvbg/bevy
that referenced
this pull request
Feb 26, 2024
# Objective Partially addresses bevyengine#3548. `queue_shadows` and `queue_material_meshes` cannot parallelize because of the `ResMut<RenderMeshInstances>` parameter for `queue_material_meshes`. ## Solution Change the `material_bind_group` field to use atomics instead of needing full mutable access. Change the `ResMut` to a `Res`, which should allow both sets of systems to parallelize without issue. ## Performance Tested against `many_foxes`, this has a significant improvement over the entire render schedule. (Yellow is this PR, red is main)  The use of atomics does seem to have a negative effect on `queue_material_meshes` (roughly a 8.29% increase in time spent in the system).  `queue_shadows` seems to be ever so slightly slower (1.6% more time spent) in the system.  `batch_and_prepare_render_phase` seems to be a mix, but overall seems to be slightly *faster* by about 5%. 
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 14, 2024
# Objective - After #11804 , The queue_prepass_material_meshes function is now executed in parallel with other queue_* systems. This optimization introduced a potential issue where mesh_instance.should_batch() could return false in queue_prepass_material_meshes due to an unset material_bind_group_id.
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
Partially addresses #3548.
queue_shadowsandqueue_material_meshescannot parallelize because of theResMut<RenderMeshInstances>parameter forqueue_material_meshes.Solution
Change the
material_bind_groupfield to use atomics instead of needing full mutable access. Change theResMutto aRes, which should allow both sets of systems to parallelize without issue.Performance
Tested against
many_foxes, this has a significant improvement over the entire render schedule. (Yellow is this PR, red is main)The use of atomics does seem to have a negative effect on
queue_material_meshes(roughly a 8.29% increase in time spent in the system).queue_shadowsseems to be ever so slightly slower (1.6% more time spent) in the system.batch_and_prepare_render_phaseseems to be a mix, but overall seems to be slightly faster by about 5%.