[perf] Change stable hasher to Blake3 #127754
Closed
Conversation
This file contains hidden or 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 Member Author
| @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 15, 2024
[perf] Change stable hasher to Blake3 Based on rust-lang/rustc-stable-hash@main...Urgau:rustc-stable-hash:blake3 cc `@michaelwoerister` r? ghost
Collaborator
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
| 💔 Test failed - checks-actions |
bors added S-waiting-on-author Member Author
| @bors try |
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2024
[perf] Change stable hasher to Blake3 Based on rust-lang/rustc-stable-hash@main...Urgau:rustc-stable-hash:blake3 cc `@michaelwoerister` r? ghost
Collaborator
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
| 💔 Test failed - checks-actions |
Member Author
| I have no idea about what's happening. stage1 and stage2 seems fine, but directly after Bolt-ing LLVM we
|
Member
| Is it possible that the Blake3 crate uses some low-level feature that BOLT does not support? |
Member Author
It's quite possible, they have some hand-rolled assembly and some C code, which we are probably using due to the auto-detection in their Let's try without BOLT for now. The benchmarks won't be accurate, maybe +10% overall, but at least will know if the benchmarks go up way too much. @bors try |
Collaborator
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2024
[perf] Change stable hasher to Blake3 Based on rust-lang/rustc-stable-hash@main...Urgau:rustc-stable-hash:blake3 cc `@michaelwoerister` r? ghost
Collaborator
| The job Click to see the possible cause of the failure (guessed by this bot) |
Collaborator
| ☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Collaborator
| Finished benchmarking commit (6f5329c): comparison URL. Overall result: ❌✅ regressions and improvements - 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 5.0%, secondary 8.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.
CyclesResults (primary 20.1%, secondary 19.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.
Binary sizeResults (primary -0.0%, secondary -0.0%)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.525s -> 738.599s (5.44%) |
rustbot added perf-regression Member Author
| We are seeing regressions up to 122% for secondary benchmarks and up to 62% in primary benchmarks with a mean 26.2% regressions. Looking at the BOLT-ing of LLVM alone (2 years ago) it didn't even have a impact on instructions directly, granted many things have changed; and while BOLT-ing Those regressions seems to indicate that Blake 3 is not a suitable replacement for our SipHasher for stable hashing of small structs. It might perform much better with bigger inputs (like files), at least that's what is being suggested with their own benchmarks showing a 16Kib input. In general it seems like Blake 3 is throughput-optimized, while for stable hashing, latency is the most important. It even has a 1024-bit fixed-input buffer, which probably explains why max-rss was so dearly affected. |
Member
| To have more up to date results than 2 years ago, as bolt has also evolved since then, you could also do a perf run with only the commit disabling it. |
Member
Member Author
| Based on the results above and this article, I'm trying Blake2s in #127809. |
Member Author
| Given the results above (even taking a considerable BOLT margin), Blake3 is probably a dead-end for our use-case. Closing. |
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
8 participants
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:blake3
cc @michaelwoerister
r? ghost