Staged Components #17871
Staged Components #17871
Conversation
both cold and staged are behind locks, so this is redundant
This offers more than just atomic staging options now.
Hopefully this also fixes no_std
Great suggestions. IDK why I didn't do this to start with. Co-authored-by: Zachary Harrold <zac@harrold.com.au>
Mostly optimizing docs for better diff views in the future. Great idea. Co-authored-by: Zachary Harrold <zac@harrold.com.au>
This opens up the possibility that invalid data is created. For example, if a thread crashes while actively staging a change, there might be an incomplete staged change. However, in the context that this will be used, that seems unlikely, and this behavior gives more flexibility, etc.
|
Just a quick reminder that #17701 is still under review and has some changes that are not reflected here. My plan is to merge that first, and then merge main into this branch, applying any changes from #17701. Hopefully that makes sense. It looks like GitHub doesn't have a convinient way to update this branch ahead of time, but if I'm missing a simple solution, let me know. |
NthTensor
added
the
M-Needs-Migration-Guide
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
Knows calculus? Yes. Can spell wordz? Gosh no. Co-authored-by: Periwink <charlesbour@gmail.com>
|
There are a few improvements I can make here after #18028 is merged. Just cleaning up some undocumented behavior. |
|
#16519 Fixed a code quality issue with the previous components implementation regarding I Believe I can add back Specifically, the options I found are to either change the visibility of If a strong consensus is expressed here between those options, I would be happy to go ahead and add that to this PR. I would personally prefer to change the visibility unless anyone knows of plans to use them somewhere else in bevy_ecs. But in the meantime, I'll leave this for later. |
| self.cold.len() <= id.0 | ||
| } | ||
|
|
||
| fn get_required_components(&self, id: ComponentId) -> Option<RequiredComponentsRef> { |
There was a problem hiding this comment.
This implementation is identical to the one for Stager<'_, StagedComponents>. I'm not sure if this is possible, but can we make a free generic function that takes impl ComponentsReader and implements this function? Alternatively, can maybe we should split up this trait to have the parts that are needed for this impl.
I believe the same logic applies for get_required_by.
There was a problem hiding this comment.
You are correct. This is duplicated code and it's painful to me as well.
Unfortunately, I could not find a solution to this that didn't involve even more duplicated code. See this comment followed by this one.
The short version is, most solutions end up being more complex than the duplicated code. If we really want to dedupe it, I think the best solution is probably just to throw it in a macro. The helper method approach from #17569 was more confusing IIRC. But personally, I think the duplicated code, painful as it is, is the best option. But if a different consensus is reached, I'm happy to implement another solution too.
But I think deduping it might best be brought up as its own PR/Issue since there are so many options:
- change lifetimes of traits
- add delegate functions
- use macros
- create another trait for core of
StagerandStagedRefand auto implement for T: that trait - use unsafe code to force the lifetimes to be valid (cuz they are, at least in this version of the code.)
Again, if there is a consensus here, I'll implement it, but otherwise, I think we should come up with a fix separately from this PR IMO.
There was a problem hiding this comment.
I've done some more digging, and I want to clarify a little bit more.
I'll use get_required_components as an example.
For StagedRef, the lifetime of the RequiredComponentsRef it returns is 'a, the same lifetime it immutably borrows. That means, we can read from the StagableWrites type with StagedRef, call get_required_components, and then drop the StagedRef without invalidating the lifetime. This is really useful.
For Stager, the lifetime of the RequiredComponentsRef it returns is 's as in &'s self, which includes 'a. That means, we can't drop StagedRef without invalidating the lifetime of the returned RequiredComponentsRef. That coupling of lifetimes is annoying, but that's how rust is preventing some bugs. If the lifetimes were not coupled, Stager could then be modified and resized, causing a dangling pointer and a use after free danger. So, while it's annoying, it's worth it.
In other words, the two implementations technically have different return types. So treating these as the same signature ends up taking the more restrictive approach to be safe. So, right now, ComponentsReader technically follows this more restrictive path, but we could change this in the future. For example, we may need to change this to keep some references valid after we change the backing components structure in World. If so, any unification of these implementations would have to be undone. For example, delegate functions wouldn't work, and the unsafe magic described above would become unsound. The macro solution would still work, which is why it's my favored solution at the moment.
Anyway, that's why I think we should let the dust settle a bit here before deduping it. It's a tricky problem to solve and the way we solve it has implications regarding how restrictive our lifetimes may need to be, which will determine if we need to pass around dummy values that just hold lifetimes or not. So I think we should cross that bridge when/if we get there.
There was a problem hiding this comment.
I'm not sure that's correct. The function roughly looks like:
impl ComponentsReader for StagedRef<'_, StagedComponents> {
fn get_required_components(&self, id: ComponentId) -> Option<RequiredComponentsRef> {
...
}
}The lifetime ellision rules say if there's one lifetime in the input, then the output will use that lifetime for all outputs. Here's a simplified example https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=5ff4f5011cd25264d8d19f455efd9d87 It turns out it considers &self a single lifetime even if &self is actually &'a StagedRef<'s>
This should happen in both cases, so I don't see the difference here.
Putting that aside, I am totally fine to punt this to a future PR.
There was a problem hiding this comment.
Kinda.
When you change it to a trait, the lifetime is tied to self too. Here's your example in a trait: example Note that it no longer compiles with the drop.
Hopefully that explains my thought process here. When we change the backing structure of the world's components, we may need to add lifetimes to the trait, which would make the duplicated code diverge.
That said, I'm no lifetime expert, so maybe I'm missing something.
|
@Soulghost, do you have opinions on that last question from @ElliottjPierce? |
Co-Authored-By: andriyDev <andriydzikh@gmail.com>
| self.cold.len() <= id.0 | ||
| } | ||
|
|
||
| fn get_required_components(&self, id: ComponentId) -> Option<RequiredComponentsRef> { |
There was a problem hiding this comment.
I'm not sure that's correct. The function roughly looks like:
impl ComponentsReader for StagedRef<'_, StagedComponents> {
fn get_required_components(&self, id: ComponentId) -> Option<RequiredComponentsRef> {
...
}
}The lifetime ellision rules say if there's one lifetime in the input, then the output will use that lifetime for all outputs. Here's a simplified example https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=5ff4f5011cd25264d8d19f455efd9d87 It turns out it considers &self a single lifetime even if &self is actually &'a StagedRef<'s>
This should happen in both cases, so I don't see the difference here.
Putting that aside, I am totally fine to punt this to a future PR.
|
Closing in favor of #18173. |
# Objective Make component registration faster. This is a tinny, almost petty PR, but it led to roughly 10% faster registration, according to my benchmarks in #17871. Up to y'all if we do this or not. It is completely unnecessary, but its such low hanging fruit that I wanted to put it out there. ## Solution Instead of cloning a `HashSet`, collect it into a `SmallVec`. Since this is empty for many components, this saves a lot of allocation and hashing work. Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective I found a bug while working on #17871. When required components are registered, ones that are more specific (smaller inheritance depth) are preferred to others. So, if a ComponentA is already required, but it is registered as required again, it will be updated if and only if the new requirement has a smaller inheritance depth (is more specific). However, this logic was not reflected in merging `RequriedComponents`s together. Hence, for complicated requirement trees, the wrong initializer could be used. ## Solution Re-write merging to work by extending the collection via `require_dynamic` instead of blindly combining the inner storage. ## Testing I created this test to ensure this bug doesn't creep back in. This test fails on main, but passes on this branch. ```rs #[test] fn required_components_inheritance_depth_bias() { #[derive(Component, PartialEq, Eq, Clone, Copy, Debug)] struct MyRequired(bool); #[derive(Component, Default)] #[require(MyRequired(|| MyRequired(false)))] struct MiddleMan; #[derive(Component, Default)] #[require(MiddleMan)] struct ConflictingRequire; #[derive(Component, Default)] #[require(MyRequired(|| MyRequired(true)))] struct MyComponent; let mut world = World::new(); let order_a = world .spawn((ConflictingRequire, MyComponent)) .get::<MyRequired>() .cloned(); let order_b = world .spawn((MyComponent, ConflictingRequire)) .get::<MyRequired>() .cloned(); assert_eq!(order_a, Some(MyRequired(true))); assert_eq!(order_b, Some(MyRequired(true))); } ``` Note that when the inheritance depth is 0 (Like if there were no middle man above), the order of the components in the bundle still matters. ## Migration Guide `RequiredComponents::register_dynamic` has been changed to `RequiredComponents::register_dynamic_with`. Old: ```rust required_components.register_dynamic( component_id, component_constructor.clone(), requirement_inheritance_depth, ); ``` New: ```rust required_components.register_dynamic_with( component_id, requirement_inheritance_depth, || component_constructor.clone(), ); ``` This can prevent unnecessary cloning. --------- Co-authored-by: Carter Anderson <mcanders1@gmail.com> Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
# Objective This is an alternative to #17871 and #17701 for tracking issue #18155. This thanks to @maniwani for help with this design. The goal is to enable component ids to be reserved from multiple threads concurrently and with only `&World`. This contributes to assets as entities, read-only query and system parameter initialization, etc. ## What's wrong with #17871 ? In #17871, I used my proposed staging utilities to allow *fully* registering components from any thread concurrently with only `&Components`. However, if we want to pursue components as entities (which is desirable for a great many reasons. See [here](https://discord.com/channels/691052431525675048/692572690833473578/1346499196655505534) on discord), this staging isn't going to work. After all, if registering a component requires spawning an entity, and spawning an entity requires `&mut World`, it is impossible to register a component fully with only `&World`. ## Solution But what if we don't have to register it all the way? What if it's enough to just know the `ComponentId` it will have once it is registered and to queue it to be registered at a later time? Spoiler alert: That is all we need for these features. Here's the basic design: Queue a registration: 1. Check if it has already been registered. 2. Check if it has already been queued. 3. Reserve a `ComponentId`. 4. Queue the registration at that id. Direct (normal) registration: 1. Check if this registration has been queued. 2. If it has, use the queued registration instead. 3. Otherwise, proceed like normal. Appllying the queue: 1. Pop queued items off one by one. 2. Register them directly. One other change: The whole point of this design over #17871 is to facilitate coupling component registration with the World. To ensure that this would fully work with that, I went ahead and moved the `ComponentId` generator onto the world itself. That stemmed a couple of minor organizational changes (see migration guide). As we do components as entities, we will replace this generator with `Entities`, which lives on `World` too. Doing this move early let me verify the design and will reduce migration headaches in the future. If components as entities is as close as I think it is, I don't think splitting this up into different PRs is worth it. If it is not as close as it is, it might make sense to still do #17871 in the meantime (see the risks section). I'll leave it up to y'all what we end up doing though. ## Risks and Testing The biggest downside of this compared to #17871 is that now we have to deal with correct but invalid `ComponentId`s. They are invalid because the component still isn't registered, but they are correct because, once registered, the component will have exactly that id. However, the only time this becomes a problem is if some code violates safety rules by queuing a registration and using the returned id as if it was valid. As this is a new feature though, nothing in Bevy does this, so no new tests were added for it. When we do use it, I left detailed docs to help mitigate issues here, and we can test those usages. Ex: we will want some tests on using queries initialized from queued registrations. ## Migration Guide Component registration can now be queued with only `&World`. To facilitate this, a few APIs needed to be moved around. The following functions have moved from `Components` to `ComponentsRegistrator`: - `register_component` - `register_component_with_descriptor` - `register_resource_with_descriptor` - `register_non_send` - `register_resource` - `register_required_components_manual` Accordingly, functions in `Bundle` and `Component` now take `ComponentsRegistrator` instead of `Components`. You can obtain `ComponentsRegistrator` from the new `World::components_registrator`. You can obtain `ComponentsQueuedRegistrator` from the new `World::components_queue`, and use it to stage component registration if desired. # Open Question Can we verify that it is enough to queue registration with `&World`? I don't think it would be too difficult to package this up into a `Arc<MyComponentsManager>` type thing if we need to, but keeping this on `&World` certainly simplifies things. If we do need the `Arc`, we'll need to look into partitioning `Entities` for components as entities, so we can keep most of the allocation fast on `World` and only keep a smaller partition in the `Arc`. I'd love an SME on assets as entities to shed some light on this. --------- Co-authored-by: andriyDev <andriydzikh@gmail.com>
# Objective Make component registration faster. This is a tinny, almost petty PR, but it led to roughly 10% faster registration, according to my benchmarks in #17871. Up to y'all if we do this or not. It is completely unnecessary, but its such low hanging fruit that I wanted to put it out there. ## Solution Instead of cloning a `HashSet`, collect it into a `SmallVec`. Since this is empty for many components, this saves a lot of allocation and hashing work. Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
TLDR
This pr builds on #17701 for the agenda in #17569. This has a number of advantages described below. Further, this seems to have no performance cost in the grand scheme of things.
Motivation
Right now, component registration requires mutable access to a world's components. However, registering a component is not considered a state change, since it doesn't actually spawn or modify any entity in the world.
This discrepancy has been an annoyance. It prevents truly readonly queries, multithreaded component access (for things like assets as entities), and it generally encumbers processes that would be much simpler if registering components and retrieving their information could be done with only immutable access.
As a cherry on top, allowing component registration immutably also further clarifies that this is not considered a state change, which make it more self-documenting.
It is my understanding that the Bevy community as a whole wants some form of this to be implemented if it can be done for free. Ergonomics would not be worth a performance loss here.
History
I had originally prototyped this in #17569 but am now splitting that PR into others. #17671 was the first step in this.
#17701 Generalizes the staging mechanism in #17569. This is how we afford immutable registration for practically free.
I recommend looking at #17701 for details and discussion around why this design is being explored. There has been lots of great discussion about it, and I'm still very open to new ideas.
Per @alice-i-cecile request, before #17701 is merged, we should review how it will be used. This pr is how it will be used.
Objective
The goal here is to implement the staging mechanism built in #17701 on components. This does not change how it is used right now. World components and the general user experience are unchanged. If this pr gets a green light, I will open a future one changing the backing world component storage to the staging implementation.
Costs
The biggest cost is the increased complexity, but I feel it's worth it. In terms of performance though, this appears to be totally free!
On my M2 Max computer, the original components implementation ran in 9.3 microseconds. Almost all of the new implementations are much faster:
The top line "directly" is the new no nonsense implementation at 8.4 microseconds. There was a hash set clone that I changed to a
.iter().collect::<SmallVec>()which I belvie is what caused the speedup.In the benchmarks:
In short, the only new implementation that was as slow as the original was "ArcStageOnWrite sync eager", which is also the least likely to be used.
Further, since these times are all so close to each other, we can confirm that the locking cost of the staging system is practically free. That's not to say it can't be improved, but for every use, this component implementation is already faster than the previous.
I also see potential ways to make this even faster in terms of syncing, but I don't want to get ahead of myself.
The biggest concern is the performance cost when the locking causes a block. But that blocking would last for a maximum of a few microseconds (if it is for a batch registration), and it would be incredibly unlikely. Component registration is already very rare. What are the odds that two threads try to register brand new components in the same couple microseconds? Pretty much zero.
So unless anyone has other performance concerns, I would think this is free from a performance perspective.
Future work
If this is merged, I will next work on changing the backing structure of the world. I might make a controversial pr for renaming and generalizing other Component usages since now, Components really just mean ColdComponents. There are lots of interfaces to clean up too by easing up mutable access requirements.
Breaking changes
I have recorded these in the migration guide. Note that there will be related breaking changes in future PRs that might ought to be grouped together.
Migration Guide
To generalize
Components, a number of associated functions have been moved toComponentsReaderandComponentsWriter.ComponentsReadernow includes:lenis_emptyget_infoget_nameget_info_uncheckedget_idcomponent_idget_resource_idresource_idComponentsWriternow includes:register_componentregister_component_with_descriptorregister_required_components_manualregister_resourceregister_resource_with_descriptorregister_non_sendThese functions are no longer available on
Componentswithout the respective trait being used in the module.Additionally, the
ComponentsInfo::required_componentsfunction has been removed. UseComponentsReader::get_required_componentsinstead. This returnsRequiredComponentsRef, which sharesiter_idswith theRequiredComponents. If you need fullRequiredComponents, useRequiredComponentsRef::get_all.Similarly,
World::get_required_componentsandWorld::get_required_components_by_idnow returnsRequiredComponentsRefinstead of&RequiredComponents.Component::register_required_components's parametercomponents: &mut Componentsnow takes&mut impl ComponentsWriter. This will only affect manualComponentimplementations. Other than updating the signature and potentially importingComponentsReader, this will likely not break the implementation body.Open questions
Should we implement ComponentsWriter, etc on the StageOnWrite types themselves? This could give more speed when trying to register existing components and a few other use cases, but it would also mean more code. Maybe for a future pr?
Should we add ComponentsWriter and ComponentsReader to the prelude? We could implement these on
Worldand add it to the prelude, which might be desirable but IDK.Should we use StageOnWrite or ArcStageOnWrite for world storage? StageOnWrite could be faster, but ArcStageOnWrite could be fully sent between threads. This might be especially useful for next-gen scene spawning. If the arcs are the same, we could skip component id translation for entity cloning between worlds. This might give enough of a performance edge to be worth it. IMO we do StageOnWrite for now and re-evaluate when we know more about bsn and can do some proper benchmarks.