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

Added a dedicated HandleMap #5418

Closed
wants to merge 3 commits into from

Conversation

SarthakSingh31
Copy link
Contributor

@SarthakSingh31 SarthakSingh31 commented Jul 21, 2022

This version is nowhere ready to merge. I am mostly looking for people's opinion. Idk if I missed something obvious in the benchmark.

Objective

Add a HandleMap<K, V> to replace HashMap<Handle<K>, V>. Its goal is to increase performance by reducing the amount of data that needs to be hashed to access a value in a hash map.

A limitation of the current implementation of HandleMap<K, V> is that it can't store a strong handle whereas HashMap<Handle<K>, V> can. I don't know how often this capability is required.

Bench

Time taken to call .get 1,000,000 times:

HandleId::Id / Total IDs 0.0 0.2 0.4 0.6 0.8 1.0
HashMap<Handle<K>, V> 21.049 ms 23.680 ms 26.296 ms 28.023 ms 27.600 ms 28.015 ms
HandleMap<K, V> 16.563 ms 18.593 ms 20.135 ms 19.353 ms 16.935 ms 14.077 ms

@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR labels Jul 22, 2022
@alice-i-cecile
Copy link
Member

Controversial label for now until we can find some clear use cases :) Cool stuff though!

@nicopap
Copy link
Contributor

nicopap commented Jul 31, 2023

This would be useful for #5080. Given it relies on a HashMap<Handle<X>, u32>

@SarthakSingh31
Copy link
Contributor Author

All these other uses will probably also benefit from the HandleMap

crates/bevy_asset/src/debug_asset_server.rs
54:    pub handles: HashMap<Handle<T>, Handle<T>>,

crates/bevy_ui/src/render/mod.rs
782:    pub values: HashMap<Handle<Image>, BindGroup>,

crates/bevy_scene/src/scene_spawner.rs
33:    spawned_scenes: HashMap<Handle<Scene>, Vec<InstanceId>>,
34:    spawned_dynamic_scenes: HashMap<Handle<DynamicScene>, Vec<InstanceId>>,

crates/bevy_pbr/src/material.rs
586:pub struct RenderMaterials<T: Material>(pub HashMap<Handle<T>, PreparedMaterial<T>>);

crates/bevy_render/src/render_resource/pipeline_cache.rs
129:    data: HashMap<Handle<Shader>, ShaderData>,
130:    shaders: HashMap<Handle<Shader>, Shader>,
214:        shaders: &HashMap<Handle<Shader>, Shader>,

crates/bevy_render/src/render_asset.rs
124:pub struct RenderAssets<A: RenderAsset>(HashMap<Handle<A>, A::PreparedAsset>);

crates/bevy_sprite/src/texture_atlas.rs
23:    pub texture_handles: Option<HashMap<Handle<Image>, usize>>,

crates/bevy_sprite/src/render/mod.rs
480:    values: HashMap<Handle<Image>, BindGroup>,

crates/bevy_sprite/src/mesh2d/material.rs
451:pub struct RenderMaterials2d<T: Material2d>(HashMap<Handle<T>, PreparedMaterial2d<T>>);

@alice-i-cecile
Copy link
Member

A limitation of the current implementation of HandleMap<K, V> is that it can't store a strong handle whereas HashMap<Handle, V> can. I don't know how often this capability is required.

I use this in my games (it's nice to create a simple library of assets to load), but I'm 100% fine to just stay with a plain HashMap there.

@SarthakSingh31
Copy link
Contributor Author

@alice-i-cecile I think I can add storing strong handles. I will make that change and report back the performance difference.

1 similar comment
@SarthakSingh31
Copy link
Contributor Author

@alice-i-cecile I think I can add storing strong handles. I will make that change and report back the performance difference.

@SarthakSingh31
Copy link
Contributor Author

@alice-i-cecile This adds the ability to store strong handles. New benchmark:

HandleId::Id / Total IDs 0.0 0.2 0.4 0.6 0.8 1.0
HashMap<Handle<K>, V> 28.373 ms 26.712 ms 28.027 ms 31.026 ms 28.168 ms 27.011 ms
HandleMap<K, V> 21.654 ms 24.155 ms 26.356 ms 23.962 ms 20.926 ms 17.914 ms

@bas-ie
Copy link
Contributor

bas-ie commented Oct 4, 2024

Backlog cleanup: closing due to inactivity. Unlikely to benefit from a tracking issue here, on balance.

@bas-ie bas-ie closed this Oct 4, 2024
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-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants