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

Fxhash to rustchash #8498

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Conversation

KGrewal1
Copy link
Contributor

@KGrewal1 KGrewal1 commented Apr 27, 2024

This removes the internal code for fxhash and instead uses the fxhash crate and subsequently migrates use of the fxhash crate (https://crates.io/crates/fxhash) to the rustc-hash crate (https://crates.io/crates/rustc-hash) implementing the same hash function: I believe the latter will need to be audited however: also manually implement the hash convenience function from fxhash --- @fitzgen

@KGrewal1 KGrewal1 requested review from a team as code owners April 27, 2024 18:04
@KGrewal1 KGrewal1 requested review from elliottt and removed request for a team April 27, 2024 18:04
Cargo.toml Outdated Show resolved Hide resolved
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. labels Apr 27, 2024
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll do the audit and merge this as well in another PR. Will link to it when it is ready.

@fitzgen
Copy link
Member

fitzgen commented Apr 29, 2024

Ah, looks like the Firefox folks kindly already audited rustc-hash so we can just go ahead and merge this!

@fitzgen fitzgen added this pull request to the merge queue Apr 29, 2024
Merged via the queue into bytecodealliance:main with commit 132ef1e Apr 29, 2024
21 checks passed

/// A convenience functon for a quick usize hash
#[inline]
pub fn hash<T: std::hash::Hash + ?Sized>(v: &T) -> usize {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your MSRV is at least 1.71, you might be interested in https://doc.rust-lang.org/std/hash/trait.BuildHasher.html#method.hash_one for this, though that gives u64 instead of usize.

Copy link
Contributor Author

@KGrewal1 KGrewal1 May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code I think that would work (and MSRV is 1.75)? Edit: doesn't seem possible due to FxHasher not having the BuildHasher trait

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

u64 would indeed be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To help avoid this problem in future I've gone and added an FxBuildHasher to rustc-hash,

https://github.com/rust-lang/rustc-hash/blob/7ab3ddd1c3364a8bb41d5bc4d89f5de4a390c196/src/lib.rs#L142-L150

I'll try to remember to come back here and simplify #8677 once there's a new release of rustc-hash on crates-dot-io to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants