Make the specialized pipeline cache two-level.#17915
Merged
superdump merged 4 commits intobevyengine:mainfrom Feb 18, 2025
Merged
Make the specialized pipeline cache two-level.#17915superdump merged 4 commits intobevyengine:mainfrom
superdump merged 4 commits intobevyengine:mainfrom
Conversation
Currently, the specialized pipeline cache maps a (view entity, mesh entity) tuple to the retained pipeline for that entity. This causes two problems: 1. Using the view entity is incorrect, because the view entity isn't stable from frame to frame. 2. Switching the view entity to a `RetainedViewEntity`, which is necessary for correctness, significantly regresses performance of `specialize_material_meshes` and `specialize_shadows` because of the loss of the fast `EntityHash`. This patch fixes both problems by switching to a *two-level* hash table. The outer level of the table maps each `RetainedViewEntity` to an inner table, which maps each `MainEntity` to its pipeline ID and change tick. Because we loop over views first and, within that loop, loop over entities visible from that view, we hoist the slow lookup of the view entity out of the inner entity loop. Additionally, this patch fixes a bug whereby pipeline IDs were leaked when removing the view. We still have a problem with leaking pipeline IDs for deleted entities, but that won't be fixed until the specialized pipeline cache is retained. This patch improves performance of the [Caldera benchmark] from 7.8× faster than 0.14 to 9.0× faster than 0.14, when applied on top of the global binding arrays PR, bevyengine#17898. [Caldera benchmark]: https://github.com/DGriffin91/bevy_caldera_scene
superdump
approved these changes
Feb 17, 2025
tychedelia
suggested changes
Feb 17, 2025
Member
tychedelia
left a comment
There was a problem hiding this comment.
Much improved, thanks. Can you just do the same for the 2d and prepass specialization checks as well?
brianreavis
reviewed
Feb 17, 2025
| let view_specialized_material_pipeline_cache = specialized_material_pipeline_cache | ||
| .entry(view.retained_view_entity) | ||
| .or_default(); | ||
|
|
Contributor
There was a problem hiding this comment.
Add an early-out here to avoid looping visible entities if not needed? Ish:
if !entity_specialization_ticks.is_changed() && !view_tick.is_newer_than(view_specialized_material_pipeline_cache.tick, ticks.this_run()) {
continue;
}
Contributor
Author
There was a problem hiding this comment.
Are you sure that's correct if entities have been added since the last time entity_specialization_ticks was updated?
Really, the solution here is going to be to use change detection with special cases for when entities are added and removed, like our other retained passes.
Contributor
There was a problem hiding this comment.
I could be missing something, but entity_specialization_ticks is always updated with new entities every frame. Driven by:
pub fn check_entities_needing_specialization<M>(
needs_specialization: Query<
Entity,
Or<(
Changed<Mesh3d>, // <-- Will pick up new entities
AssetChanged<Mesh3d>,
Changed<MeshMaterial3d<M>>, // <-- Will pick up new entities
AssetChanged<MeshMaterial3d<M>>,
)>,
>,
mut entities_needing_specialization: ResMut<EntitiesNeedingSpecialization<M>>,
) where
M: Material,
{
entities_needing_specialization.clear();
for entity in &needs_specialization {
// Feeds entity_specialization_ticks
entities_needing_specialization.push(entity);
}
}Regardless, this can be deferred to another PR. Just thought it could be a big quick win for static scenes.
Contributor
Author
There was a problem hiding this comment.
Yeah, let's defer this to another PR; this has already gotten big.
| let view_specialized_material_pipeline_cache = specialized_material_pipeline_cache | ||
| .entry(extracted_view_light.retained_view_entity) | ||
| .or_default(); | ||
|
|
`MainEntity`/`Entity` where `RetainedViewEntity` is more appropriate
Contributor
Author
|
@tychedelia Done. I changed the |
tychedelia
approved these changes
Feb 17, 2025
superdump
approved these changes
Feb 18, 2025
Contributor
|
This can be merged as soon as CI passes. |
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
4 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.
Currently, the specialized pipeline cache maps a (view entity, mesh entity) tuple to the retained pipeline for that entity. This causes two problems:
Using the view entity is incorrect, because the view entity isn't stable from frame to frame.
Switching the view entity to a
RetainedViewEntity, which is necessary for correctness, significantly regresses performance ofspecialize_material_meshesandspecialize_shadowsbecause of the loss of the fastEntityHash.This patch fixes both problems by switching to a two-level hash table. The outer level of the table maps each
RetainedViewEntityto an inner table, which maps eachMainEntityto its pipeline ID and change tick. Because we loop over views first and, within that loop, loop over entities visible from that view, we hoist the slow lookup of the view entity out of the inner entity loop.Additionally, this patch fixes a bug whereby pipeline IDs were leaked when removing the view. We still have a problem with leaking pipeline IDs for deleted entities, but that won't be fixed until the specialized pipeline cache is retained.
This patch improves performance of the Caldera benchmark from 7.8× faster than 0.14 to 9.0× faster than 0.14, when applied on top of the global binding arrays PR, #17898.