-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Use XXH3 hash instead of SIP128 for stable hashing #97952
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit f12b0ed7d4798058c0f5dbf07d9ad6d8f2ebc185 with merge 2d3d00c03a8a9f87a5384ed886d9996e17316b96... |
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit cb2c47a94cc845ebf5d017e5901c85fd6cc7218b with merge 393f79804e7cd37b9b054bd261630dba7c969ded... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit fa77e8931eb3811d7ab3eeed9b8ae300f86628f0 with merge 88727198ddd5bd9f02c6d7ba34fc0b3e4b89b3ca... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
Queued 88727198ddd5bd9f02c6d7ba34fc0b3e4b89b3ca with parent ec55c61, future comparison URL. |
Finished benchmarking commit (88727198ddd5bd9f02c6d7ba34fc0b3e4b89b3ca): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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. @bors rollup=never Footnotes |
How do hashcollision chances between xxh3 and siphash compare? We need extremely low collision rates as any collision can cause a miscompilation. |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit ff1489a with merge 5e83f34b1fffab8706f81070e81f017f7f5b8dbe... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Try build successful - checks-actions |
Queued 5e83f34b1fffab8706f81070e81f017f7f5b8dbe with parent 744e397, future comparison URL. |
Finished benchmarking commit (5e83f34b1fffab8706f81070e81f017f7f5b8dbe): 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)ResultsThis 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.
CyclesResultsThis 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.
Footnotes |
I wonder if this is worth it. There is one big win on incr-unchanged for ctfe-stress-5, but other than that there are a lot of interleaved regressions and wins in instruction counts. Let's see how do cycles looks like once perf stabilises. |
The instruction count results are shrug, the cycle count results are amazing, the wall-time (and bootstrap) results are terrible. That's a very strange set of results. |
The parent commit here is the last one with the old machine configuration (per rust-lang/rustc-perf#1450), so everything other than instruction counts is probably misleading. @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit ff1489a with merge e1b19ca349a8408a331893f190193ad37686f845... |
☀️ Try build successful - checks-actions |
Queued e1b19ca349a8408a331893f190193ad37686f845 with parent a8a847e, future comparison URL. |
Finished benchmarking commit (e1b19ca349a8408a331893f190193ad37686f845): 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)ResultsThis 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.
CyclesResultsThis 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.
Footnotes |
I see that this is still assigned to me -- but I won't have time to help with this. The initial XXH3 port was a spare time project of mine, but others have to push it over the finish line, if there's interest to do so. From the performance results, it looks like nice but only modest wins (except for cases where large chunks of data are hashed in bulk). |
We have discussed it with @lqd and @nnethercote and I think that the decision was that it is not worth it to pursue this further. There are indeed some nice performance wins, but the code complexity is too high to justify switching to it, as the biggest wins are localized to niche benchmarks and there are also some regressions. |
Sounds good. I hope it was still an interesting investigation for everyone 🙂 |
Definitely! Now we know that the performance of hashing large blobs can be improved significantly (although it's probably not a bottleneck for most crates). |
This is an experiment to if XXH3 could be faster than SIP128 for incremental stable hashing.
r? @michaelwoerister