Skip to content

Commit 5876352

Browse files
authored
Optimize common usages of AssetReader (#14082)
# 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 {} ```
1 parent bd7dcd3 commit 5876352

29 files changed

+260
-149
lines changed

crates/bevy_animation/src/graph.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use std::io::{self, Write};
44
use std::ops::{Index, IndexMut};
55

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

338338
async fn load<'a>(
339339
&'a self,
340-
reader: &'a mut Reader<'_>,
340+
reader: &'a mut dyn Reader,
341341
_: &'a Self::Settings,
342342
load_context: &'a mut LoadContext<'_>,
343343
) -> Result<Self::Asset, Self::Error> {

crates/bevy_asset/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ bevy_reflect = { path = "../bevy_reflect", version = "0.14.0-dev", features = [
2828
bevy_tasks = { path = "../bevy_tasks", version = "0.14.0-dev" }
2929
bevy_utils = { path = "../bevy_utils", version = "0.14.0-dev" }
3030

31+
stackfuture = "0.3"
3132
async-broadcast = "0.5"
3233
async-fs = "2.0"
3334
async-lock = "3.0"

crates/bevy_asset/src/io/android.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use std::{ffi::CString, path::Path};
1616
pub struct AndroidAssetReader;
1717

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

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

crates/bevy_asset/src/io/file/file_asset.rs

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -9,40 +9,30 @@ use std::path::Path;
99

1010
use super::{FileAssetReader, FileAssetWriter};
1111

12+
impl Reader for File {}
13+
1214
impl AssetReader for FileAssetReader {
13-
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
15+
async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
1416
let full_path = self.root_path.join(path);
15-
match File::open(&full_path).await {
16-
Ok(file) => {
17-
let reader: Box<Reader> = Box::new(file);
18-
Ok(reader)
17+
File::open(&full_path).await.map_err(|e| {
18+
if e.kind() == std::io::ErrorKind::NotFound {
19+
AssetReaderError::NotFound(full_path)
20+
} else {
21+
e.into()
1922
}
20-
Err(e) => {
21-
if e.kind() == std::io::ErrorKind::NotFound {
22-
Err(AssetReaderError::NotFound(full_path))
23-
} else {
24-
Err(e.into())
25-
}
26-
}
27-
}
23+
})
2824
}
2925

30-
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
26+
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
3127
let meta_path = get_meta_path(path);
3228
let full_path = self.root_path.join(meta_path);
33-
match File::open(&full_path).await {
34-
Ok(file) => {
35-
let reader: Box<Reader> = Box::new(file);
36-
Ok(reader)
29+
File::open(&full_path).await.map_err(|e| {
30+
if e.kind() == std::io::ErrorKind::NotFound {
31+
AssetReaderError::NotFound(full_path)
32+
} else {
33+
e.into()
3734
}
38-
Err(e) => {
39-
if e.kind() == std::io::ErrorKind::NotFound {
40-
Err(AssetReaderError::NotFound(full_path))
41-
} else {
42-
Err(e.into())
43-
}
44-
}
45-
}
35+
})
4636
}
4737

4838
async fn read_directory<'a>(

crates/bevy_asset/src/io/file/sync_file_asset.rs

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,16 @@ impl AsyncSeek for FileReader {
4242
}
4343
}
4444

45+
impl Reader for FileReader {
46+
fn read_to_end<'a>(
47+
&'a mut self,
48+
buf: &'a mut Vec<u8>,
49+
) -> stackfuture::StackFuture<'a, std::io::Result<usize>, { crate::io::STACK_FUTURE_SIZE }>
50+
{
51+
stackfuture::StackFuture::from(async { self.0.read_to_end(buf) })
52+
}
53+
}
54+
4555
struct FileWriter(File);
4656

4757
impl AsyncWrite for FileWriter {
@@ -87,13 +97,10 @@ impl Stream for DirReader {
8797
}
8898

8999
impl AssetReader for FileAssetReader {
90-
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
100+
async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
91101
let full_path = self.root_path.join(path);
92102
match File::open(&full_path) {
93-
Ok(file) => {
94-
let reader: Box<Reader> = Box::new(FileReader(file));
95-
Ok(reader)
96-
}
103+
Ok(file) => Ok(FileReader(file)),
97104
Err(e) => {
98105
if e.kind() == std::io::ErrorKind::NotFound {
99106
Err(AssetReaderError::NotFound(full_path))
@@ -104,14 +111,11 @@ impl AssetReader for FileAssetReader {
104111
}
105112
}
106113

107-
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
114+
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
108115
let meta_path = get_meta_path(path);
109116
let full_path = self.root_path.join(meta_path);
110117
match File::open(&full_path) {
111-
Ok(file) => {
112-
let reader: Box<Reader> = Box::new(FileReader(file));
113-
Ok(reader)
114-
}
118+
Ok(file) => Ok(FileReader(file)),
115119
Err(e) => {
116120
if e.kind() == std::io::ErrorKind::NotFound {
117121
Err(AssetReaderError::NotFound(full_path))

crates/bevy_asset/src/io/gated.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ impl<R: AssetReader> GatedReader<R> {
5555
}
5656

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

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

crates/bevy_asset/src/io/memory.rs

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -277,29 +277,41 @@ impl AsyncSeek for DataReader {
277277
}
278278
}
279279

280+
impl Reader for DataReader {
281+
fn read_to_end<'a>(
282+
&'a mut self,
283+
buf: &'a mut Vec<u8>,
284+
) -> stackfuture::StackFuture<'a, std::io::Result<usize>, { super::STACK_FUTURE_SIZE }> {
285+
stackfuture::StackFuture::from(async {
286+
if self.bytes_read >= self.data.value().len() {
287+
Ok(0)
288+
} else {
289+
buf.extend_from_slice(&self.data.value()[self.bytes_read..]);
290+
let n = self.data.value().len() - self.bytes_read;
291+
self.bytes_read = self.data.value().len();
292+
Ok(n)
293+
}
294+
})
295+
}
296+
}
297+
280298
impl AssetReader for MemoryAssetReader {
281-
async fn read<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
299+
async fn read<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
282300
self.root
283301
.get_asset(path)
284-
.map(|data| {
285-
let reader: Box<Reader> = Box::new(DataReader {
286-
data,
287-
bytes_read: 0,
288-
});
289-
reader
302+
.map(|data| DataReader {
303+
data,
304+
bytes_read: 0,
290305
})
291306
.ok_or_else(|| AssetReaderError::NotFound(path.to_path_buf()))
292307
}
293308

294-
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<Box<Reader<'a>>, AssetReaderError> {
309+
async fn read_meta<'a>(&'a self, path: &'a Path) -> Result<impl Reader + 'a, AssetReaderError> {
295310
self.root
296311
.get_metadata(path)
297-
.map(|data| {
298-
let reader: Box<Reader> = Box::new(DataReader {
299-
data,
300-
bytes_read: 0,
301-
});
302-
reader
312+
.map(|data| DataReader {
313+
data,
314+
bytes_read: 0,
303315
})
304316
.ok_or_else(|| AssetReaderError::NotFound(path.to_path_buf()))
305317
}

0 commit comments

Comments
 (0)