Optimize common usages of AssetReader

[JoJoJet]

Objective

The AssetReader trait allows customizing the behavior of fetching bytes for an AssetPath, and expects implementors to return dyn AsyncRead + AsyncSeek. This gives implementors of AssetLoader great flexibility to tightly integrate their asset loading behavior with the asynchronous task system.

However, almost all implementors of AssetLoader don't use the async functionality at all, and just call AsyncReadExt::read_to_end(&mut Vec<u8>). This is incredibly inefficient, as this method repeatedly calls poll_read on the trait object, filling the vector 32 bytes at a time. At my work we have assets that are hundreds of megabytes which makes this a meaningful overhead.

Solution

Turn the Reader type alias into an actual trait, with a provided method read_to_end. This provided method should be more efficient than the existing extension method, as the compiler will know the underlying type of Reader when generating this function, which removes the repeated dynamic dispatches and allows the compiler to make further optimizations after inlining. Individual implementors are able to override the provided implementation -- for simple asset readers that just copy bytes from one buffer to another, this allows removing a large amount of overhead from the provided implementation.

Now that Reader is an actual trait, I also improved the ergonomics for implementing AssetReader. Currently, implementors are expected to box their reader and return it as a trait object, which adds unnecessary boilerplate to implementations. This PR changes that trait method to return a pseudo trait alias, which allows implementors to return impl Reader instead of Box<dyn Reader>. Now, the boilerplate for boxing occurs in ErasedAssetReader.

Testing

I made identical changes to my company's fork of bevy. Our app, which makes heavy use of read_to_end for asset loading, still worked properly after this. I am not aware if we have a more systematic way of testing asset loading for correctness.

---

Migration Guide

The trait method bevy_asset::io::AssetReader::read (and read_meta) now return an opaque type instead of a boxed trait object. Implementors of these methods should change the type signatures appropriately

impl AssetReader for MyReader {
    // Before
    async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
        let reader = // construct a reader
        Box::new(reader) as Box<Reader<'a>>
    }

    // After
    async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
        // create a reader
    }
}

bevy::asset::io::Reader is now a trait, rather than a type alias for a trait object. Implementors of AssetLoader::load will need to adjust the method signature accordingly

impl AssetLoader for MyLoader {
    async fn load<'a>(
        &'a self,
        // Before:
        reader: &'a mut bevy::asset::io::Reader,
        // After:
        reader: &'a mut dyn bevy::asset::io::Reader,
        _: &'a Self::Settings,
        load_context: &'a mut LoadContext<'_>,
    ) -> Result<Self::Asset, Self::Error> {
}

Additionally, implementors of AssetReader that return a type implementing futures_io::AsyncRead and AsyncSeek might need to explicitly implement bevy::asset::io::Reader for that type.

impl bevy::asset::io::Reader for MyAsyncReadAndSeek {}

[alice-i-cecile]

Nice to see this getting stress-tested and polished :) Thanks!

Minor nits about docs.

[UkoeHB]

EDIT: Solved this by removing <'_> from the reader. It should be mentioned in the migration guide.

This PR messed up my (very basic) asset loaders:

return type references an anonymous lifetime, which is not constrained by the fn input types
lifetimes appearing in an associated or opaque type are not considered constrained
consider introducing a named lifetime parameter

It's claiming the '_ lifetime in Reader<'_> is being captured. My loader does call reader.read_to_end(...).await?; so presumably the lifetime is now leaking into the future.

Example:

struct CobwebAssetLoader;

impl AssetLoader for CobwebAssetLoader
{
    type Asset = CobwebAssetFile;
    type Settings = ();
    type Error = CobwebAssetLoaderError;

    async fn load<'a>(
        &'a self,
        reader: &'a mut dyn Reader<'_>,
        _settings: &'a (),
        load_context: &'a mut LoadContext<'_>,
    ) -> Result<Self::Asset, Self::Error>
    {
        let mut bytes = Vec::new();
        reader.read_to_end(&mut bytes).await?;
        let data: serde_json::Value = from_slice(&bytes)?;
        Ok(CobwebAssetFile {
            file: LoadableFile::new(&load_context.asset_path().path().to_string_lossy()),
            data,
        })
    }

    fn extensions(&self) -> &[&str]
    {
        &[".caf.json"]
    }
}

[JoJoJet]

Seems like a misleading error message for what should be a simple fix (just remove the explicit elided lifetime). Do you mind making a rustc issue?

[UkoeHB]

Ok, rust-lang/rust#127585