Skip to content

Commit

Permalink
Optimize common usages of AssetReader (bevyengine#14082)
Browse files Browse the repository at this point in the history
# 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

```rust
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

```rust
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.

```rust
impl bevy::asset::io::Reader for MyAsyncReadAndSeek {}
```
  • Loading branch information
JoJoJet authored and MrGVSV committed Jul 5, 2024
1 parent 01be885 commit 51bd25c
Show file tree
Hide file tree
Showing 29 changed files with 260 additions and 149 deletions.
4 changes: 2 additions & 2 deletions crates/bevy_animation/src/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::io::{self, Write};
use std::ops::{Index, IndexMut};

use bevy_asset::io::Reader;
use bevy_asset::{Asset, AssetId, AssetLoader, AssetPath, AsyncReadExt as _, Handle, LoadContext};
use bevy_asset::{Asset, AssetId, AssetLoader, AssetPath, Handle, LoadContext};
use bevy_reflect::{Reflect, ReflectSerialize};
use petgraph::graph::{DiGraph, NodeIndex};
use ron::de::SpannedError;
Expand Down Expand Up @@ -337,7 +337,7 @@ impl AssetLoader for AnimationGraphAssetLoader {

async fn load<'a>(
&'a self,
reader: &'a mut Reader<'_>,
reader: &'a mut dyn Reader,
_: &'a Self::Settings,
load_context: &'a mut LoadContext<'_>,
) -> Result<Self::Asset, Self::Error> {
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_asset/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ bevy_reflect = { path = "../bevy_reflect", version = "0.14.0-dev", features = [
bevy_tasks = { path = "../bevy_tasks", version = "0.14.0-dev" }
bevy_utils = { path = "../bevy_utils", version = "0.14.0-dev" }

stackfuture = "0.3"
async-broadcast = "0.5"
async-fs = "2.0"
async-lock = "3.0"
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_asset/src/io/android.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use std::{ffi::CString, path::Path};
pub struct AndroidAssetReader;

impl AssetReader for AndroidAssetReader {
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
let asset_manager = bevy_winit::ANDROID_APP
.get()
.expect("Bevy must be setup with the #[bevy_main] macro on Android")
Expand All @@ -25,11 +25,11 @@ impl AssetReader for AndroidAssetReader {
.open(&CString::new(path.to_str().unwrap()).unwrap())
.ok_or(AssetReaderError::NotFound(path.to_path_buf()))?;
let bytes = opened_asset.buffer()?;
let reader: Box<Reader> = Box::new(VecReader::new(bytes.to_vec()));
let reader = VecReader::new(bytes.to_vec());
Ok(reader)
}

async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
let meta_path = get_meta_path(path);
let asset_manager = bevy_winit::ANDROID_APP
.get()
Expand All @@ -39,7 +39,7 @@ impl AssetReader for AndroidAssetReader {
.open(&CString::new(meta_path.to_str().unwrap()).unwrap())
.ok_or(AssetReaderError::NotFound(meta_path))?;
let bytes = opened_asset.buffer()?;
let reader: Box<Reader> = Box::new(VecReader::new(bytes.to_vec()));
let reader = VecReader::new(bytes.to_vec());
Ok(reader)
}

Expand Down
42 changes: 16 additions & 26 deletions crates/bevy_asset/src/io/file/file_asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,40 +9,30 @@ use std::path::Path;

use super::{FileAssetReader, FileAssetWriter};

impl Reader for File {}

impl AssetReader for FileAssetReader {
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
let full_path = self.root_path.join(path);
match File::open(&full_path).await {
Ok(file) => {
let reader: Box<Reader> = Box::new(file);
Ok(reader)
File::open(&full_path).await.map_err(|e| {
if e.kind() == std::io::ErrorKind::NotFound {
AssetReaderError::NotFound(full_path)
} else {
e.into()
}
Err(e) => {
if e.kind() == std::io::ErrorKind::NotFound {
Err(AssetReaderError::NotFound(full_path))
} else {
Err(e.into())
}
}
}
})
}

async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
let meta_path = get_meta_path(path);
let full_path = self.root_path.join(meta_path);
match File::open(&full_path).await {
Ok(file) => {
let reader: Box<Reader> = Box::new(file);
Ok(reader)
File::open(&full_path).await.map_err(|e| {
if e.kind() == std::io::ErrorKind::NotFound {
AssetReaderError::NotFound(full_path)
} else {
e.into()
}
Err(e) => {
if e.kind() == std::io::ErrorKind::NotFound {
Err(AssetReaderError::NotFound(full_path))
} else {
Err(e.into())
}
}
}
})
}

async fn read_directory<'a>(
Expand Down
24 changes: 14 additions & 10 deletions crates/bevy_asset/src/io/file/sync_file_asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ impl AsyncSeek for FileReader {
}
}

impl Reader for FileReader {
fn read_to_end<'a>(
&'a mut self,
buf: &'a mut Vec<u8>,
) -> stackfuture::StackFuture<'a, std::io::Result<usize>, { crate::io::STACK_FUTURE_SIZE }>
{
stackfuture::StackFuture::from(async { self.0.read_to_end(buf) })
}
}

struct FileWriter(File);

impl AsyncWrite for FileWriter {
Expand Down Expand Up @@ -87,13 +97,10 @@ impl Stream for DirReader {
}

impl AssetReader for FileAssetReader {
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
let full_path = self.root_path.join(path);
match File::open(&full_path) {
Ok(file) => {
let reader: Box<Reader> = Box::new(FileReader(file));
Ok(reader)
}
Ok(file) => Ok(FileReader(file)),
Err(e) => {
if e.kind() == std::io::ErrorKind::NotFound {
Err(AssetReaderError::NotFound(full_path))
Expand All @@ -104,14 +111,11 @@ impl AssetReader for FileAssetReader {
}
}

async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
let meta_path = get_meta_path(path);
let full_path = self.root_path.join(meta_path);
match File::open(&full_path) {
Ok(file) => {
let reader: Box<Reader> = Box::new(FileReader(file));
Ok(reader)
}
Ok(file) => Ok(FileReader(file)),
Err(e) => {
if e.kind() == std::io::ErrorKind::NotFound {
Err(AssetReaderError::NotFound(full_path))
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_asset/src/io/gated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl<R: AssetReader> GatedReader<R> {
}

impl<R: AssetReader> AssetReader for GatedReader<R> {
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
let receiver = {
let mut gates = self.gates.write();
let gates = gates
Expand All @@ -68,7 +68,7 @@ impl<R: AssetReader> AssetReader for GatedReader<R> {
Ok(result)
}

async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
self.reader.read_meta(path).await
}

Expand Down
40 changes: 26 additions & 14 deletions crates/bevy_asset/src/io/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -277,29 +277,41 @@ impl AsyncSeek for DataReader {
}
}

impl Reader for DataReader {
fn read_to_end<'a>(
&'a mut self,
buf: &'a mut Vec<u8>,
) -> stackfuture::StackFuture<'a, std::io::Result<usize>, { super::STACK_FUTURE_SIZE }> {
stackfuture::StackFuture::from(async {
if self.bytes_read >= self.data.value().len() {
Ok(0)
} else {
buf.extend_from_slice(&self.data.value()[self.bytes_read..]);
let n = self.data.value().len() - self.bytes_read;
self.bytes_read = self.data.value().len();
Ok(n)
}
})
}
}

impl AssetReader for MemoryAssetReader {
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
self.root
.get_asset(path)
.map(|data| {
let reader: Box<Reader> = Box::new(DataReader {
data,
bytes_read: 0,
});
reader
.map(|data| DataReader {
data,
bytes_read: 0,
})
.ok_or_else(|| AssetReaderError::NotFound(path.to_path_buf()))
}

async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
self.root
.get_metadata(path)
.map(|data| {
let reader: Box<Reader> = Box::new(DataReader {
data,
bytes_read: 0,
});
reader
.map(|data| DataReader {
data,
bytes_read: 0,
})
.ok_or_else(|| AssetReaderError::NotFound(path.to_path_buf()))
}
Expand Down
Loading

0 comments on commit 51bd25c

Please sign in to comment.