use `hashbrown` in more crates (etc.) #6938

brody4hire · 2025-01-17T16:21:17Z

Connections

helpful for no-std support as requested in issue: Support no_std where possible (specifically for wasm32v1-none) #6826
continuation of hashbrown updates introduced in PR: Start using hashbrown #6925

Description

use hashbrown in naga & top-level wgpu crates
completely replace indirect usage of std::collections::HashMap via rustc_hash::FxHashMap in wgpu-hal
use hashbrown in main wgpu crate
use hashbrown in deno_webgpu
use hashbrown with default-features = false, as needed to support no-std build
other minor updates

POSSIBLE API IMPACT

Exported naga API with these updates would be exporting types & members using HashMap & HashSet from hashbrown rather than std::collections. I think this is a crucial part of supporting no-std. I don't expect the blast radius to be very high, just wanted to point this out to avoid potential surprises for anyone.

/cc @bushrat011899

Testing

cargo test --all-features -p naga
cargo test --all-features -p wgpu-core
cargo test --all-features -p wgpu-hal
cargo test --all-features -p wgpu
cargo xtask test

Checklist

Run cargo fmt.
Run taplo format.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
- ~~--target wasm32-unknown-emscripten~~
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.
Resolve or defer XXX todo items in these updates

…rown-further

…n hashbrown::HashSet::contains call

naga/src/back/spv/writer.rs

naga/Cargo.toml

…rown-further

cwfitzgerald

Looking generally good, some comments!

cwfitzgerald · 2025-01-21T20:29:44Z

CHANGELOG.md

+##### Use `hashbrown` in `naga` & main `wgpu` crates
+
+Use `hashbrown` to simplify no-std support. (This may help improve performance as well.)
+
+By Brody in [#6938](https://github.com/gfx-rs/wgpu/pull/6938).
+


This actually applies to more than just this section - we generally reserve the larger "Major Changes" items for breaking changes or very heavy refactors. So like when no-std lands in the whole stack, that definitely will deserve a large item. But for this, a single line is fine as this is all internal and not user facing.

Makes total sense, I just pushed updates to compress all of this info & remove the unmeasured performance speculation. I have also squished the older hashbrown changelog update into here, out of the other place. I think this should be much cleaner.

Incidentally for small edits like this, I wouldn't mind if the reviewer wants to make the edit as a co-author if we want to save time going back-and-forth. Totally up to your best judgment on this.

Makes total sense, I just pushed updates to compress all of this info & remove the unmeasured performance speculation. I have also squished the older hashbrown changelog update into here, out of the other place. I think this should be much cleaner.

Nice

Incidentally for small edits like this, I wouldn't mind if the reviewer wants to make the edit as a co-author if we want to save time going back-and-forth. Totally up to your best judgment on this.

Yeah I do that for complex changes where explaining it is harder than just doing it :) For most cases though, commenting is easier than fully task switching to a new branch and getting out of "review" mode.

CHANGELOG.md

naga/src/back/spv/writer.rs

…rown-further

…ontains fn call

naga/src/back/spv/writer.rs

brody4hire · 2025-01-21T21:45:23Z

naga/src/lib.rs

+/// (Similar to rustc_hash::FxHashMap but using hashbrown::HashMap instead of std::collections::HashMap.)
+/// To construct a new instance: `FastHashMap::default()`
+pub type FastHashMap<K, T> =
+    hashbrown::HashMap<K, T, std::hash::BuildHasherDefault<rustc_hash::FxHasher>>;
+
 /// Hash set that is faster but not resilient to DoS attacks.
-pub type FastHashSet<K> = rustc_hash::FxHashSet<K>;
+/// (Similar to rustc_hash::FxHashSet but using hashbrown::HashSet instead of std::collections::HashMap.)
+pub type FastHashSet<K> =
+    hashbrown::HashSet<K, std::hash::BuildHasherDefault<rustc_hash::FxHasher>>;


I noticed that egui is using hashers from ahash & nohash-hasher, started wondering if either of these would be worth considering. I also noticed what looks like some more hashers listed in: https://github.com/RustCrypto/hashes

I am guessing we should defer consideration to the future, just wanted to note this before I forget.

Yeah probably. Using a special hasher likely needs to be supported by benchmarks anyway. I would be interested in seeing this investigated at some point. Could you make an issue?

bushrat011899

Nice! Splitting out these "controversial" changes into more bite-sized pieces is really nice to review.

bushrat011899 · 2025-01-21T21:41:05Z

Cargo.toml

+# (with default-features = false to help avoid using any features that may require std::collections & support no-std build)
+rustc-hash = { version = "1", default-features = false }


As an aside, I think libraries should generally use default-features = false anyway, since there is no way for a user to disable a feature if it's listed in a dependency, even if it's not actually being used.

cwfitzgerald

One last iteration. Might be worth setting up a hashmap on a banned type list to properly enforce this at least until we're fully checking non-std.

naga/src/back/spv/writer.rs

cwfitzgerald · 2025-01-22T02:09:35Z

naga/src/lib.rs

+/// (Similar to rustc_hash::FxHashMap but using hashbrown::HashMap instead of std::collections::HashMap.)
+/// To construct a new instance: `FastHashMap::default()`
+pub type FastHashMap<K, T> =
+    hashbrown::HashMap<K, T, std::hash::BuildHasherDefault<rustc_hash::FxHasher>>;
+
 /// Hash set that is faster but not resilient to DoS attacks.
-pub type FastHashSet<K> = rustc_hash::FxHashSet<K>;
+/// (Similar to rustc_hash::FxHashSet but using hashbrown::HashSet instead of std::collections::HashMap.)
+pub type FastHashSet<K> =
+    hashbrown::HashSet<K, std::hash::BuildHasherDefault<rustc_hash::FxHasher>>;


Yeah probably. Using a special hasher likely needs to be supported by benchmarks anyway. I would be interested in seeing this investigated at some point. Could you make an issue?

cwfitzgerald

Oop wrong button sorry 😅

Co-authored-by: Connor Fitzgerald <[email protected]>

use hashbrown in naga etc.

4c5e314

brody4hire mentioned this pull request Jan 17, 2025

no-std support in naga #6940

Draft

9 tasks

brody4hire added 5 commits January 18, 2025 20:08

Merge branch 'trunk' of https://github.com/gfx-rs/wgpu into use-hashb…

3ce7143

…rown-further

quick workaround for missing impl Arbitrary for hashbrown::HashMap

aacbd12

document a hint for FastHashMap type alias

fe48978

clippy fixup - apologies

e2ae32c

switch to hashbrown::HashSet - working with explicit type parameter i…

aabdd17

…n hashbrown::HashSet::contains call

brody4hire commented Jan 19, 2025

View reviewed changes

naga/src/back/spv/writer.rs Outdated Show resolved Hide resolved

brody4hire changed the title ~~use hashbrown in naga etc.~~ use hashbrown in naga & main wgpu crates` etc. Jan 19, 2025

brody4hire changed the title ~~use hashbrown in naga & main wgpu crates` etc.~~ use hashbrown in naga & main wgpu crates etc. Jan 19, 2025

brody4hire added 4 commits January 19, 2025 00:20

simplify fuzz workaround & some fuzz code in general

ece94a2

update import in naga/src/back/glsl/mod.rs

fbbdff3

fixup! update import in naga/src/back/glsl/mod.rs

afc0c79

update changelog

dd78a75

Wumpf reviewed Jan 19, 2025

View reviewed changes

naga/Cargo.toml Outdated Show resolved Hide resolved

brody4hire added 6 commits January 19, 2025 03:50

Merge branch 'trunk' of https://github.com/gfx-rs/wgpu into use-hashb…

0429b10

…rown-further

fixup naga/Cargo.toml - hopefully nothing fishy now

efb2edd

fixup again with apologies

f44a190

doh missed deserialize

96b1e50

fixup serde - wgpu-core/Cargo.toml

e63cfd1

taplo format fixup: naga/Cargo.toml

7f6b149

brody4hire marked this pull request as ready for review January 19, 2025 09:51

brody4hire requested review from a team and crowlKats as code owners January 19, 2025 09:51

brody4hire changed the title ~~use hashbrown in naga & main wgpu crates etc.~~ use hashbrown in naga & main wgpu crates, etc. Jan 19, 2025

brody4hire added 4 commits January 19, 2025 15:02

fixup rustc-hash dependency to support no-std build

f4ac45d

fixup toml fmt, my bad

d2813d2

add blank lines to naga/src/lib.rs

d9ba06b

update comment in top-level Cargo.toml

cb46cb9

update wgpu-hal/src/vulkan/mod.rs to use naga::FastHashMap

d74c64b

brody4hire requested a review from Wumpf January 20, 2025 04:53

brody4hire added 2 commits January 20, 2025 01:47

Merge branch 'trunk' into use-hashbrown-further

0066725

Merge branch 'trunk' of https://github.com/gfx-rs/wgpu into use-hashb…

fbd5eaf

…rown-further

cwfitzgerald requested changes Jan 21, 2025

View reviewed changes

Merge branch 'trunk' of https://github.com/gfx-rs/wgpu into use-hashb…

3f84acc

…rown-further

brody4hire changed the title ~~use hashbrown in naga & main wgpu crates, etc.~~ use hashbrown in more crates (etc.) Jan 21, 2025

brody4hire added 4 commits January 21, 2025 16:13

update & condense info in changelog

59ad1f0

Merge branch 'trunk' of https://github.com/gfx-rs/wgpu into use-hashb…

d59bd3d

…rown-further

redo update: update & condense info in changelog

ae3a970

add comment regarding explicit type needed with hashbrown::HashSet::c…

aef9a9f

…ontains fn call

brody4hire commented Jan 21, 2025

View reviewed changes

naga/src/back/spv/writer.rs Outdated Show resolved Hide resolved

brody4hire commented Jan 21, 2025

View reviewed changes

bushrat011899 approved these changes Jan 21, 2025

View reviewed changes

cwfitzgerald approved these changes Jan 22, 2025

View reviewed changes

cwfitzgerald requested changes Jan 22, 2025

View reviewed changes

Update writer.rs - remove extra parens from comment

db39e26

Co-authored-by: Connor Fitzgerald <[email protected]>

cwfitzgerald self-assigned this Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `hashbrown` in more crates (etc.) #6938

use `hashbrown` in more crates (etc.) #6938

brody4hire commented Jan 17, 2025 •

edited

Loading

cwfitzgerald left a comment

cwfitzgerald Jan 21, 2025

brody4hire Jan 21, 2025

cwfitzgerald Jan 21, 2025

brody4hire Jan 21, 2025

cwfitzgerald Jan 22, 2025

bushrat011899 left a comment

bushrat011899 Jan 21, 2025

cwfitzgerald left a comment

cwfitzgerald Jan 22, 2025

cwfitzgerald left a comment

		# (with default-features = false to help avoid using any features that may require std::collections & support no-std build)
		rustc-hash = { version = "1", default-features = false }

use hashbrown in more crates (etc.) #6938

Are you sure you want to change the base?

use hashbrown in more crates (etc.) #6938

Conversation

brody4hire commented Jan 17, 2025 • edited Loading

cwfitzgerald left a comment

Choose a reason for hiding this comment

cwfitzgerald Jan 21, 2025

Choose a reason for hiding this comment

brody4hire Jan 21, 2025

Choose a reason for hiding this comment

cwfitzgerald Jan 21, 2025

Choose a reason for hiding this comment

brody4hire Jan 21, 2025

Choose a reason for hiding this comment

cwfitzgerald Jan 22, 2025

Choose a reason for hiding this comment

bushrat011899 left a comment

Choose a reason for hiding this comment

bushrat011899 Jan 21, 2025

Choose a reason for hiding this comment

cwfitzgerald left a comment

Choose a reason for hiding this comment

cwfitzgerald Jan 22, 2025

Choose a reason for hiding this comment

cwfitzgerald left a comment

Choose a reason for hiding this comment

use `hashbrown` in more crates (etc.) #6938

use `hashbrown` in more crates (etc.) #6938

brody4hire commented Jan 17, 2025 •

edited

Loading