Generalized and atomic staging utils #17701
Conversation
both cold and staged are behind locks, so this is redundant
This offers more than just atomic staging options now.
|
The |
Ah. I wasn't aware of that. I can absolutely move it. |
|
I'd personally prefer if it stayed in |
Hopefully this also fixes no_std
|
Great, thanks for the thorough response. Seems like we should def proceed with this then. |
|
@bushrat011899 I'm satisfied with where it's at at the moment. You had a lot of great suggestions in the last round. Sorry it took so long to do them. Looking forward to more! |
| /// This trait provides some conviniencies around [`StagableWritesCore`]. | ||
| /// | ||
| /// For example, mutable references are used to enforce safety for some functions. | ||
| pub trait StagableWrites { |
There was a problem hiding this comment.
Why does this need to be a separate trait? Can't we just impl StagableWritesCore { /*methods go here*/ }?
There was a problem hiding this comment.
Thats a really good question.
StagableWritesCore gives information about the type coordinating the stage-on-write solution. This is the core of the actual implementation. StagableWrites protects against deadlocks by requiring mutable access to a reference to the StagableWritesCore type, not to the inner type itself. That reference could be a ArcStageOnWrite or a RefStageOnWrite.
As an example, let's say we have a ArcStageOnWrite in thread A, and we share that reference to threads B and C by cloning the arc. Thread's B and C can prevent internal deadlocks by using StagableWrites on their arcs (provided they don't clone the arcs internally). If the two traits were unified, we would need to clone a mutable reference between threads (not allowed) or use the unsafe, deadlock-prone writing methods on StagableWritesCore.
The extra trait StagableWrites was the best way I could think of to balance deadlock prevention with thread sharing. If you have any other ideas, I'm all ears.
|
@andriyDev Thanks for the great docs review. I don't know how I managed some of those typos lol. |
This should make it harder to deadlock.
|
Closing in favor of #18173. |
# 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>
5 participants
Objective
The goal here is to provide a general alternative to a
RwLockfor rarely changed collections. When a change is made, it stages it in temporary storage without actually modifying the data. Then, at user defined points, the changes are applied to the data, draining the staged changes.This has come up a lot regarding components, ex: #17569, but the same treatment has been discussed for reflection and other areas where a type has to be registered once and only once, on demand. This allows that functionality to be put on separate threads, which could unblock a lot of additional ideas, like assets as entities and read-only queries.
Further, it may be useful to put these "StageOnWrite" data structures in an Arc, so that has been implemented too. This could unlock benefits like not having to translate component ids when spawning scenes or cloning entities, but more on that in future PRs and discussions.
There's a lot of things this PR benefits but it should be evaluated as a stand-alone addition for these utilities. Still, those goals and use cases did influence the design, so keep that in mind.
Solution
StagedChangestrait that tracks changes to a "target" data structure,Cold.StageOnWriteandAtomicStageOnWrite.Testing
I did create a test module with an example, but the potential for bugs here is mostly in systems using this feature, so I kept it kind of minimal for now.