Don't recreate material bind groups if none of the GPU resources changed. - #25058
Open
pcwalton wants to merge 3 commits into
Open
Don't recreate material bind groups if none of the GPU resources changed.#25058pcwalton wants to merge 3 commits into
pcwalton wants to merge 3 commits into
Conversation
changed. At the moment, whenever a material changes, we free and recreate the bind group in the material bind group allocator. This is necessary in general when one or more GPU resources (buffers, textures, samplers) change, because it's possible for changing one of those to cause a material to overflow the slab. However, when only POD (plain old data) changes, for example a color, this constitutes needless overhead. This patch improves the performance in this case by adding an early check and fast path to material asset preparation for bindless materials. With this PR, before we free and reallocate a material, we walk through the list of all GPU resources, checking to see if the new version of the material has the same bindings. If so, we keep the material as-is in the bind group allocator and only update the data buffers. Additionally, this PR removes the allocations in the `unprepared_bind_group` function by introducing an `BindGroupBuilder` abstraction that allows reusing allocations between frames. The `BindGroupBuilder::binding_resources` vector and the `BindGroupBuilder::data_buffer` are now temporary staging buffers that can be reused from bind group to bind group. Note that we currently still perform allocations for non-bindless materials. This can be improved, but because non-bindless materials upload their data later than bindless materials do, reusing the data buffers isn't as straightforward. Importantly, this PR opens the door to parallel material preparation in the future. This is because neither the fast path nor the check to make sure we can take the fast path mutate the `MaterialBindGroupAllocator`, outside of the data buffer itself. Therefore, in the future, we can migrate the material data buffer to `AtomicSparseBufferVec` and update it in parallel just as we do for mesh instance uniforms. Note however that parallel material preparation needs some more work in the assets infrastructure, and I suspect that work will collide with assets-as-entities, so I opted to leave that for a follow-up (and also to prevent this patch from growing too large). Future work could involve adding a special fast path for materials that are *only* POD to avoid most of the material bind group allocator machinery entirely. Note that, if updating shader-accessible instance data is performance-critical, applications may find [`GpuComponentArrayBuffer`] to be a better fit than the material system. On `many_cubes --vary-material-data-per-instance --animate-materials --instance-count 16000`, this PR improves the performance of `prepare_erased_assets` on `StandardMaterial` from 33.0 ms/frame to 21.7 ms/frame, a 1.51× speedup. [`GpuComponentArrayBuffer`]: bevyengine#24922
pcwalton
added
C-Performance
| /// Set `force_no_bindless` to true to require that bindless textures *not* | ||
| /// be used. `ExtendedMaterial` uses this in order to ensure that the base | ||
| /// material doesn't use bindless mode if the extension doesn't. | ||
| fn unprepared_bind_group( |
Contributor
There was a problem hiding this comment.
Migration guide for this and UnpreparedBindGroup?
| /// | ||
| /// The [`Self::try_update_data`] fast path uses this method to determine | ||
| /// whether the fast path can be used. | ||
| fn gpu_resources_are_unchanged( |
Contributor
There was a problem hiding this comment.
Am I reading this right that if a resource's bindless index isn't in any index table, we just assume it didn't change? That's fine for the derive since everything's covered, but if someone writes their own BindlessDescriptor and misses an index, they'd get stale bindings with no warning. Maybe a debug_assert or a note on try_update_data?
| /// Populates an [`BindGroupBuilder`] with all bindings that this | ||
| /// type exposes. | ||
| /// | ||
| /// In cases where `OwnedBindingResource` is not available (as for bindless |
Contributor
There was a problem hiding this comment.
Should this comment get updated?
| } | ||
| } | ||
|
|
||
| impl BindGroupBuilder { |
Contributor
There was a problem hiding this comment.
merge in to the impl above?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
At the moment, whenever a material changes, we free and recreate the bind group in the material bind group allocator. This is necessary in general when one or more GPU resources (buffers, textures, samplers) change, because it's possible for changing one of those to cause a material to overflow the slab. However, when only POD (plain old data) changes, for example a color, this constitutes needless overhead.
This patch improves the performance in this case by adding an early check and fast path to material asset preparation for bindless materials. With this PR, before we free and reallocate a material, we walk through the list of all GPU resources, checking to see if the new version of the material has the same bindings. If so, we keep the material as-is in the bind group allocator and only update the data buffers.
Additionally, this PR removes the allocations in the
unprepared_bind_groupfunction by introducing anBindGroupBuilderabstraction that allows reusing allocations between frames. TheBindGroupBuilder::binding_resourcesvector and theBindGroupBuilder::data_bufferare now temporary staging buffers that can be reused from bind group to bind group.Note that we currently still perform allocations for non-bindless materials. This can be improved, but because non-bindless materials upload their data later than bindless materials do, reusing the data buffers isn't as straightforward.
Importantly, this PR opens the door to parallel material preparation in the future. This is because neither the fast path nor the check to make sure we can take the fast path mutate the
MaterialBindGroupAllocator, outside of the data buffer itself. Therefore, in the future, we can migrate the material data buffer toAtomicSparseBufferVecand update it in parallel just as we do for mesh instance uniforms. Note however that parallel material preparation needs some more work in the assets infrastructure, and I suspect that work will collide with assets-as-entities, so I opted to leave that for a follow-up (and also to prevent this patch from growing too large).Future work could involve adding a special fast path for materials that are only POD to avoid most of the material bind group allocator machinery entirely. Note that, if updating shader-accessible instance data is performance-critical, applications may find
GpuComponentArrayBufferto be a better fit than the material system.On
many_cubes --vary-material-data-per-instance --animate-materials --instance-count 16000, this PR improves the performance ofprepare_erased_assetsonStandardMaterialfrom 33.0 ms/frame to 21.7 ms/frame, a 1.51× speedup.