Use a slotmap for render assets storage by superdump · Pull Request #13013 · bevyengine/bevy

superdump · 2024-04-18T05:54:11Z

Objective

Optimise material mesh entity rendering code paths
Prepare for 'draw stream' rendering which needs to pack render commands into a Vec<u32>

Solution

AssetId is quite large due to supporting a UUID variant and the type is used in very hot loops for batching and rendering. In rendering, we don't need the UUID. We can use a smaller id to refer to the prepared render asset.
Use a slotmap as the backing store for RenderAssets
Maintain a map from AssetId<A: RenderAsset> to slotmap key inside RenderAssets and use it to look up keys for known render assets at extraction time
Add a UpdatePendingRenderAssetKeyPlugin to help with updating asset keys on instance data for assets that were loaded during the current frame, to avoid a 1-frame lag.

Benchmarks

On an M1 Max, running cargo run --release --example bevymark -- --benchmark --waves 160 --per-wave 1000 --mode mesh2d. Yellow is main, red is this PR:

A 6.3% reduction in median frame time.

Main:

PR:

Noteworthy spans:

92.16ms -> 86.00ms : main_pass_2d
19.20ms -> 17.72mz : batch_and_prepare_sorted_render_phase
 6.07ms ->  4.34ms : queue_material2d_meshes
 0.95ms ->  1.71ms : extract_material_meshes_2d

Changelog

Added: RenderAssetKey slotmap key type to be used with RenderAssets
Added: get_key, get_with_key, and get_with_key_mut methods to RenderAssets to use slotmap keys.
Added: UpdatePendingRenderAssetKeyPlugin to help with updating asset keys on instance data for assets that were loaded during the current frame, to avoid a 1-frame lag.
Changed: Use slotmap for internal storage for RenderAssets for improved performance due to the smaller asset reference data size of slotmap keys compared to AssetId

Smaller keys!

crates/bevy_pbr/src/light_probe/mod.rs

superdump · 2024-04-18T06:00:49Z

crates/bevy_pbr/src/light_probe/environment_map.rs

-            diffuse: item.diffuse_map.id(),
-            specular: item.specular_map.id(),
-        })
+pub fn update_pending_environment_maps(


I did want this to use the plugin, but I feel like it's an exception, and I don't want to make the trait for the helper plugin much more complicated just to cover this case. Not certain it's the right choice though.

superdump · 2024-04-18T06:01:40Z

crates/bevy_pbr/src/material.rs

    }
 }

+pub fn extract_visible_material_instances<M: Material>(


Maybe this simple extraction case can be pluginified.

crates/bevy_pbr/src/prepass/mod.rs

crates/bevy_pbr/src/light_probe/irradiance_volume.rs

crates/bevy_pbr/src/material.rs

crates/bevy_pbr/src/render/light.rs

crates/bevy_pbr/src/render/mesh.rs

crates/bevy_render/src/extract_instances.rs

crates/bevy_render/src/render_asset.rs

JMS55

Not the cleanest solution, but it's not awful either. I don't love that the extract time goes up so much, nor the extra complexity involved. I won't block on it, but we could use more docs around the logic in the various systems. There's several nested conditions which makes understanding the code at a glance difficult.

I think there's a couple of questions worth asking:

Can we remove AssetId::Uuid outright?
If we want to move RenderAsset preparation into the asset loader directly, mostly for images in the context of this PR, will it be possible now that there's an extra layer of indirection for render assets?

We definitely should run the screenshot diff tool on all the examples before merging this.

JMS55 · 2024-04-20T05:06:41Z

crates/bevy_pbr/src/meshlet/material_draw_prepare.rs


-        for material_id in render_material_instances.values() {
-            let Some(material) = render_materials.get(*material_id) else {
+        for &material_key in render_material_instances.values() {


I have this fixed in a future PR by changing it to .values().collect::<HashSet<_>>(), but this code is currently bugged and adds too many materials, as it does not deduplicate between multiple entities with the same material (AssetId).

What this code really wants to do is iterate over every instance of material M that exists. I don't think we have a good way of doing this, even with these changes, right?

JMS55 · 2024-04-20T05:20:31Z

crates/bevy_pbr/src/material.rs

+) {
+    material_instances.clear();
+    for (entity, view_visibility, material) in &query {
+        if view_visibility.get() {


This isn't a change from how it is now, but meshlets uses RenderMaterialInstances. But, ViewVisibility should never get computed for MeshletMeshes?? It only gets updated for WithMesh entities, and it defaults to false afaiu. What am I missing here?

Hmm, the meshlet example works, so there doesn't seem to be any problem. I'm not sure how it works, but this also didn't really change anything versus main, other than to allow use of RenderAssetKey.

pcwalton · 2024-04-20T05:27:08Z

@cart Weren't you planning on removing AssetId::Uuid at some point?

superdump · 2024-04-21T22:40:05Z

I'm moving this PR to a draft. After the push back about the regression in extraction time, I went down a rabbit hole of changing Handle and AssetId to not be enums, instead to be structs and require always having an AssetIndex member, and optionally a Uuid. See Discord for details, but the short version is it performs even better than this PR, and it feels like a better solution if we can stomach not having support for compile-time const Handle.

superdump · 2024-04-27T12:36:58Z

Closing. After discussion with @cart we decided not to do this, but rather wait for the long-term solution.

Use a slotmap for render assets storage

47b5307

Smaller keys!