Replace hash maps with fast hash maps #2150
Conversation
grovesNL
changed the title
grovesNL
changed the title
grovesNL
changed the title
grovesNL
changed the title
|
Using |
src/backend/metal/src/device.rs
Outdated
| @@ -6,17 +6,19 @@ use {conversions as conv, command, native as n}; | |||
| use native; | |||
|
|
|||
| use std::borrow::Borrow; | |||
| use std::collections::hash_map::{Entry, HashMap}; | |||
| use std::collections::HashMap; | |||
There was a problem hiding this comment.
why this change? it's the same as the old code but more verbose
There was a problem hiding this comment.
Yeah I need to combine these again. Originally HashMap was removed entirely then I realized we still need it for the spirv_cross overrides
src/hal/src/lib.rs
Outdated
| @@ -55,6 +56,9 @@ pub mod queue; | |||
| pub mod range; | |||
| pub mod window; | |||
|
|
|||
| #[doc(hidden)] | |||
| pub mod auxil; | |||
There was a problem hiding this comment.
actually, it appears to me that backend has a very similar goal. Should we move the fast hashmap into backend?
src/backend/metal/src/lib.rs
Outdated
| @@ -12,6 +12,7 @@ extern crate core_graphics; | |||
| extern crate block; | |||
| extern crate smallvec; | |||
| extern crate spirv_cross; | |||
| extern crate fxhash; | |||
There was a problem hiding this comment.
why does Metal need this explicitly? I'd assume it to just use the types exported from hal
There was a problem hiding this comment.
You're right, just left it there accidentally
|
CI failure seems unrelated, I'll rebase now |
|
thanks! |
2150: Replace hash maps with fast hash maps r=kvark a=grovesNL Fixes #2148 PR checklist: - [X] `make` succeeds (on *nix) - [X] `make reftests` succeeds - [X] tested examples with the following backends: Metal/GL/DX12/Vulkan Changes: - Replaced `HashMap` with `FastHashMap` type (uses `FxHash`) - Changed `Queues` to use a `Vec` instead of `HashMap`. It doesn't seem like a `HashMap` is necessary here, and the family ID is already present within the queue group, if I understand correctly. But this seems trivial overall because this shouldn't be a hot path ever. - mtl: Changed bindings to use a `Vec` (holey, filled with potentially a lot of `None`) indexed by binding. Not sure if this is the best approach but it should provide fast iteration and lookup by binding Overall this seems to give a pretty good improvement in Dota 2 as far as I can tell from profiling. I don't have precise benchmarks. Co-authored-by: Joshua Groves <[email protected]>
Fixes #2148
PR checklist:
makesucceeds (on *nix)make reftestssucceedsChanges:
HashMapwithFastHashMaptype (usesFxHash)Queuesto use aVecinstead ofHashMap. It doesn't seem like aHashMapis necessary here, and the family ID is already present within the queue group, if I understand correctly. But this seems trivial overall because this shouldn't be a hot path ever.Vec(holey, filled with potentially a lot ofNone) indexed by binding. Not sure if this is the best approach but it should provide fast iteration and lookup by bindingOverall this seems to give a pretty good improvement in Dota 2 as far as I can tell from profiling. I don't have precise benchmarks.