Skip to content

Reconsider Choice of HashMap Hasher #6999

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

Open
brody4hire opened this issue Jan 27, 2025 · 4 comments
Open

Reconsider Choice of HashMap Hasher #6999

brody4hire opened this issue Jan 27, 2025 · 4 comments
Labels
area: performance How fast things go type: enhancement New feature or request

Comments

@brody4hire
Copy link
Contributor

brody4hire commented Jan 27, 2025

As discussed for changes proposed in #6938 - #6938 (comment): this project has been using some kind of "fast" hasher from rustc-hash v1, which we are now planning to use with hashbrown, and I have identified some other possible hashers to recommend evaluating:

This kind of decision should be backed up with some benchmark testing.

Context: PR #6938 is part of some preparation I am contributing for no-std support, as requested in: #6826


P.S. I found some older context here, which I would like to comment on:

  • Both fxhash and rustc-hash are in the dependency tree #3369 / PR Replace fxhash with rustc-hash #3502 - it looks to me like the old fxhash crate was some kind of copy of the hasher from rustc-hash. I think it was 100% 200% correct to drop use of the old fxhash in favor of rustc-hash. My one issue with rustc-hash is that it seems to contain both FxHasher, which can be used with hashbrown, and FxHashMap / FxHashSet typedefs which are hard-coded to using std::collections. So moving forward, we should use rustc_hash::FxHasher but not rustc_hash::FxHashMap or rustc_hash::FxHashSet.

P.S. 2: Considering that this could affect API, as Fitzgerald noted with the label, I start to wonder if this could have serious impact on any external users?

@cwfitzgerald cwfitzgerald added type: enhancement New feature or request area: api Issues related to API surface labels Jan 27, 2025
@cwfitzgerald cwfitzgerald changed the title reconsider hasher(s) Reconsider Choice of HashMap Hasher Jan 27, 2025
@cwfitzgerald cwfitzgerald added area: performance How fast things go and removed area: api Issues related to API surface labels Jan 27, 2025
@cwfitzgerald
Copy link
Member

I think I just misclassified this, this isn't exposed to the user.

@a1phyr
Copy link
Contributor

a1phyr commented Jan 29, 2025

foldhash is the new default hasher from hashbrown, it may also be worth considering.

@cwfitzgerald
Copy link
Member

I think the default hashbrown hasher is a very sensible default.

@brody4hire
Copy link
Contributor Author

https://github.com/xacrimon/dashmap - found this in my GitHub recommendations FWIW

superdump pushed a commit to bevyengine/naga_oil that referenced this issue Mar 22, 2025
`rustc-hash` can't be updated for now because naga/wgpu is still on 1.1
and it's used in some public naga structs that we use
gfx-rs/wgpu#6999

Closes #114
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: performance How fast things go type: enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

3 participants