Optimize Visibility Systems

[aevyrie]

Objective

• Make visibility systems less slow.
• Saves as much as 1.2ms of CPU time on a (GPU bound) 6.7ms frame, rendering caldera hotel.
• Fixes Major perf regression in d00240d #22256

Solution

• Replace the EntityHashSet with a component added directly to entities. This still allows for correct change detection triggers on visibility, but avoids hashing. This also enables parallel updates.

Summary

• This PR lead to a rabbit hole that uncovered Major perf regression in d00240d #22256
• This PR resolves the regression introduced since 0.17 released
• A more fair comparison is not to main (which has a regression), but to 0.17
• Compared to 0.17.3, this is a 27% speedup to the PostUpdate schedule for caldera hotel on an M4 Max

Testing

• Many cubes, many foxes, caldera
• Yellow is new, red is old.
• Note the bimodality in the old traces, this was consistently repeatable, and seems to have something to do with EntityHashSet and EntityHashMap. Worth investigating that further, as I've seen that bimodal behavior before, and blamed it on pcores vs ecores, but I verified that is not happening.
• Summary: caldera hotel no longer exhibits bimodal performance with a pathological mode that runs very slowly. Roughly comparing the ~90th percentile performance, the optimized code is about 1.2ms faster. This is particularly significant when you consider this is running on a device that is already hitting 150fps (GPU bound), so it only has a frame budget of 6.7ms.
• Notably, visibility checking was the last egregiously performing set of systems in the common hot path of most bevy apps, and no longer stick out as obviously slow in a frame profile:

A high level comparison of the change in CPU time by looking at just PostUpdate between old(red) and new(yellow)

caldera hotel - all entities in view

For this test, I didn't touch the camera, so all entities were in view at all times.

check_visibility

reset_view_visibility

mark_newly_hidden_entities_invisible

This is 23us slower than the "fast mode" of the existing code on main, but 180-250us faster than the weird "slow mode" on main. The new code is significantly more stable, and does not exhibit the super slow mode.

caldera hotel - no entities in view

For this test, I immediately rotated the camera so the hotel was not in view.

check_visibility

This is largely a wash. The old code is 2us faster, but this is likely in the realm of noise.

reset_view_visibility

This is a big win. The main peak is about 30us faster now, but the major win is the worst case, which is nearly 500us faster.

mark_newly_hidden_entities_invisible

Same story as the other caldera comparison with everything in view, this is a bit slower than the fast mode, but way faster than the slow mode on main.

[aevyrie]

This needs a comments pass - visibility comments are pretty out of date, and this makes it worse.

[aevyrie]

Just grokked some things about why the way the code is the way it is. Need to fix some things and trying to make it way less confusing.

[aevyrie]

Some overall frametime numbers from Griffin on discord

I have some odd results.
Ran with the builtin benchmark on my caldera hotel 01 repo. 7950x/4070ti/linux (CPU time)
6.06ms bevy main (not super consistent but didn't see spikes)
4.86ms faster-vis-checks (often ~4ms with random spikes up to ~13ms)
4.74ms bevy 0.17.3 (also often ~4ms with random spikes up to ~13ms)

[aevyrie]

Verified this exact same behavior reproduces on a lower CPU spec windows machine as well. This confirms that bimodal behavior seemingly coming from the hash set is not platform specific.

Saw a 8.25ms main -> 7.64ms frametime improvement once I upped the parallelism of the thread pool.

[hukasu]

Is the only transitions that needs to be verified is changing from visible to hidden? The inverse (hidden to visible) is not needed?

[aevyrie]

What do you mean by "verified"?

Do these docs help? (on WasVisibleNowHidden) https://github.com/bevyengine/bevy/pull/22226/changes?diff=split#diff-fec33d34072b7b4be336571ceecf2c10fc9bafd8366c1b29531089b7e3ef621cR527

[hukasu]

maybe checked would be a better term

would having a N-M relationship (when available) that links entities to cameras replace that component?
i.e.

#[derive(Component)]
#[relationship(relationship_target = VisibleIn)]
struct Viewing(HashSet<Entity>);

#[derive(Component)]
#[relationship_target(relationship = Viewing)]
struct VisibleIn(HashSet<Entity>);

and checking if the entity has VisibleIn or if VisibleIn::len() == 0 be equivalent to WasVisibleNowHidden?

[aevyrie]

What does using a relationship provide here? I don't follow the purpose of your suggestion.

Revisiting your original question, I think I understand what you're asking now

Is the only transitions that needs to be verified is changing from visible to hidden?

No, we need to prevent all spurious change triggers, the ViewVisibility component should only be mutated when the value has changed globally. This is because rendering systems downstream use change detection to avoid excess updates to the GPU when nothing has changed.

This is challenging to do efficiently, and especially in a parallelizable way. The docs explain why we need to track this state in a scratch space, and doing so in a component makes parallel updates easy compared to a single global hash set. It's not immediately clear how using relations would help with that.

and checking if the entity has VisibleIn or if VisibleIn::len() == 0 be equivalent to WasVisibleNowHidden?

I don't think so, WasVisible_ is specifically covering the case of the visibility of the entity last update. This proposed relation is just covering the current state as far as I can tell.

Also, as far as I've seen in the past, relationship updates are extremely slow compared to dense parallel component updates.

[hukasu]

Ok, that cleared my question

[alice-i-cecile]

Simple enough, and those numbers are great. Thanks!

[pcwalton]

Nice, only one minor nit.