Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy on Write AssetPaths #9729

Merged
merged 12 commits into from
Sep 9, 2023
Merged

Copy on Write AssetPaths #9729

merged 12 commits into from
Sep 9, 2023

Conversation

cart
Copy link
Member

@cart cart commented Sep 9, 2023

Objective

The AssetServer and AssetProcessor do a lot of AssetPath cloning (across many threads). To store the path on the handle, to store paths in dependency lists, to pass an owned path to the offloaded thread, to pass a path to the LoadContext, etc , etc. Cloning multiple string allocations multiple times like this will add up. It is worth optimizing this.

Referenced in #9714

Solution

Added a new CowArc<T> type to bevy_util, which behaves a lot like Cow<T>, but the Owned variant is an Arc<T>. Use this in place of Cow<str> and Cow<Path> on AssetPath.


Changelog

  • AssetPath now internally uses CowArc, making clone operations much cheaper
  • AssetPath now serializes as AssetPath("some_path.extension#Label") instead of as AssetPath { path: "some_path.extension", label: Some("Label) }

Migration Guide

// Old
AssetPath::new("logo.png", None);

// New
AssetPath::new("logo.png");

// Old
AssetPath::new("scene.gltf", Some("Mesh0");

// New
AssetPath::new("scene.gltf").with_label("Mesh0");

AssetPath now serializes as AssetPath("some_path.extension#Label") instead of as AssetPath { path: "some_path.extension", label: Some("Label) }

@cart cart added A-Assets Load files from disk to use for things like images, models, and sounds C-Performance A change motivated by improving speed, memory usage or compile times labels Sep 9, 2023
@cart cart added this to the 0.12 milestone Sep 9, 2023
@cart cart added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 9, 2023
@cart cart mentioned this pull request Sep 9, 2023
45 tasks
@alice-i-cecile
Copy link
Member

Reasonable enough, but it is a shame we don't have benchmarks or even stress tests for asset processing currently.

@cart
Copy link
Member Author

cart commented Sep 9, 2023

We have a pseudo stress test in the form of running the asset processor on everything in the assets folder. This is as simple as turning on AssetPlugin::processed_dev() in any example and running it. It will log the total duration. To repeat, just delete the imported_assets folder and run again.

I ran that about 10 times for both main and this pr before submitting it and the change here was dwarfed by very swingey results (my guess is filesystem i/o). We might need something a bit more targeted to isolate the perf difference.

@cart
Copy link
Member Author

cart commented Sep 9, 2023

However given that Arc<str> is essentially a referenced counted String, imo this really does come down to the cost of reallocating a string on clone vs incrementing an atomic index (which should definitely be a win in this case).

@alice-i-cecile
Copy link
Member

Yep, agreed with your instincts here. Thanks for testing that; I'll spin out an issue.

Copy link
Contributor

@tguichaoua tguichaoua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CowArc is missing Display and also Ord and PartialOrd to be complet. But since the last two are not used in bevy I think it's ok to omit them.

crates/bevy_asset/src/path.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@killercup killercup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense but would be good to see benchmarks, maybe also to see how much memory pressure is related to this.

I think there's a couple crates out there that try to make atomic string types as small as possible (at the cost of indirection). If this turns out to be performance critical, maybe some of their tricks can be implemented generically here, too.

crates/bevy_utils/src/cow_arc.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/path.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member Author

cart commented Sep 9, 2023

Something else interesting we could consider: add a CowArc::Static(&'static str) variant and change From<&'a str> to From<&'static>.

This would do two things:

  1. It would remove the need to produce owned / Arc-ed strings for static strings. Clones of these types become "just" a &'static str pointer copy, and we retain this context transparently as things like AssetPath::into_owned are called.
  2. It would force non-static &str to go through a different constructor (ex: manually call the AssetPath constructor for asset_server.load(AssetPath::new(some_non_static_str)). HOWWWEVER, we would still have From<&'a String>, which means asset_server.load(&some_string) would still work. This would only apply to explicit &str types, which are even less common.

The theory being that a very high percentage of paths defined in user code are static strs. AssetLoaders basically never use static strs, but every case in the built in Bevy AssetLoader code uses &String, so no changes were necessary.

But it would come at the cost of nasty ergonomics / confusing apis for borrowed strs.

It would also make CowArc much more opinionated. But this would likely save a lot of work on average and so far the UX hit seems pretty much non-existent.

I had to change zero Bevy examples or AssetLoader impls after the change.

@cart
Copy link
Member Author

cart commented Sep 9, 2023

I just pushed the change to illustrate. We can revert if y'all don't like it.

crates/bevy_asset/src/path.rs Show resolved Hide resolved
crates/bevy_utils/src/cow_arc.rs Outdated Show resolved Hide resolved
crates/bevy_utils/src/cow_arc.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 9, 2023
@cart cart added this pull request to the merge queue Sep 9, 2023
Merged via the queue into bevyengine:main with commit 17edf4f Sep 9, 2023
22 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Sep 13, 2023
# Objective

- Silence `Failed to send DropEvent for StrongHandle "SendError(..)"`
errors when `StrongHandle` were dropped during application shutdown.
- Re-export `BoxedFuture` considering that it's used everywhere in
bevy_asset
- Fixed an issue introduced by #9729. 
  ```
Asset 'final_gather.rgen' encountered an error in
dust_render::shader::spirv::SpirvLoader: Failed to deserialize asset
meta: SpannedError { code: InvalidValueForType { expected: "string
AssetPath", found: "a sequence" }, position: Position { line: 9, col: 24
} }
  ```
Basically, for processed assets with dependencies, bevy will serialize
the metafile as follows:
  ```
  (
    meta_format_version: "1.0",
    processed_info: Some((
hash: (203, 239, 108, 156, 180, 23, 157, 217, 159, 36, 158, 193, 185,
253, 242, 156),
full_hash: (77, 58, 30, 200, 21, 180, 221, 133, 151, 83, 247, 47, 193,
70, 228, 97),
        process_dependencies: [
            (
full_hash: (56, 46, 55, 118, 3, 6, 213, 250, 124, 26, 153, 87, 15, 85,
4, 89),
                path: ("standard.glsl"), # <<---------- See here
            ),
        ],
    )),
    asset: Load(
        loader: "dust_render::shader::spirv::SpirvLoader",
        settings: (),
    ),
  )
  ```
`AssetPath` gets serialized as `("standard.glsl")` which was then
deserialized as a sequence instead of our `AssetPath`.

## Solution

- Serialize `AssetPath` directly as a string instead. The above metafile
would be serialized as follows:
 ```
  (
    meta_format_version: "1.0",
    processed_info: Some((
hash: (203, 239, 108, 156, 180, 23, 157, 217, 159, 36, 158, 193, 185,
253, 242, 156),
full_hash: (77, 58, 30, 200, 21, 180, 221, 133, 151, 83, 247, 47, 193,
70, 228, 97),
        process_dependencies: [
            (
full_hash: (56, 46, 55, 118, 3, 6, 213, 250, 124, 26, 153, 87, 15, 85,
4, 89),
path: "standard.glsl", # <<------- No longer a tuple struct
            ),
        ],
    )),
    asset: Load(
        loader: "dust_render::shader::spirv::SpirvLoader",
        settings: (),
    ),
  )
  ```

---
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

The `AssetServer` and `AssetProcessor` do a lot of `AssetPath` cloning
(across many threads). To store the path on the handle, to store paths
in dependency lists, to pass an owned path to the offloaded thread, to
pass a path to the LoadContext, etc , etc. Cloning multiple string
allocations multiple times like this will add up. It is worth optimizing
this.

Referenced in bevyengine#9714 

## Solution

Added a new `CowArc<T>` type to `bevy_util`, which behaves a lot like
`Cow<T>`, but the Owned variant is an `Arc<T>`. Use this in place of
`Cow<str>` and `Cow<Path>` on `AssetPath`.

---

## Changelog

- `AssetPath` now internally uses `CowArc`, making clone operations much
cheaper
- `AssetPath` now serializes as `AssetPath("some_path.extension#Label")`
instead of as `AssetPath { path: "some_path.extension", label:
Some("Label) }`


## Migration Guide

```rust
// Old
AssetPath::new("logo.png", None);

// New
AssetPath::new("logo.png");

// Old
AssetPath::new("scene.gltf", Some("Mesh0");

// New
AssetPath::new("scene.gltf").with_label("Mesh0");
```

`AssetPath` now serializes as `AssetPath("some_path.extension#Label")`
instead of as `AssetPath { path: "some_path.extension", label:
Some("Label) }`

---------

Co-authored-by: Pascal Hertleif <killercup@gmail.com>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Silence `Failed to send DropEvent for StrongHandle "SendError(..)"`
errors when `StrongHandle` were dropped during application shutdown.
- Re-export `BoxedFuture` considering that it's used everywhere in
bevy_asset
- Fixed an issue introduced by bevyengine#9729. 
  ```
Asset 'final_gather.rgen' encountered an error in
dust_render::shader::spirv::SpirvLoader: Failed to deserialize asset
meta: SpannedError { code: InvalidValueForType { expected: "string
AssetPath", found: "a sequence" }, position: Position { line: 9, col: 24
} }
  ```
Basically, for processed assets with dependencies, bevy will serialize
the metafile as follows:
  ```
  (
    meta_format_version: "1.0",
    processed_info: Some((
hash: (203, 239, 108, 156, 180, 23, 157, 217, 159, 36, 158, 193, 185,
253, 242, 156),
full_hash: (77, 58, 30, 200, 21, 180, 221, 133, 151, 83, 247, 47, 193,
70, 228, 97),
        process_dependencies: [
            (
full_hash: (56, 46, 55, 118, 3, 6, 213, 250, 124, 26, 153, 87, 15, 85,
4, 89),
                path: ("standard.glsl"), # <<---------- See here
            ),
        ],
    )),
    asset: Load(
        loader: "dust_render::shader::spirv::SpirvLoader",
        settings: (),
    ),
  )
  ```
`AssetPath` gets serialized as `("standard.glsl")` which was then
deserialized as a sequence instead of our `AssetPath`.

## Solution

- Serialize `AssetPath` directly as a string instead. The above metafile
would be serialized as follows:
 ```
  (
    meta_format_version: "1.0",
    processed_info: Some((
hash: (203, 239, 108, 156, 180, 23, 157, 217, 159, 36, 158, 193, 185,
253, 242, 156),
full_hash: (77, 58, 30, 200, 21, 180, 221, 133, 151, 83, 247, 47, 193,
70, 228, 97),
        process_dependencies: [
            (
full_hash: (56, 46, 55, 118, 3, 6, 213, 250, 124, 26, 153, 87, 15, 85,
4, 89),
path: "standard.glsl", # <<------- No longer a tuple struct
            ),
        ],
    )),
    asset: Load(
        loader: "dust_render::shader::spirv::SpirvLoader",
        settings: (),
    ),
  )
  ```

---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants