Wasmtime GitHub notifications bot (Apr 27 2024 at 18:04): Wasmtime GitHub notifications bot (Apr 27 2024 at 18:04):KGrewal1 opened PR #8498 from KGrewal1:fxhash-to-rustchash to bytecodealliance:main:
This removes the internal code for fxhash, 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--- @fitzgen
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:05):KGrewal1 requested elliottt for a review on PR #8498.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:05):KGrewal1 requested wasmtime-compiler-reviewers for a review on PR #8498.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:05):KGrewal1 requested wasmtime-default-reviewers for a review on PR #8498.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:08):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:08):bjorn3 created PR review comment:
There is no need to rename this package, right?
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:09):KGrewal1 updated PR #8498.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:13):KGrewal1 submitted PR review.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:13):KGrewal1 created PR review comment:
I actually made a mistake: fxhash specifically is currently needed as souper-harvest needs this hash convenience function which is not present in rustc-hash https://docs.rs/fxhash/0.2.1/fxhash/fn.hash.html .
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:13):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:13):bjorn3 created PR review comment:
I didn't mean undoing the migration, I mean using
rustc-hash = "1.1.0"instead offxhash = {package = "rustc-hash", version = "1.1.0"}.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:13):KGrewal1 submitted PR review.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:13):KGrewal1 created PR review comment:
I chose to rename though to minimise the size of the diff: appreciate if that is considered ugly and so not done
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:14):KGrewal1 edited PR #8498:
This removes the internal code for fxhash, <s>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<\s> --- @fitzgen
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:14):KGrewal1 edited PR #8498:
This removes the internal code for fxhash and instead uses the fxhash crate <s>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<\s> --- @fitzgen
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:16):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:16):bjorn3 created PR review comment:
I actually made a mistake: fxhash specifically is currently needed as souper-harvest needs this hash convenience function which is not present in rustc-hash
Inlining the convenience function is fine I think.
I chose to rename though to minimise the size of the diff: appreciate if that is considered ugly and so not done
Doing the move in a separate commit without renaming would still be easy to review.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:24):KGrewal1 updated PR #8498.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:25):KGrewal1 edited PR #8498:
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
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:25):KGrewal1 submitted PR review.
Wasmtime GitHub notifications bot (Apr 27 2024 at 18:25):KGrewal1 created PR review comment:
Have now done this
Wasmtime GitHub notifications bot (Apr 29 2024 at 15:19):fitzgen submitted PR review:
Thanks! I'll do the audit and merge this as well in another PR. Will link to it when it is ready.
Wasmtime GitHub notifications bot (Apr 29 2024 at 15:25):fitzgen commented on PR #8498:
Ah, looks like the Firefox folks kindly already audited
rustc-hashso we can just go ahead and merge this!
Wasmtime GitHub notifications bot (Apr 29 2024 at 15:48):fitzgen merged PR #8498.
Wasmtime GitHub notifications bot (May 21 2024 at 18:22):scottmcm submitted PR review.
Wasmtime GitHub notifications bot (May 21 2024 at 18:22):scottmcm created PR review comment:
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
u64instead ofusize.
Wasmtime GitHub notifications bot (May 21 2024 at 20:26):KGrewal1 submitted PR review.
Wasmtime GitHub notifications bot (May 21 2024 at 20:26):KGrewal1 created PR review comment:
Looking at the code I think that would work (and MSRV is 1.75)? Can't see any reason that it'd need to be usize, if anyone else knows more about it than me?
Wasmtime GitHub notifications bot (May 21 2024 at 20:33):KGrewal1 edited PR review comment.
Wasmtime GitHub notifications bot (May 21 2024 at 21:22):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (May 21 2024 at 21:22):fitzgen created PR review comment:
u64would indeed be fine.
Wasmtime GitHub notifications bot (May 22 2024 at 18:25):scottmcm submitted PR review.
Wasmtime GitHub notifications bot (May 22 2024 at 18:25):scottmcm created PR review comment:
To help avoid this problem in future I've gone and added an
FxBuildHasherto rustc-hash,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.
Last updated: Nov 22 2024 at 16:03 UTC