[Merged by Bors] - Filter material handles on extraction by superdump · Pull Request #4178 · bevyengine/bevy

superdump · 2022-03-11T05:48:41Z

Objective

While optimising many_cubes, I noticed that all material handles are extracted regardless of whether the entity to which the handle belongs is visible or not. As such >100k handles are extracted when only <20k are visible.

Solution

Only extract material handles of visible entities.
This improves many_cubes -- sphere from ~42fps to ~48fps. It reduces not only the extraction time but also system commands time. Handle<StandardMaterial> extraction and its system commands went from 0.522ms + 3.710ms respectively, to 0.267ms + 0.227ms an 88% reduction for this system for this case. It's very view dependent but...

alice-i-cecile · 2022-03-23T01:32:53Z

+    query: Query<(Entity, &ComputedVisibility, &Handle<M>)>,
+    mut prev_len: Local<usize>,
+) {
+    let mut materials = Vec::with_capacity(*prev_len);


This seems overly elaborate: I would prefer to rebase off of #4244 and use the size hint.

alice-i-cecile · 2022-03-23T01:35:25Z

General idea makes a ton of sense, and those perf wins are great. IMO we should prioritize #4244 and build off of it for this to have a faster and less bespoke approach to get the query's size.

superdump · 2022-03-24T10:42:27Z

General idea makes a ton of sense, and those perf wins are great. IMO we should prioritize #4244 and build off of it for this to have a faster and less bespoke approach to get the query's size.

Do we have to make PRs wait on other PRs when we don't know how long other PRs may take to merge? I think it's better to address the things that need changing due to something like #4244 across the codebase in #4244 or a follow-up to it. And then try to make sure that those patterns / APIs are used in PRs that are merged after.

alice-i-cecile · 2022-03-24T16:58:38Z

Totally fair. Can you please make a TODO then @superdump?

mockersf · 2022-03-25T00:55:22Z

 impl<M: SpecializedMaterial> Plugin for MaterialPlugin<M> {
    fn build(&self, app: &mut App) {
        app.add_asset::<M>()
-            .add_plugin(ExtractComponentPlugin::<Handle<M>>::default())


why didn't you add the visibility filter in the general plugin instead of doing a custom version for just material?

You can only filter on component presence? ComputedVisibility is a bool. When I was benchmarking using ComputedVisibility as a marker component, I would then use it as a filter. But the adding/removing of a ComputedVisibility marker component had significant cost as I recall

I mean doing

--- a/crates/bevy_render/src/render_component.rs +++ b/crates/bevy_render/src/render_component.rs @@ -164,13 +164,15 @@ impl<T: Asset> ExtractComponent for Handle<T> { fn extract_components<C: ExtractComponent>( mut commands: Commands, mut previous_len: Local<usize>, - mut query: StaticSystemParam<Query<(Entity, C::Query), C::Filter>>, + mut query: StaticSystemParam<Query<(Entity, ComputedVisibility, C::Query), C::Filter>>, ) where <C::Filter as WorldQuery>::Fetch: FilterFetch, { let mut values = Vec::with_capacity(*previous_len); - for (entity, query_item) in query.iter_mut() { - values.push((entity, (C::extract_component(query_item),))); + for (entity, computed_visiblity, query_item) in query.iter_mut() { + if computed_visiblity.is_visible { + values.push((entity, (C::extract_component(query_item),))); + } } *previous_len = values.len(); commands.insert_or_spawn_batch(values);

so that all ExtractComponentPlugin would check for visibility

Then all components being extracted would have to have a ComputedVisibility component. I suppose you could use an Option<> in the query. And if maybe_computed_visibility.is_none() or maybe_computed_visibility.unwrap().is_visible but it also feels a bit unexpected perhaps?

Do we have the guarantee that all SpecializedMaterial will have the component ComputedVisibility?

It could make sense to use ComputedVisibility as a global filter on everything that will be extracted?

I agree that hard-coding here feels odd given how general this is. Visibility is pretty generalized, but I imagine it isn't "exactly" as general as the concept of "component extraction". Maybe we just need an ExtractVisibleComponentPlugin variant? Or an ExtractComponentPlugin::<T>::extract_visible() constructor?

How about a query that includes Option<ComputedVisibility> and extracts if maybe_computed_visibility.map_or(true, |computed_visibility| computed_visibility.is_visible)?

that feels like it would make all extract a little slower when we should be able to say at build time wether there is the ComputedVisibility component or not

The alternative that I see is to have two separate queries, one with ComputedVisibility in it, one with a Without filter.

yup, following Cart suggestion of using ExtractComponentPlugin::<T>::extract_visible() instead of ExtractComponentPlugin::<T>::default() to trigger using the filtered query sounds nice to me

mockersf · 2022-03-25T00:57:11Z

this wouldn't really be improved by #4244: the computed_visibility.is_visible step is a filter which would lose the exact size iterator property of a known size

superdump · 2022-03-25T03:44:03Z

Totally fair. Can you please make a TODO then @superdump?

Will do when I’m at my computer.

EDIT: Or not due to the filter comment above.

alice-i-cecile · 2022-03-25T04:30:20Z

this wouldn't really be improved by #4244: the computed_visibility.is_visible step is a filter which would lose the exact size iterator property of a known size

Ah yep, you're right there. Sorry for misleading; and skip the TODO.

cart

I think this is a good idea, but I do think this should be generalized and applied where relevant rather than hard coding it.

…ible entities

superdump · 2022-05-03T11:21:11Z

I've retested after the extract_visible() constructor suggestion with many_cubes -- sphere for the extraction system and its corresponding system commands spans.
main: 0.519ms + 3.52ms
this: 0.386ms + 0.221ms

Not sure why extraction of StandardMaterial handles is now about 0.1ms slower than I measured before, but it's still faster and way faster system commands as most of the entities are not visible.

superdump · 2022-05-03T14:02:20Z

I think this is a good idea, but I do think this should be generalized and applied where relevant rather than hard coding it.

Done. I looked over the places using ExtractComponentPlugin and it's only used for 2D/3D materials currently, outside of examples. I chose to leave the examples using ::default() to not add noise to them as that argument has been used previously. 2D doesn't use culling... though I just realised it does have ComputedVisibility if only for propagated Visibility and RenderLayers. I'll switch it to extract_visible() there too. Beyond that we maybe 'need to' port extraction of other components to ExtractComponentPlugin, but that could be a separate PR in my opinion.

cart · 2022-05-03T18:27:48Z

bors r+

# Objective - While optimising many_cubes, I noticed that all material handles are extracted regardless of whether the entity to which the handle belongs is visible or not. As such >100k handles are extracted when only <20k are visible. ## Solution - Only extract material handles of visible entities. - This improves `many_cubes -- sphere` from ~42fps to ~48fps. It reduces not only the extraction time but also system commands time. `Handle<StandardMaterial>` extraction and its system commands went from 0.522ms + 3.710ms respectively, to 0.267ms + 0.227ms an 88% reduction for this system for this case. It's very view dependent but...

bors · 2022-05-03T18:42:11Z

Pull request successfully merged into main.

Build succeeded:

# Objective - While optimising many_cubes, I noticed that all material handles are extracted regardless of whether the entity to which the handle belongs is visible or not. As such >100k handles are extracted when only <20k are visible. ## Solution - Only extract material handles of visible entities. - This improves `many_cubes -- sphere` from ~42fps to ~48fps. It reduces not only the extraction time but also system commands time. `Handle<StandardMaterial>` extraction and its system commands went from 0.522ms + 3.710ms respectively, to 0.267ms + 0.227ms an 88% reduction for this system for this case. It's very view dependent but...

github-actions Bot added the S-Needs-Triage This issue needs to be labelled label Mar 11, 2022

superdump added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled labels Mar 11, 2022

james7132 requested changes Mar 11, 2022

View reviewed changes

Comment thread crates/bevy_pbr/src/material.rs Outdated

james7132 requested changes Mar 11, 2022

View reviewed changes

Comment thread crates/bevy_pbr/src/material.rs Outdated

james7132 approved these changes Mar 13, 2022

View reviewed changes

superdump requested review from alice-i-cecile and mockersf March 23, 2022 01:22

alice-i-cecile reviewed Mar 23, 2022

View reviewed changes

mockersf reviewed Mar 25, 2022

View reviewed changes

cart reviewed Apr 25, 2022

View reviewed changes

superdump added 4 commits May 2, 2022 23:03

Only extract SpecializedMaterial2d handles if they are visible

dd34813

Filter material handles on extraction

a75fa80

bevy_pbr: Use clone_weak for material handles

1794083

ExtractComponentPlugin: Support extracting the component only for vis…

9b3d816

…ible entities

superdump force-pushed the filter-material-handles branch from 3b4d0a5 to 9b3d816 Compare May 3, 2022 11:21

mockersf approved these changes May 3, 2022

View reviewed changes

bors Bot changed the title ~~Filter material handles on extraction~~ [Merged by Bors] - Filter material handles on extraction May 3, 2022

bors Bot closed this May 3, 2022

Uh oh!

Conversation

superdump commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Mar 23, 2022

Uh oh!

superdump commented Mar 24, 2022

Uh oh!

alice-i-cecile commented Mar 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mockersf Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mockersf May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mockersf commented Mar 25, 2022

Uh oh!

superdump commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cart left a comment

Choose a reason for hiding this comment

Uh oh!

superdump commented May 3, 2022

Uh oh!

superdump commented May 3, 2022

Uh oh!

cart commented May 3, 2022

Uh oh!

bors Bot commented May 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

superdump commented Mar 11, 2022 •

edited

Loading

mockersf Mar 25, 2022 •

edited

Loading

mockersf May 2, 2022 •

edited

Loading

superdump commented Mar 25, 2022 •

edited

Loading

alice-i-cecile commented Mar 25, 2022 •

edited

Loading