Throttle render assets #12622

robtfm · 2024-03-21T15:45:00Z

Objective

allow throttling of gpu uploads to prevent choppy framerate when many textures/meshes are loaded in.

Solution

RenderAssets can implement byte_len() which reports their size. implemented this for Mesh and Image
users can add a RenderAssetBytesPerFrame which specifies max bytes to attempt to upload in a frame
render_assets::<A> checks how many bytes have been written before attempting to upload assets. the limit is a soft cap: assets will be written until the total has exceeded the cap, to ensure some forward progress every frame

notes:

this is a stopgap until we have multiple wgpu queues for proper streaming of data
requires more robust gpu image use #12606

issues

~~fonts sometimes only partially upload. i have no clue why, needs to be fixed~~ fixed now.
choosing the #bytes is tricky as it should be hardware / framerate dependent
many features are not tested (env maps, light probes, etc) - they won't break unless RenderAssetBytesPerFrame is explicitly used though

JMS55 · 2024-03-21T16:21:06Z

Is this about saving CPU time, or GPU/PCIE transfers?

For the former, we should move render asset preparation to background tasks (I wish we had a better way to mark systems as background instead of having to manually spawn tasks from a system). I have an issue open for this.

For the latter, yeah I think we need separate hardware queues. But I can see the value of this until then.

robtfm · 2024-03-21T16:23:10Z

it's about transfers. currently if you load a bunch of large renderassets the whole app stalls in prepare_asset::<A> until they're all done

robtfm · 2024-03-21T16:43:12Z

actually now you say that i'm not sure... prepare_asset<Image> uses create_texture_with_data which should just be submitting writes to the queue, so i don't know exactly why i see the stall point in prepare_asset<Image> itself. maybe it would be resolved using cpu-background uploading.

IceSentry · 2024-04-11T21:03:20Z

I just tried upgrading this to main. It's pretty simple, just need to do the same trick used for asset_usage where the param is Self::SourceAsset instead of self.

I tested it in a simple app that just spawns a bunch of helmets to see how it would behave and it doesn't seem to do anything. It still spawns every helmet at the same time with a major stutter. I never see the exhausted debug log. Maybe I broke something while porting it. The code seems like it should work though, so not sure what I'm missing.

crates/bevy_render/src/render_asset.rs

robtfm · 2024-04-11T22:44:15Z

thanks for updating it - just making sure, you did add the resource as well? something like app.insert_resource(RenderAssetBytesPerFrame::new(16777216)) ?

i definitely see the impact on 0.13 and can't see any reason it should have changed with the flip to implementing RenderAsset on targets ... although in my scenario images are by far the largest part of the problem, possibly i messed up the mesh size calculation.

Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>

IceSentry · 2024-04-12T02:29:00Z

Yeah, I did insert the resource, but I did it in a separate branch where I copied code manually so I may have missed something

crates/bevy_render/src/render_asset.rs

crates/bevy_render/src/mesh/mesh/mod.rs

crates/bevy_render/src/texture/image.rs

IceSentry · 2024-04-12T17:43:02Z

I tested again based on the branch before the changes and it did work, so I probably messed up something when I was copying the code.

With the fix I suggested all that will be needed is to merge main I think.

robtfm · 2024-04-12T17:44:21Z

ok cool, i'll go through properly at some point this weekend.

Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>

robtfm · 2024-04-20T13:03:31Z

i got a bit tangled up with the merge so i just applied the changes onto a new branch and merged it in (with a couple of tweaks).

i can't really test very well since my project is not on main - i tried with 20 copies of the flight helmet and it seems to throttle correctly according to the debug output, but its not really enough data to see the impact visually.

mockersf · 2024-04-20T14:27:20Z

tried on a project I have that loads 65536 meshes with their textures, this works as expected. Bevy freezes without setting RenderAssetBytesPerFrame, works fine with it. FPS is still bad because of the number unique meshes / textures, and you're not supposed to display all the high details one at the same time 😄

throttling.mp4

crates/bevy_render/src/render_asset.rs

Co-authored-by: François Mockers <francois.mockers@vleue.com>

JMS55 · 2024-04-20T16:06:03Z

@mockersf can we add this as a cargo example? I may work on further asset loading perf improvements in the future, and it would be amazing to have this as a test bench.

IceSentry

LGTM

Would be nice to have a large_asset_loading example or something like that, but I don't want to block this PR on that.

mockersf · 2024-04-22T21:45:31Z

@mockersf can we add this as a cargo example? I may work on further asset loading perf improvements in the future, and it would be amazing to have this as a test bench.

I can do that in a few days, but it'll definitely never be merged in Bevy given how ugly it is 😄
Everything is loaded from a 65MB 7zip file

Would be nice to have a large_asset_loading example or something like that, but I don't want to block this PR on that.

I can try to get something prettier (and smaller) than what I currently have later

JMS55 · 2024-04-25T03:17:19Z

crates/bevy_render/src/render_asset.rs

    let mut param = param.into_inner();
    let queued_assets = std::mem::take(&mut prepare_next_frame.assets);
    for (id, extracted_asset) in queued_assets {
-        if extracted_assets.removed.contains(&id) {
+        if extracted_assets.removed.contains(&id) || extracted_assets.added.contains(&id) {


What is the purpose of skipping newly added assets here?

This is the retry loop for assets that were previously extracted and were not ready in the previous frame. If the asset has been added again, there are now 2 versions and we don’t want to prepare the old one.

This was the source of the font bug I had where some characters would not render:

The code in prepare_sprites clears its cache on asset change events, then checks for presence in RenderAssets to repopulate the cache. If we prepare the old version then the code in the sprite module will see it as ready and will cache the previous version, resulting in the old asset being permanently bound for some glyphs.

Ahhh, makes sense. Could you add a comment about this?

JMS55 · 2024-04-25T03:17:38Z

crates/bevy_render/src/render_asset.rs

@@ -161,13 +170,15 @@ impl<A: RenderAsset> RenderAssetDependency for A {
 pub struct ExtractedAssets<A: RenderAsset> {
    extracted: Vec<(AssetId<A::SourceAsset>, A::SourceAsset)>,
    removed: Vec<AssetId<A::SourceAsset>>,
+    added: Vec<AssetId<A::SourceAsset>>,


Should be a HashSet over a Vec.

Good point, will change this and removed as well

# Objective - Since #12622 example `compute_shader_game_of_life` crashes ``` thread 'Compute Task Pool (2)' panicked at examples/shader/compute_shader_game_of_life.rs:137:65: called `Option::unwrap()` on a `None` value note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Encountered a panic in system `compute_shader_game_of_life::prepare_bind_group`! thread '<unnamed>' panicked at examples/shader/compute_shader_game_of_life.rs:254:34: Requested resource compute_shader_game_of_life::GameOfLifeImageBindGroups does not exist in the `World`. Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins. Encountered a panic in system `bevy_render::renderer::render_system`! ``` ## Solution - `exhausted()` now checks that there is a limit

robtfm added 2 commits March 21, 2024 15:32

throttle renderasset bytes

be68929

cleanup merge

e6ebbfe

robtfm added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Mar 21, 2024

robtfm added 5 commits March 21, 2024 17:02

oops

5974ef8

correct behaviour for assets reextracted while pending

4008b77

Merge remote-tracking branch 'upstream/main' into throttle-render-assets

3fc4241

make loops symmetrical

f601e62

remove unready assets

5f5119f

IceSentry reviewed Apr 11, 2024

View reviewed changes

crates/bevy_render/src/render_asset.rs Outdated Show resolved Hide resolved

IceSentry reviewed Apr 11, 2024

View reviewed changes

crates/bevy_render/src/render_asset.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

a8ece2d

Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>

IceSentry reviewed Apr 12, 2024

View reviewed changes

crates/bevy_render/src/render_asset.rs Outdated Show resolved Hide resolved

IceSentry reviewed Apr 12, 2024

View reviewed changes

crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved

IceSentry reviewed Apr 12, 2024

View reviewed changes

crates/bevy_render/src/texture/image.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

a3bd114

Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>

JMS55 added this to the 0.14 milestone Apr 18, 2024

robtfm added 3 commits April 20, 2024 13:57

redo

7227a33

Merge branch 'throttle-renderassets-2' into throttle-render-assets

92a6890

fix merge

339e96b

fmt

fa80135

fix doc link

5501346

mockersf reviewed Apr 20, 2024

View reviewed changes

crates/bevy_render/src/render_asset.rs Outdated Show resolved Hide resolved

mockersf mentioned this pull request Apr 20, 2024

extract at most 5000 assets of a kind per frame #9893

Closed

Update crates/bevy_render/src/render_asset.rs

cfb83ac

Co-authored-by: François Mockers <francois.mockers@vleue.com>

mockersf approved these changes Apr 20, 2024

View reviewed changes

IceSentry approved these changes Apr 22, 2024

View reviewed changes

IceSentry 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 Apr 23, 2024

JMS55 suggested changes Apr 25, 2024

View reviewed changes

use hashsets for added and removed

dc0a5bc

mockersf added this pull request to the merge queue Apr 26, 2024

Merged via the queue into bevyengine:main with commit 91a393a Apr 26, 2024
27 checks passed

mockersf mentioned this pull request Apr 27, 2024

asset throttling: don't be exhausted if there is no limit #13112

Merged

IceSentry added the C-Needs-Release-Note Work that should be called out in the blog due to impact label Apr 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throttle render assets #12622

Throttle render assets #12622

robtfm commented Mar 21, 2024 •

edited

JMS55 commented Mar 21, 2024

robtfm commented Mar 21, 2024

robtfm commented Mar 21, 2024

IceSentry commented Apr 11, 2024 •

edited

robtfm commented Apr 11, 2024

IceSentry commented Apr 12, 2024 •

edited

IceSentry commented Apr 12, 2024 •

edited

robtfm commented Apr 12, 2024

robtfm commented Apr 20, 2024

mockersf commented Apr 20, 2024

JMS55 commented Apr 20, 2024

IceSentry left a comment

mockersf commented Apr 22, 2024

JMS55 Apr 25, 2024

robtfm Apr 25, 2024

JMS55 Apr 25, 2024

JMS55 Apr 25, 2024

robtfm Apr 25, 2024

Throttle render assets #12622

Throttle render assets #12622

Conversation

robtfm commented Mar 21, 2024 • edited

Objective

Solution

JMS55 commented Mar 21, 2024

robtfm commented Mar 21, 2024

robtfm commented Mar 21, 2024

IceSentry commented Apr 11, 2024 • edited

robtfm commented Apr 11, 2024

IceSentry commented Apr 12, 2024 • edited

IceSentry commented Apr 12, 2024 • edited

robtfm commented Apr 12, 2024

robtfm commented Apr 20, 2024

mockersf commented Apr 20, 2024

JMS55 commented Apr 20, 2024

IceSentry left a comment

Choose a reason for hiding this comment

mockersf commented Apr 22, 2024

JMS55 Apr 25, 2024

Choose a reason for hiding this comment

robtfm Apr 25, 2024

Choose a reason for hiding this comment

JMS55 Apr 25, 2024

Choose a reason for hiding this comment

JMS55 Apr 25, 2024

Choose a reason for hiding this comment

robtfm Apr 25, 2024

Choose a reason for hiding this comment

robtfm commented Mar 21, 2024 •

edited

IceSentry commented Apr 11, 2024 •

edited

IceSentry commented Apr 12, 2024 •

edited

IceSentry commented Apr 12, 2024 •

edited