Arc-d assets

[andriyDev]

Objective

Fixes #15723. This is an alternative to #15346 (and the associated PR #15359) obsolete. Note if we decide asset locking is preferable, there are learnings from this PR we can apply to #15359.

Solution

1. Assets are now stored as Arc<A> instead of just A.

2. get remains the same, but I've added an alternative called get_arc which returns a clone of the Arc that stores an asset.

3. Replaced the get_mut with one of get_cloned_mut, get_inplace_mut, and get_reflect_cloned_mut (this last one is just because it's necessary for reflection, but not necessarily needed for users).

4. Added add_arc and insert_arc to allow adding Arc types directly.

   1. add currently takes Into<A>. Trying to change this to Into<Arc<A>> breaks a lot of callsites like meshes.add(Rectangle::new(1.0, 2.0)) (since this requires two Into calls, which Rust can't figure out).
   2. As a result, making add_arc take Into<Arc<A>> is a fine middle ground to allow both Arc-ed assets and owned assets.
   3. For completeness, I also added insert_arc.

5. RenderAsset now takes an Arc<Self::SourceAsset>. This means in most cases, cloning the asset can be skipped when passing data to the render world. Data only needs to be cloned if you mutate an asset.

Testing

• Added an example that shows off using an Arc-d asset in an async context.

---

Showcase

Assets can now be used in asynchronous contexts! By using Assets<A>::get_arc, you can get a clone of the underlying Arc of an asset, and pass it to an async closure.

fn my_task_launcher(configs: Res<Assets<ProceduralGenerationConfig>>, selected_config: Res<SelectedConfigHandle>) {
    let config = configs.get_arc(&selected_config.0).expect("The asset is loaded");
    
    AsyncComputeTaskPool::get().spawn(move async || {
        // Do something with the `config` in a task!
        std::thread::sleep(std::time::Duration::from_secs(config.time_it_takes_to_generate));
        println!("Done generating a level!");
    });
}

Migration Guide

• Assets::get_mut has been removed. Use get_cloned_mut for Clone assets, or get_in_place_mut for non-Clone assets (and handle the case where the asset is aliased).
• Assets::iter_mut now returns an iterator of &mut Arc<A>. Consider using Arc::make_mut for Clone assets, or Arc::get_mut for non-Clone assets (and handle the case where the asset is aliased).
• RenderAssets now take an Arc<Self::SourceAsset>. Consider using Arc::try_unwrap() and falling back to a clone if the asset is aliased (try_unwrap returns an error). Otherwise, prefer borrowing the source asset's data instead of moving it.

[BenjaminBrienen]

Can you fix the inconsistency between "[Arc]-ed" and "[Arc]d"? I prefer the latter.

[andriyDev]

Can you fix the inconsistency between "[Arc]-ed" and "[Arc]d"? I prefer the latter.

Done!

[andriyDev]

No-op rebase to handle conflicts with #16368, and updated error to using thiserror after #16684.

[andriyDev]

@ElliottjPierce First, technically a file is not strictly necessary, however it is motivating. That is, if it wasn't a file, a reader might be confused - why are we making this an asset if we could have just passed this around as an Arc<LinearInterpolation> in a resource? So while I could technically make this work without a file, I think it explains the concept much better: we can load assets and still pass them around in async. That said, I did rename the file to match the name of the example.

Second, I am skeptical that assets as entities will really change much in terms of async. If anything, I expect it to make it harder to access assets in async, since now you need to synchronize with the whole world (or the whole asset world or something). I'm not saying we shouldn't do assets-as-entities (I very much want it), but I believe it is gonna end up being mostly orthogonal.

Next, this is absolutely a risk. However, I think the splitting of get_mut into get_cloned_mut and get_inplace_mut (and their respective doc comments will likely give people enough info on what's going on (I hope!). The other thing to note: both get_*_mut versions still require &mut Assets<A>. So it is still not possible to have two changes happen at the same time. So at least that is not at risk.

And yes, we are losing the "dense" nature of assets. But unlike components, I would expect most assets are fairly big anyway, and so won't benefit much from cache locality. Plus, assets are accessed in a more "random-access" way anyway. I think it's pretty rare to iterate over all assets. Users can still mimic the RwLock setup by just making their asset a RwLock wrapper.

I don't think this is the final solution - I agree, assets-as-entities may change things. But we should still support assets in async. This is necessary for asset saving (which I want to work on next).

Thanks for your review!!

[ElliottjPierce]

technically a file is not strictly necessary, however it is motivating.

Makes sense. Definitely worth keeping then, good call.

I am skeptical that assets as entities will really change much in terms of async.

Me too. I just mean if/when we have assets as entities in their own world, and that world is referenced by both the main and render world, etc, the cloning and sharing of assets between contexts won't be much of a motivation anymore. The async stuff is still a very strong motivation, but only for a subset of asset types that need to be referenced in a task or something. And it might be easier to do that with incremental systems or tasks in the asset world anyway. I know that context is hypothetical; just thinking ahead.

And yes, we are losing the "dense" nature of assets.

Good points here too. You've convinced me.

Users can still mimic the RwLock setup by just making their asset a RwLock wrapper.

I know we aren't loosing anything here. Just feels odd that we are standardizing the Arc pattern instead of leaving it up to the user whether to do A , Arc<A>, or Arc<RwLock<A>>. But I suppose Arc is the most common.

I don't think this is the final solution - I agree, assets-as-entities may change things.

Still, that's in the future, and as long as we're ok re-evaluating this in like a year, I do agree that this is the best solution until then.

Next, this is absolutely a risk.

I'm ok with the additional foot guns here. We just need a really good introduction of the concept. If you have label permissions, it might be worth adding needs-release-note, but IDK how Bevy does that process.

Thanks for your review!!

You've reviewed two of mine, so I still owe you one lol. And I'm excited for that saving work. Arcs certainly are the right choice for that, and I suppose it's a reasonable assumption that all assets should be "savable". It's always confused me why game engines have great systems for scriptable objects (unity), resources (godot), etc, but never make them savable at runtime. So I'm excited to see this come to Bevy, IIUC. Thanks!

[mockersf]

did you check the impact of this on rendering an asset heavy scene?

[mockersf]

really dislike the public api change, but not sure there's a better way to do it

[kristoff3r]

I don't understand how get_cloned_mut is ever the right thing to do. In case it's aliased you clone the asset, make your changes, but unless you store it somewhere or re-register with the asset system won't it just be dropped again? But none of the uses in the PR seems to do that.

I haven't read the entire PR so I might have missed something.

[andriyDev]

@kristoff3r get_cloned_mut uses Arc::make_mut. It takes a mutable reference to an arc (in this case, the arc that is stored in Assets). If the arc is aliases, it clones the data, makes a new arc, and then stores that arc in the mutable reference. This means the arc stored in Assets is replaced when you call get_cloned_mut for an aliased asset.

[kristoff3r]

@kristoff3r get_cloned_mut uses Arc::make_mut. It takes a mutable reference to an arc (in this case, the arc that is stored in Assets). If the arc is aliases, it clones the data, makes a new arc, and then stores that arc in the mutable reference. This means the arc stored in Assets is replaced when you call get_cloned_mut for an aliased asset.

Ah that makes total sense, thanks for explaining.

[andriyDev]

@mockersf Sorry I realized I didn't respond to your other question: I have not tested this on an asset heavy scene. Does Bevy have an example or stress test of this?

It is possible that the extra dereferences hurt performance, but that might not be too big a deal. We already put render assets in a hashmap so cache locality might already be hurting? Unsure.

[djeedai]

I'm not sure Arc is the right tool for the job. Sure, it solves the issue of thread safety and concurrent (read-only) access. But by design Arc and most shared pointers do not assign any preferred role to their owners. That is, all owners of an Arc<A> have the same "status" regarding the ownership and usage of A. In many usages that's a good thing, but it's not in the case with assets. The asset management system (for lack of a better name) which is in charge of loading (and saving; mostly in Editor) assets, and maintaining them resident in memory based on usage (under constrained memory budget) should have a much stronger role than all other "users" of an asset. With Assets<A> we have most of this, because Assets owns the asset and nobody else. With Arc we lose this, because by design the ownership becomes shared. And the fact anyone can keep an old version of an asset alive by mistake scares me; for having done that countless time by mistake with bind groups in rendering, this leads to many bugs which are often hard to track, because the asset may have changed only in subtle ways, and looking at the data in a debugger often tricks you into thinking it's still the correct one.

I would suggest investigating a custom smart pointer (or Arc with some additional wrapper which derefs to A?) which does something similar to Arc, but on top of that can mark the asset as "old"/"deprecated", so that any other user holding an Arc<A> can immediately realize that the asset is outdated and they need to pool Assets to refresh it. In fact, ideally that would happen automatically under the hood, if we can "repoint" the smart pointer to the newer asset. That way you cannot, by design, use an outdated asset. This may require a read-only pointer (no mutable access), which as mentioned should be the dominant use case anyway; in that case you'd have to go back to Assets<A> to mutate, but I think that's reasonable?

Thoughts?

[JMS55]

Fyi MeshletMesh is an example of a read-only asset that I use Arc to share with the render world cheaply.

In general, I wish we could share assets between the main/render world without any copies, in the same way that bevy schedules systems across multiple threads to avoid conflicts.

[alice-i-cecile]

@cart @andriyDev I'm going to cut arc'd assets PR [from the 0.16 milestone]. I know that this is overdue for a review (and generally I'm supportive of it), but I don't think it's high enough priority to ship something high risk at the last minute or delay the release candidate

Me on Discord

[ElliottjPierce]

I made a discussion post #18949 that is an alternative to this. I don't have a crazy strong opinion, but I think my proposal is worth investigating before merging this. Just in case.

[ElliottjPierce]

Just a head's up that @andriyDev and I have come up with a followup to this PR. See here.

I think we need to start with merging this pr, but I think in the end, we'll have a less breaking api change and a much more flexible system.

[PixelDust22]

If the goal of this PR is to make assets usable in async context, why not just wrap the specific read-only assets in Arc yourself? So instead of Assets<T> Someone could just use Asset<Arc<T>> instead of wrapping everything in Arc across the board.

The ownership of assets used to be very simple: Handle<T> is simply a handle, the resource is owned by Asset<T>. This PR makes it possible to get an &mut Arc<T> from Asset<T> with a handle, but what exactly am I supposed to do with that &mut Arc<T> if I want to mutate it? Once you wrap something inside Arc<T> it's no longer mutable.

Both this PR and #15346 is trying to do one thing: we're trying to discretize the ownership of Asset<T> so that one could have access over one T without having the whole Asset<T>. #18295 is also related to this. I agree with that sentiment: it's too constraining to require asset users to either have nothing or have it all. This only works in the context of systems and basically no where else.

I feel like we're dancing around the constraints posed by Asset<T>. Why not just go all the way and get rid of Asset<T> altogether?

• Every Handle can just be an async Arc<RwLock<T>>.
• Async context and loaders can get a &T or &mut T from the handle by awaiting on it. #18295 can use the regular load method then await on it for example.
• Sync context (like a regular system) can try_deref the handle. If there's contention it'll return None.
• Asset<T> is a dummy resource to eliminate contention between systems

So yeah I do think that this PR requires a bit more deliberation so that we can think about the root problem we're trying to solve (the "have nothing or have it all" asset ownership model) rather than a particular pain point caused by that ownership model.

My pain point in this case is #18295 and I think it shares the same root cause as this one.

[JMS55]

If the goal of this PR is to make assets usable in async context, why not just wrap the specific read-only assets in Arc yourself? So instead of Assets<T> Someone could just use Asset<Arc<T>> instead of wrapping everything in Arc across the board.

This is what MeshletMesh does for instance.

[andriyDev]

@PixelDust22 First, it's not true that "once something is in an Arc, it's no longer mutable". https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.get_mut allows you mutable access if there's only one Arc and no Weaks. That's exactly why I expose get_arc_mut.

Your idea of just making assets be Arc<RwLock<T>> is straight forward, but it means now accessing each asset pays a cost of having to do a lock. In contrast, this PR's arcing means most accesses won't need to pay a cost except in the rare situation that something is holding the Arc. The writer has to pay the cost, but gets to decide whether they want to pay the cost with cloning or deferring the write to when the asset isn't aliased. It's a tradeoff.

If we go with just an Arc RwLock, I would just avoid having an assets resource at all and just only deal with the assets directly. Handles also become pointless in that model, since you could just get the Arc instead. At that point, we're not even using the ECS (which I suppose is fine, but is kinda sad).

[PixelDust22]

With Arc::get_mut you lose one of the most important guarantees with &mut Assets<T>: When you have a &mut Assets<T>, a strong handle, and the asset is loaded, you may still never be able to get the asset because someone somewhere accidentally saved a copy of Arc<T>. This makes the asset ownership model unnecessarily complicated - it's no longer clear who owns what.

So again, I think we should allow separate ownership for each T instead of requiring the user to borrow all T just to access one of them.

Regarding overhead: The current ownership model with Assets<T> is nice because there's virtually no overhead when you write rarely, perhaps only once when the asset was first loaded. That's the common scenario with assets and I agree that we should continue to keep read-only overhead low.

However, the status quo is really heavy-handed whenever you need to write. The writing system will have to borrow all T mutably with ResMut<Assets<T>>. Any other systems that need T cannot run at all while that system is being executed. I think you would agree that this is in fact much more overhead compared to an RwLock.

In the common scenario with mostly reads and virtually no contention, just how much more performant is Assets<T> compared to a lock on the RwLock? A lock on a RwLock is roughly 20 cycles. With UUID asset IDs you still need to do a hash map lookup. Is it actually more clock cycles to do a lock compared to a hash map lookup? Is it really worth saving 20 cycles at the cost of reduced parallelism and increased software complexity? Can we further reduce that gap by having a really good RwLock that optimizes for our needs?

If we go with just an Arc RwLock, I would just avoid having an assets resource at all and just only deal with the assets directly. Handles also become pointless in that model, since you could just get the Arc instead.

Exactly, and that's the whole point: get rid of Assets<T> altogether. Handles are just really well optimized Arc<RwLock<T>> extended with the features that we need, like change detection and the ability to await for asset loads in async context.

At that point, we're not even using the ECS (which I suppose is fine, but is kinda sad).

I mean Assets<T> isn't too ECS either. We're really just abusing the bevy system scheduler to do very coarse-grained synchronization for us. We shouldn't "use ECS" for the sake of using ECS. It should be our top goal to make our design easy to use.