Synchronized component registration

[ElliottjPierce]

Objective

Registering components currently requires mutable access to a world, but conceptually, registering a component doesn't change the world's state. As discussed on discord, we want to fix that discrepancy by allowing component registration with only a read reference to the world.

If that can be done, this will open up the ability to create read only queries and could be the basis of resolving similar battles with the borrow checker.

Solution

Other solutions have been brought up in discord, but this PR attempts to solve the problem by synchronizing component registration itself.

Components now stores ComponentsData and StagedComponents. ComponentsData is effectively the previous components implementation. When a component is registered or required components are set, we lock the staged components, queue those modifications on the stage, and release the lock. Eventually (during SubApp::update for now), we apply those queued changes to the normal ComponentsData, clearing the staged changes. When we are just reading data, including registering an already registered component, the lock is only engaged if the requested data still lives in the staged changes. Effectively, the lock is almost never hit unless there is some registration process happening.

For performance, the original Components implementation has been kept, and both the new and the old are available via an abstraction trait, ComponentsView. When registering in bulk, the previous implementation may be slightly faster, and it's pretty simple to use that one instead. Additionally, when a lock needs to be used for registration, it uses a new ComponentsLock. That lock is kept between nearby registrations by default, so there is minimal locking overhead.

There is some duplicated code between different implementations of ComponentsView. We could abstract it away, but I wanted to leave it for now since we may choose to let the implementations diverge.

Testing

Current tests pass for me, but no new tests were created.

We will probably want to benchmark this vs the old implementation, but I wanted to leave that up to someone who knows better exactly what situations to benchmark.

Pros

• Fixes the problem.
• Is still very fast.
• Fully backwards compatible (components can still be registered at full speed if you have mutable access).

Cons

• More complexity.
• Can be slower in some situations.
• Harder to get Component info out due to locking guards being dropped. This will fix the problem eventually.

Migration Guide

To make catching bugs and butterfly effects easier, I went ahead and adapted multiple signatures. For example, many signatures changed from &mut to &. Also, Component::register_required_components now accepts a ComponentsView instead of Components. We can spread some of these changes over multiple PRs. It was just easier for me to follow it with the changes.

Previously, when a sparse set component was registered, its set was created. Now, the set is created when the component is inserted (or spawned) in an entity. I did this during BundleInfo creation (Maybe there's a better place). At any rate, lots of code depended on registered components having valid sets. Now the rule is spawned components have valid sets. I updated relevant usages and safety comments, but more testing should probably be done. For now, it passes my "smell test."

[ElliottjPierce]

CI should fail right now since we still need to create ComponentSparseSets. Right now we aren't doing that, so the dead code causes a failure. The unwrap I mentioned in sparse set queries may also be causing crashes in examples.

[ElliottjPierce]

The work I just finished doing should ensure there is no performance regression. When registering with mutable access to Components, it checks if there's any staged changes, and if not, it uses the previous implementation.

The only costs would be if we use the synchronous registration methods or if we are getting data for a type that was recently registered. I think we can keep this to a minimum by only using the synchronous methods when we need to (ie. read-only queries.)

Then again, with a bit more work, it might be fast enough to just use the synchronized methods out of convenience. I think the next step is to benchmark.

[ElliottjPierce]

I just added some basic benchmarks, and it looks good. Really good.

• Raw is the previous Components implementation (still available in the new one).
• Mut is when you have mutable access to Components and know you you will be registering in bulk.
• Locked is when you have immutable access to Components and know you you will be registering in bulk.
• Full is when you have mutable access to Components and don't know that you're registering in bulk.
• Full_Staged is the same as full, but it has to stage changes instead of applying them directly.
• Synced is when you have immutable access to Components and don't know that you're registering in bulk.

To me, these benchmarks indicate that when I adapted the previous implementation to work in ComponentsMut, I may have written if to run faster than before. Raw is slower than Mut and Full is slower than Full_Staged which seem to support this.

In a future PR, it may be worth trying to improve the performance of the old registration.

What the benchmarks show is that this optional synchronization is effectively free and that it can be made even faster.

Also note that these benchmarks are in the worst possible order. They ones with no required components are registered before the ones that require them. If the order was reversed, there would be even less locking, and I suspect it would run faster.

[ElliottjPierce]

At this point, the PR is feature complete IMO.

Benches

Here are the current benches for my M2 Max:

If anyone wants to run other benches, feel free. There are a few places that need to lock on a hot path, like some component insertions, but they only lock if the data they need is staged, so I don't expect that to be a problem.

Needs Careful Review

I'm still relatively new to the guts of bevy's ecs, to it is definitely possible that I made a logic mistake somewhere. I would especially appreciate feedback on the following:

• Feedback from someone more familiar with atomic operations to make sure I had the right approach with Components::staged_changed, especially my choice of ordering.
• Feedback on where I cleaned Components. Should we do this more often than SupApp::update? Maybe also during mutable registration?
• Feedback on where I created the sparse sets. Right now we do it at BundleInfo::new, but we could do it at component registration within BundleInfo::new at the cost of cloning component info. I don't think the extra memory would be worth it, but I'm open to feedback.
• Feedback on naming and ComponentsView trait ecosystem. There may be a better way to organize this. I just threw it together as a prototype.

Future work

• Open up signatures. Lots of things still need &mut Components but can be changed to &mut impl ComponentsViewExclusive. (Or &Components if we want to be specific at the potential cost of performance.)
• Open up signatures that still get &mut World for registration. Now they only need &World.
• Replace TryQuery API with a synchronous version of initing QueryState. We should keep the current version with &mut World too IMO to prevent unneeded locking.
• Improve performance of ComponentsData. For some reason ComponentsMut is performing faster, even though it is doing more. There is room for improvement here.
• Use Either crate to make accessing Components without necessarily locking it easier. For example, if you have &Components and want to get a impl ComponentsViewRef, right now you must use lock_read, but if Components::staged_changed is false, you could just get the inner &ComponentsData.
• When this is stabilized, we can improve or replace ComponentInfoRef.
• Components.rs is now getting pretty long. It may be worth re-organizing it, but IDK.

[ElliottjPierce]

@alice-i-cecile do you think it would be reasonable to tackle this in 0.16? I think getting readonly queries for 0.16 would be awesome, but I don't have as much of a bird's eye view as you do.

Also, I know this PR is bigger than ideal. If you prefer, I can split it up at your digression. Or we can try to perfect this one. Up to you ;)

[chescock]

I'm working on doing a full review, but my first thought is that this is a lot of code, and I wonder if we can find a simpler way to achieve this?

Is the goal here just to let us query with &World? Because I think that only requires reserving ComponentIds, and we could get away with deferring the actual registration until values of that component get inserted and we have &mut World. (Cart noted on Discord that it's a little more complicated than that if we want to keep the IDs dense, but I think it's still doable.)

Or are there cases where we need to read component metadata before we insert values of those components, such that this complexity really is essential?

Alternately, ... how bad would it be for performance to just stick the existing Components struct inside of a RwLock? That would let us register with &World without needing to abstract over two data stores. The lock shouldn't have that much contention, and performance-critical code will already be trying to cache ComponentIds, so it might not be that bad.

[ElliottjPierce]

@chescock Great points!

As for why we don't just reserve ComponentIds, you're right. That would be a more elegant solution for read-only queries. When I talked with @alice-i-cecile, I think the endgame was to pre-register all components so that we could have the option to toss around &Components everywhere, maybe even in an Arc. But since we want to allow runtime components (ie from mods or scripting support), we need to add a way to fully register components without mutable access. I don't know what the motivation behind it is; I just know it's a goal, and it makes sense as a feature. The bigger thing in my opinion is that, per discussions in discord, registering a component is not considered a mutation or state change in a world, but it still takes &mut World. Allowing it with just &World better signifies the intent IMO.

As for why I didn't just through current Components in a RwLock, you're right there too. If we need to split up this PR, that would be my preferred way of doing it. The reason I didn't go with this is because it felt painful to have to hit a lock just to retrieve some info about an already registered component. I don't want that to become a scaling problem down the line, but I agree that the performance loss would be minimal at the moment. This was something @cart had discussed on discord too, "Yeah accessing a locked data structure on every component info read doesn't feel like the answer."

If you want more info about the motivation behind the design, the first discussion started here, "Another quick question: Does Bevy consider registering a component to be a mutation to the..." Later in the conversation, I think Cart had expressed concern that even this staging solution could be too much locking. I think Alice had also said that this could also fix a problem Cart ran into when experimenting with assets as entities during assets v2. I would love to unblock that if possible. See #11266, but I don't know enough about that to say anything definite.

I'm certainly not saying the architecture I drew up is the best, but hopefully that gives you an idea of the rational behind it. If you have better ideas that would still address this, trust me, I'm all ears. The biggest downside of this architecture IMO is that we have to keep track of two components impls: one for ComponentsData and one for ComponentsMut. I'd be especially interested in a design that avoids that, but I'd personally rather have to pay for two implementations than pay a performance regression IMO.

[chescock]

Thanks for the comprehensive answer! That's really helpful context!