Synchronized component registration #17569

ElliottjPierce · 2025-01-28T00:33:07Z

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."

I realized the new components would need to know about previously registered components, so I simplified.

I deleted a lot of mostly unused utility functions too since it's so painful to implement until https://doc.rust-lang.org/std/sync/struct.MappedRwLockReadGuard.html is stabalized.

Added back in the mutable versions. Keep lock during frequent registrations.

Before, required components could be read directly from ComponentInfo. But sicne there could be staged changes to required components, this could be wrong. Hence, that interface was removed, and now, required components can be requested directly.

The new ones were cloned from the original so merging is not needed.

these were mostly originally inlined, but it was dropped along the way.

ElliottjPierce · 2025-01-28T00:39:05Z

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 · 2025-01-29T20:04:06Z

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.

I figured this was a more precise name.

ElliottjPierce · 2025-01-31T15:44:38Z

@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 · 2025-01-31T16:56:58Z

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 · 2025-01-31T19:42:50Z

@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 · 2025-01-31T21:22:19Z

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

chescock

I did a first pass, and I think I understand the overall structure. It looks good so far! I left a few questions and suggestions.

I want to look some more and try to see how the implementation changed. It's a little hard to see from the diff since so much moved around.

crates/bevy_ecs/src/bundle.rs

chescock · 2025-02-03T19:14:38Z

crates/bevy_ecs/src/component.rs

+                } else {
+                    RequiredComponentsStagedRef {
+                        cold: None,
+                        working: from_info,


It looks like when there is no staged data, you put the cold data in working and leave cold: None. Why not always put cold in cold and staged in working (and then rename it staged)?

For RequiredComponentsStagedRef in particular, you are right. But in RequiredComponentsStagedMut, the working is not necessarily staged, and the cold is not necessarily present. I would be happy to change the naming for RequiredComponentsStagedRef if that is desired, but my intent here was to make its parallel with RequiredComponentsStagedMut more clear. IDK which is a better approach.

Yup, I agree that Ref and Mut should match! I'm still not quite following the difference between working and staged, though.

... Hmm, and looking at get_required_components_mut, that one does always put cold in cold and staged in working. So I don't follow how this is consistent between the two. I guess the difference is that the staged value is never None because it creates it on demand?

crates/bevy_ecs/src/component.rs

chescock · 2025-02-03T19:30:35Z

crates/bevy_ecs/src/component.rs

+impl ComponentsView for Components {
+    #[inline]
+    fn register_component<T: Component>(&mut self) -> ComponentId {
+        if self.any_staged() {


I think this says that this API will add components to cold storage if there are none staged, but otherwise add them to staging, since updating cold storage while things are staged would invalidate staged IDs. Is that right?

Why not just unconditionally do self.full().register_component() to clean() if necessary? We'll need to clean eventually, so it seems cheaper to do it eagerly so that there is less staged.

since updating cold storage while things are staged would invalidate staged IDs. Is that right?

Bingo! We could reserve IDs as another solution but that seemed too complex for a first pass.

As for why not just use full, we certainly could. I did not benchmark this approach, but my concern was that for batch registration, the constant calls to clean could cause performance problems compared to just this if statement. Probably wouldn't be too bad though, so we could do this if desired.

My idea behind this was to abstract the details of how when and if we stage behind different types that implement ComponentsView. A batch registration function could just take a ComponentsView, and the caller could decide if we use a clean up front or a as_mut or let it check each time like this.

IMO we should keep this as is. If we want the default behavior to be full().register_component(), I think that default should live in World::register_component to give more power to the user. Alternatively, we could remove the ComponentsVIew impl for Components to better signify that users should either use as_mut, full or full_unchecked.

That's my opinion. I'll leave it up to review as to what we end up doing.

chescock · 2025-02-03T19:34:49Z

crates/bevy_ecs/src/component.rs

+    #[inline]
+    #[expect(
+        clippy::semicolon_if_nothing_returned,
+        reason = "This should return whatever the forwarded method returns."


I'd vote for just adding the semicolons. If the return type ever changes, you'll have to change this method anyway to write out the return type.

Totally fair. If no one expresses other opinions, I'll add the semicolons before merging.

crates/bevy_ecs/src/entity/clone_entities.rs

crates/bevy_ecs/src/component.rs

chescock · 2025-02-03T20:07:34Z

crates/bevy_ecs/src/component.rs

+
+impl Drop for ComponentsMut<'_> {
+    fn drop(&mut self) {
+        self.check_for_changes();


Assuming you do keep this flag: Is this necessary for soundness? Because you can bypass this by mem::forget()ing the ComponentsMut.

That's a really good point. Although, who's going to be manually dropping this? Maybe we should just make check_for_changes public? Otherwise, this might be grounds to remove the whole flag thing. I'll wait for a consensus here, but I'm fine either way.

chescock · 2025-02-03T20:08:51Z

crates/bevy_ecs/src/component.rs

+impl<'a> ComponentsRef<'a> {
+    /// See [`ComponentsViewRef::get_required_components`]
+    #[inline]
+    fn get_required_components_with_lifetime(


What does with_lifetime mean here?

Great question. Other implementations of ComponentsViewRef need this in order to avoid code duplication. The lifetimes are needed to ensure the reference is still valid when converting other Components views into ComponentsRef as a temporary variable (Otherwise the lifetime is assumed to be tied to the temporary variable, which rust can't guarantee is valid). But the lifetime doesn't make sense as part of the trait, so I included it separately.

This is kind of a hacky solution IMO, but I felt it was better than duping the code. If anyone has an idea of a more clear name, that better explains the intent of the lifetime, feel free.

crates/bevy_ecs/macros/src/component.rs

ElliottjPierce

Thanks @chescock for your review! You raised a lot of good points. Let me know if I missed any.

crates/bevy_ecs/macros/src/component.rs

crates/bevy_ecs/src/bundle.rs

ElliottjPierce · 2025-02-04T00:06:43Z

crates/bevy_ecs/src/component.rs

+impl<'a> ComponentsRef<'a> {
+    /// See [`ComponentsViewRef::get_required_components`]
+    #[inline]
+    fn get_required_components_with_lifetime(


Great question. Other implementations of ComponentsViewRef need this in order to avoid code duplication. The lifetimes are needed to ensure the reference is still valid when converting other Components views into ComponentsRef as a temporary variable (Otherwise the lifetime is assumed to be tied to the temporary variable, which rust can't guarantee is valid). But the lifetime doesn't make sense as part of the trait, so I included it separately.

This is kind of a hacky solution IMO, but I felt it was better than duping the code. If anyone has an idea of a more clear name, that better explains the intent of the lifetime, feel free.

ElliottjPierce · 2025-02-04T00:10:57Z

crates/bevy_ecs/src/component.rs

+                } else {
+                    RequiredComponentsStagedRef {
+                        cold: None,
+                        working: from_info,


For RequiredComponentsStagedRef in particular, you are right. But in RequiredComponentsStagedMut, the working is not necessarily staged, and the cold is not necessarily present. I would be happy to change the naming for RequiredComponentsStagedRef if that is desired, but my intent here was to make its parallel with RequiredComponentsStagedMut more clear. IDK which is a better approach.

ElliottjPierce · 2025-02-04T00:25:50Z

crates/bevy_ecs/src/component.rs

+impl ComponentsView for Components {
+    #[inline]
+    fn register_component<T: Component>(&mut self) -> ComponentId {
+        if self.any_staged() {


since updating cold storage while things are staged would invalidate staged IDs. Is that right?

Bingo! We could reserve IDs as another solution but that seemed too complex for a first pass.

As for why not just use full, we certainly could. I did not benchmark this approach, but my concern was that for batch registration, the constant calls to clean could cause performance problems compared to just this if statement. Probably wouldn't be too bad though, so we could do this if desired.

My idea behind this was to abstract the details of how when and if we stage behind different types that implement ComponentsView. A batch registration function could just take a ComponentsView, and the caller could decide if we use a clean up front or a as_mut or let it check each time like this.

IMO we should keep this as is. If we want the default behavior to be full().register_component(), I think that default should live in World::register_component to give more power to the user. Alternatively, we could remove the ComponentsVIew impl for Components to better signify that users should either use as_mut, full or full_unchecked.

That's my opinion. I'll leave it up to review as to what we end up doing.

crates/bevy_ecs/src/entity/clone_entities.rs

crates/bevy_ecs/src/world/mod.rs

crates/bevy_ecs/src/component.rs

ElliottjPierce · 2025-02-04T00:53:26Z

crates/bevy_ecs/src/component.rs

+
+impl Drop for ComponentsMut<'_> {
+    fn drop(&mut self) {
+        self.check_for_changes();


That's a really good point. Although, who's going to be manually dropping this? Maybe we should just make check_for_changes public? Otherwise, this might be grounds to remove the whole flag thing. I'll wait for a consensus here, but I'm fine either way.

Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

ElliottjPierce · 2025-02-04T01:22:34Z

Victoronz · 2025-02-04T01:39:30Z

crates/bevy_ecs/src/world/mod.rs

+    /// Retrieves this world's [`Components`] collection mutably.
+    #[inline]
+    pub fn components_mut(&mut self) -> &mut Components {
+        &mut self.components
+    }
+


Is it necessary for this to be public? As far as I know, all previous &mut Components accesses were in-engine only, and the lack of a public components_mut was intentional.

Great point. The reason I did that was to enable users to either do as_mut or full at their choosing to speed up batch registration. IDK if that's worth it though. Definitely needs more eyes on this.

Victoronz · 2025-02-04T01:53:16Z

Something that should be carefully thought about is what we intend to do with WorldQuery::init_state; #13442 was reverted for the time being to keep supporting bevy-trait-query, however it is still something that we want to eventually do, once Resources-as-Entities becomes a proper thing.
How would this feature interact with that fix, are they compatible?

ElliottjPierce · 2025-02-04T02:07:54Z

@Victoronz I like having that initializer struct, but I think in the future it could be a trait. Ex: fn init(&mut impl ComponentsInitializer). Then we could pass into that mutable components if we have them or locked components if we need to lock. I think this will adapt to that pretty nicely, but that's just my guess. I'm working right now on getting rid of the Storages requirement too, which should simplify things.

# Objective Progresses #17569. The end goal here is to synchronize component registration. See the other PR for details for the motivation behind that. For this PR specifically, the objective is to decouple `Components` from `Storages`. What components are registered etc should have nothing to do with what Storages looks like. Storages should only care about what entity archetypes have been spawned. ## Solution Previously, this was used to create sparse sets for relevant components when those components were registered. Now, we do that when the component is inserted/spawned. This PR proposes doing that in `BundleInfo::new`, but there may be a better place. ## Testing In theory, this shouldn't have changed any functionality, so no new tests were created. I'm not aware of any examples that make heavy use of sparse set components either. ## Migration Guide - Remove storages from functions where it is no longer needed. - Note that SparseSets are no longer present for all registered sparse set components, only those that have been spawned. --------- Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

# Objective Progresses bevyengine#17569. The end goal here is to synchronize component registration. See the other PR for details for the motivation behind that. For this PR specifically, the objective is to decouple `Components` from `Storages`. What components are registered etc should have nothing to do with what Storages looks like. Storages should only care about what entity archetypes have been spawned. ## Solution Previously, this was used to create sparse sets for relevant components when those components were registered. Now, we do that when the component is inserted/spawned. This PR proposes doing that in `BundleInfo::new`, but there may be a better place. ## Testing In theory, this shouldn't have changed any functionality, so no new tests were created. I'm not aware of any examples that make heavy use of sparse set components either. ## Migration Guide - Remove storages from functions where it is no longer needed. - Note that SparseSets are no longer present for all registered sparse set components, only those that have been spawned. --------- Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

ElliottjPierce · 2025-03-04T15:00:54Z

Closing in favor of #18155, and its associated PRs. Thanks to everyone who has worked on this. The feedback has been invaluable for this and future implementations.

ElliottjPierce added 20 commits January 26, 2025 17:27

outlined structure

54d411d

removed storages from registration

8112810

re-archetected synchronization

6761337

sorted out registering

41b243c

consolidated components together

98ba5c3

I realized the new components would need to know about previously registered components, so I simplified.

gets component info

d5232d1

I deleted a lot of mostly unused utility functions too since it's so painful to implement until https://doc.rust-lang.org/std/sync/struct.MappedRwLockReadGuard.html is stabalized.

some mutable gets

0bd939e

repaired some signatures

d009642

component interface MVP

49ee1dc

cleaned up most usages

0c04a57

components are cleaned

788b61d

cleaned up implementation

9cc0537

re-aranged for convinience

578491a

re-organized to use previous implementations where possible

139eedb

Added back in the mutable versions. Keep lock during frequent registrations.

use trait interface

4d9fb2b

fixed most errors

070e629

replace instead of merge.

0fb4071

The new ones were cloned from the original so merging is not needed.

inlined some functions

e3c8833

these were mostly originally inlined, but it was dropped along the way.

final utils

e37011f

ElliottjPierce added 3 commits January 28, 2025 21:29

reserve space before spawning

58f7443

no panics on missing set

9f8fe6f

fixed docs

ac6ce01

alice-i-cecile requested review from james7132, joseph-gio, alice-i-cecile and maniwani January 29, 2025 20:08

alice-i-cecile removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Jan 29, 2025

alice-i-cecile requested review from chescock and Victoronz January 29, 2025 20:09

ElliottjPierce and others added 2 commits January 29, 2025 20:57

renamed is_new to is_staged

4242865

I figured this was a more precise name.

Merge branch 'main' into synchronized-component-registration

fe58464

Merge branch 'main' into synchronized-component-registration

c57a570

chescock reviewed Feb 3, 2025

View reviewed changes

ElliottjPierce commented Feb 4, 2025

View reviewed changes

Apply suggestions from code review

3993bc5

Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>

Victoronz reviewed Feb 4, 2025

View reviewed changes

ElliottjPierce mentioned this pull request Feb 4, 2025

Isolate component registration #17671

Merged

ElliottjPierce mentioned this pull request Feb 6, 2025

Generalized and atomic staging utils #17701

Closed

ElliottjPierce mentioned this pull request Feb 15, 2025

Staged Components #17871

Closed

ElliottjPierce mentioned this pull request Mar 4, 2025

Tracking: Atomically Staged Components #18155

Closed

13 tasks

ElliottjPierce closed this Mar 4, 2025

Uh oh!

Synchronized component registration #17569

Synchronized component registration #17569

Uh oh!

Conversation

ElliottjPierce commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Pros

Cons

Migration Guide

Uh oh!

ElliottjPierce commented Jan 28, 2025

Uh oh!

ElliottjPierce commented Jan 29, 2025

Benches

Needs Careful Review

Future work

Uh oh!

ElliottjPierce commented Jan 31, 2025

Uh oh!

chescock commented Jan 31, 2025

Uh oh!

ElliottjPierce commented Jan 31, 2025

Uh oh!

chescock commented Jan 31, 2025

Uh oh!

chescock left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

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!

Uh oh!

ElliottjPierce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElliottjPierce commented Feb 4, 2025

Uh oh!

Victoronz Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ElliottjPierce commented Jan 28, 2025 •

edited

Loading

Victoronz Feb 4, 2025 •

edited

Loading

Victoronz commented Feb 4, 2025 •

edited

Loading