[perf] Change stable hasher to Blake2s #127809
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-testsuite
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 16, 2024
[perf] Change stable hasher to Blake2s Based on rust-lang/rustc-stable-hash@main...Urgau:rustc-stable-hash:blake2 With inputs from https://jszym.com/blog/short_input_hash/ and rust-lang#127754. cc `@michaelwoerister` `@Kobzol` r? ghost
|
Given that blake is a cryptographic hash, any subslice of the hash should also be a good quality hash. So going to 128 or 64 bit hashes should not need to use the whole value. |
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (4172c7c): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 0.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 15.0%, secondary 19.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.1%, secondary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 700.256s -> 708.594s (1.19%) |
rustbot
added
perf-regression
|
Well, at least max RSS didn't regress now... |
|
Unfortunately it seems like Blake2s is also not fast enough for our use-case. Therefore closing. |
|
Do we need all uses of StableHasher to be collision free? I did expect some uses of StableHasher to need the hashes to be consistent between compilation sessions (thus using StableHasher), but not depend on collision freedom for correctness. |
That's measured in python. According to the related HN discussion those benchmarks are dominated by python function call overhead. So they're probably not a good indicator for hashing performance of a native rust impl. To get good cryptographic performance for short inputs we probably need something specialized for that purpose that makes use of hardware acceleration like Haraka (rust impl; found on cryptography exchange) . But such constructions are fairly new and not mainstream, so their cryptographic claims aren't battle-tested. |
I don't know; @michaelwoerister probably knows better than me about that.
Yes, that's a valid point, and I agree that those absolute numbers are not to be trusted, but I still think that even taking into account the different overhead due to the bindings and python, that the overall ranking is still IMO informative.
Interesting, I will read more about it. |
Yes, we want stable hashes to be virtually unique. |
|
There is a Blake2s Rust implementation that is hardware accelerated (SSE4.1 and AVX2), |
|
Haraka looks interesting. One thing to keep in mind is that we need the hash values to also be the same independent of platform. I.e. when cross compiling libstd, we need hashes computed on the host to match what will later be computed when the compiler is running on the target. If the target does not have an optimized implementation but still produces the same hash value, that would be OK though. |
To elaborate on that: there might cases like that but at least for DepNode, DefPathHash, StableCrateId, legacy-symbol-name hashes, and incr. comp. query result hashing we need there to be no collisions. For the first three of these we check for collisions and have never found one. Overall, I prefer to just give the guarantee of virtual uniqueness for stable hashes. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 17, 2024
[perf] Change stable hasher to Blake2s Based on rust-lang/rustc-stable-hash@main...Urgau:rustc-stable-hash:blake2 With inputs from https://jszym.com/blog/short_input_hash/ and rust-lang#127754. cc `@michaelwoerister` `@Kobzol` r? ghost
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (6f7ba5f): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 2.6%, secondary 7.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 51.4%, secondary 44.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.0%, secondary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 768.642s -> 777.99s (1.22%) |
rustbot
removed
the
S-waiting-on-perf
we build for x64-v1, so the hardware acceleration may incur also runtime checks |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on rust-lang/rustc-stable-hash@main...Urgau:rustc-stable-hash:blake2
With inputs from https://jszym.com/blog/short_input_hash/ and #127754.
cc @michaelwoerister @Kobzol
r? ghost