Asset locking

[andriyDev]

What problem does this solve or what need does it fill?

Today, there is no way to access an asset from a thread/async block. The best you can do is clone the asset entirely, or make your asset a wrapper around an Arc with all the data. This is especially painful for #11216, which needs to be able to send assets (and their subassets) away to be serialized.

What solution would you like?

Asset locking! A user can lock an asset, which puts the asset into an Arc. This blocks mutating the asset until the asset is unlocked. Now the asset can be passed to threads and read from there.

Non-goal: being able to mutate an asset from an async block. While we could support this, I don't think we should. This would mean requiring a Mutex or similar around the asset while it's locked, which would slow down everything accessing the asset, not just the async block.

Expected usage

1. A system takes a ResMut<Assets<A>>.
2. The system calls let asset_arc = assets.lock(my_handle).
3. A task or similar is spawned that is given the Arc.
4. The task reads the Arc to do its processing.
5. The task finishes its work and drops the Arc.
6. The asset is automatically (after a world update) unlocked, returning it to normal asset storage.

Design proposal

1. Both dense and hash storage store an AssetPresence (bikeshedding ready), which looks like:

   enum AssetPresence<A> {
       None,
       Unlocked(A),
       Locked(Arc<A>),
   }

   When inserting an asset into Assets<A>, it begins in the Unlocked state. Note, it's actually totally fine to replace a locked asset. We're not locking the AssetId, we're locking the actual data. So just throwing way the existing Arc and replacing it is fine. This does lead to the slightly odd case that a locked asset may still trigger an AssetEvent::Modified. This seems fine, since relying on an AssetId remaining locked is weird and point 6 also makes this some more reasonable.

2. Two new functions are added to Assets<A> to lock and unlock assets:

   pub fn lock(&mut self, id: impl Into<AssetId<A>>) -> Option<Arc<A>> {
       // Lock the asset if it exists.
   }

   This either returns None (if the asset doesn't exist of course), moves the asset to the locked state (if the asset was unlocked), or just clones the existing Arc (if the asset was already locked).

   pub fn try_unlock(&mut self, id: impl Into<AssetId<A>>) -> Result<(), UnlockAssetError> {
       // Unlock the asset if it exists, and there is only one strong reference to the Arc.
   }

   This either returns Ok if the asset was successfully, an error if the asset doesn't exist, or an error if the asset is still being used. Unfortunately, this has no way of telling you what is keeping the asset locked. This is because cloning the Arc has no way of reporting "why" it was cloned. We could make our own wrapper around Arc to also record its reason for being cloned so here we can figure out what is locking an asset, but the complexity of this seems non-trivial and I'm not convinced the value is worth it (how often will you really not know why an asset is locked?).

   The reason for the unlock function being public is because in theory a system could lock an asset and then unlock the asset in the same frame (if they don't need the lock). Otherwise, the system could only lock the asset, and then have to wait a frame for the asset to become unlocked again. This is likely a rare case, but if we have to implement the next system anyway, this is a nice function to do so.

   Optional: try_unlock could also fail if the asset is already unlocked. It's not clear though how this information will benefit you. Maybe you could filter an asset iterator by whether the asset is unlocked?

3. A new system is added to Assets<A> to automatically unlock assets.

   fn unlock_assets(mut assets: ResMut<Self>) {
       // Try unlocking all assets.
   }

   This system will iterate through all assets and try to unlock them. This system will be automatically added when adding asset types. This way, you don't need to worry about unlocking assets, you just lock them and then some time in the future, they are unlocked.

4. Assets::get_mut can now return an error (not just None).

   pub fn get_mut(&mut self, id: impl Into<AssetId<A>>) -> Result<&mut A, MutableAssetError>

   There are now two reasons you can't get an asset mutably: either it's not present, or it's locked. In either case, returning a value doesn't really make sense. This does mean, if you want to handle the case where the asset is locked, you have to do an Assets::get call to get the locked asset, but I bet most of the time you want to get_mut there is no fallback.

   Note Assets::get is pretty much unchanged (from an API point of view). If the AssetPresence is None return Option::None. If the asset is unlocked, get a borrow to the asset. If the asset is locked, get a borrow to the value in the Arc. Nothing fancy!

5. Assets::iter_mut now returns AssetPresenceMut (bikeshedding intensifies).

   pub enum AssetPresenceMut<'a, A> {
       Unlocked(&'a mut A),
       Locked(Arc<A>),
   }

   While for Assets::get_mut I don't think it makes sense to return the Arcd value, for an iterator I think we should provide the Arcd value. It would be incredibly confusing if an asset ID was missing from this iterator if the asset was just locked.

6. Assets::remove now returns an AssetPresence.

   Removing an asset no longer gives you an owned asset. It may give you a locked asset. Therefore, we need to be able to return either the owned asset or the Arcd asset.

   We could choose to prevent removing locked assets (and similarly prevent inserting over locked assets), but it's not clear what we're winning here and is just adding more ways to fail.

7. ReflectAsset needs to match.

   • get_unchecked_mut and get_mut both need to return Result<&mut dyn Reflect, MutableAssetError> to match Assets::get_mut.

   • remove needs to return a ReflectedAssetPresence

     pub enum ReflectedAssetPresence {
         None,
         Unlocked(Box<dyn Reflect>),
         Locked(Arc<dyn Reflect>),
     }

What alternative(s) have you considered?

As discussed at the start, you can clone your data or Arc your data. Both of those are impractical. Assets can be large, so cloning the data is not practical. Arc-ing your data may not be possible if you don't own the type (e.g., Bevy's Mesh asset can't be Arc'ed externally without rewriting ~all of Bevy rendering), and also leads to your data being permanently immutable.

Additional context

This idea came from the Bevy discord!

[alice-i-cecile]

Great design writeup; I really appreciate it!

[cart]

I've been thinking for awhile that introducing Arc (or something like it) to the asset system in some way would be good. Arc-ed assets could also help with efficiently passing assets to the render app, although that gets hairy pretty quickly.

In the current form, this seems like a reasonably non-invasive proposal that enables read-only threaded asset scenarios (which I like). The downside is that I don't think this is an ideal system for navigating something like the "render app situation", as using it there would mean perma-locking all assets of a given type (ex: all meshes).

Introducing the ability for arbitrary code to permanently block mutations to an asset does feel dangerous, as right now asset mutations can be attempted at any moment. In the proposed system, anything wanting to fully safely do a mutation has to write "defensive code" to handle the case where some other arbitrary code has locked it (although in practice most people would probably just choose to fail in those cases). The act of locking in that context, is in effect a promise that I know all user-defined app code, 3rd party plugins, and upstream bevy plugins will not attempt to mutate this in a way that will fail ungracefully for the duration of the lock. Thats a pretty big "promise" for a user to make (and its a promise a 3rd party plugin intended to be consumed arbitrarily literally cannot make) , and I'm guessing most users won't have the whole system in their head when they make that promise. Practically, in most cases I suspect this will work out ok, but I do expect this to cause problems, and it could cause them in unpredictable / crashy / hard-to-predict-ahead-of-time ways.

In spite of that, I'm not necessarily against this changeset as it does enable important scenarios, and I do really like that this doesn't rock the boat too much. But I'd like us to be thoughtful about a few things first:

1. Lets seriously consider the effect this will have on people's code. What is the ideal balance between enabling new scenarios and preventing weird / hard-to-predict / outside of user control errors.
2. Can this in some way make the "render app asset situation" better? Are there any alternative implementations that can solve both problems? If so, which implementation is preferable?
3. Should "locked" be the default state? Aka we treat the majority of assets as effectively read-only, with clone-on-mutate behaviors encouraged? Should the default state be configured per asset type, per asset instance, neither, or both?

[andriyDev]

First, your idea of clone-on-mutate is very clever! I think I will actually implement this into the existing PR (for Clone assets). Note that locking an asset does not fully block mutations. That is, replacing the asset is still allowed. So the "promise" you're describing (for Clone types) is more like "I promise nothing locks the asset, otherwise I will pay the cost". This "promise" problem is still relevant for non-Clone types, but those are likely rare.

I don't like locking being the default, since it makes all mutation have a potentially large overhead (cloning the asset). I like clone-on-mutate as a backup solution, but I'd like to keep mutation mostly in ECS-land (as in, if you have mutable access to Assets<T> you should be able to mutate the asset directly).

The render app asset situation could be somewhat improved with asset locking. For assets that want to be in both the MAIN_WORLD and RENDER_WORLD (any other case requires a move, not a clone), we can skip the current implementation's clone and lock instead. I'm not too familiar with how this part works (why I didn't implement this in my PR), but I'd expect that once the data has been copied to the GPU, we can drop the Arc, and then the asset becomes mutable in the MAIN_WORLD again. Even if that's not true, this still just means that the asset can be mutated normally until it gets copied to the RENDER_WORLD, after which, you can still mutate it using copy-on-mutate.

I think enabling new scenarios is worth it here. Today, your only strategy for moving assets into async contexts is to fully clone the asset, and this sucks for the purposes of asset saving.

---

To summarize, if we make a function like get_mut_or_clone (which does the copy-on-mutate behavior when locked), and make this the front-door for users, we get the benefit of locking, without introducing those weird errors you're suggesting (and thank you for this idea @cart)

[JMS55]

We could also piggyback off of RenderAssetUsages, and Arc the resources only when they're RENDER_WORLD only or something.

[andriyDev]

I decided to write up @cart's idea of just always Arcing assets in #15723 and I feel better about it (especially after finding out about Arc::make_mut). Though I still feel uncomfortable about the disadvantage I wrote.