bevy_ecs performance regression in 0.16.0 release (get + insertion)

[alice-i-cecile]

Problem

As spotted by @SanderMertens of flecs, Bevy's ECS perf had a serious regression from Bevy 0.15 to Bevy 0.16.

For many users, this may have been outweighed by the improvements to rendering performance and transform propagation; hence why this is just now being found and reported.

See this gist for the exact benches used.

Path to a solution

We should focus on the get / get_mut regressions first: those are larger so they'll be easier to spot. We may have multiple independent regressions, particularly since insertion / removal are also affected. The relations regressions are less surprising (we completely changed our strategy there), but should be rechecked once the other regressions are fixed.

1. Bench for Bevy 0.15.
2. Bench for Bevy 0.16
3. Bench for Bevy main (we may have somehow fixed this)
4. Bisect from Bevy 0.15 to Bevy 0.16 by git commits to figure out where this was introduced.
5. Report back so we can coordinate a fix.
6. Fix!
7. Ideally, release a point release removing or at least ameliorating the regression.

[ElliottjPierce]

I have a sneaking suspicion this comes in part from queued component registration . See here. This explains the 11x regression in get_not_found. We can fix it pretty easy with Components::get_valid_id instead of get_id, which now has atomic locking in it.

I don't think this fully explains the regression for get/get_mut, but I wouldn't be surprised if they are related.

First step is definitely benchmarks. I'd be curious if this went away with Query instead of EntityWorldMut and if we flush queued component registration before running the benches. If the flush helps, that would explain why the regression was not felt anywhere: the flush is called once per frame already.

[SanderMertens]

if we flush queued component registration before running the benches

@ElliottjPierce how do I do that?

EDIT: note that the code repeats the same benchmark many times, so if component registration only happens on the first call it likely only still has a small impact (but would still be good to eliminate).

[ElliottjPierce]

if we flush queued component registration before running the benches

@ElliottjPierce how do I do that?

Just call flush after the component is in the registration queue. Probably don't even need this tbh; just trying to eliminate things.

EDIT: note that the code repeats the same benchmark many times, so if component registration only happens on the first call it likely only still has a small impact (but would still be good to eliminate).

Yes, I'm thinking in particularly about the 11x regression in get_not_found, which has a component that is not registered. When checking for unregistered components, it locks. If the component, C2 in the bench, were registered head of time or we changed the get_id like I mentioned, this wouldn't be an issue. But right now, the bench locks when .get::<C2>() etc are called.

I suspect that between this and #19505, these regressions will be fixed.

[SanderMertens]

@ElliottjPierce

Just call flush after the component is in the registration queue. Probably don't even need this tbh; just trying to eliminate things.

How do I do that? How do I put a component in the registration queue? What do I call flush on? The world?

[SanderMertens]

That does actually seem to make a difference. If I call world.spawn(C2(...)) ahead of time the benchmark improves significantly. I'll share updated numbers after the fixes have landed.

[ElliottjPierce]

@ElliottjPierce

Just call flush after the component is in the registration queue. Probably don't even need this tbh; just trying to eliminate things.

How do I do that? How do I put a component in the registration queue? What do I call flush on? The world?

See #18173. I think actually doing it is irrelevant to the benches. It's its existence that causes there to be the atomic locking sometimes. If you do want to test it, World::flush and World::components_queue are what you want, but I don't think it's important here.

[alice-i-cecile]

Merging the two linked PRs: we should re-bench once those are in. If that's resolved, I'd like to do a point release ASAP.