-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Avoid sorting in hash map stable hashing #91837
Conversation
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue
Yeah, that's what I meant that it would probably need a |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit a5f5f6b with merge 4324513d2ed2011b78a1cf96c1ffddf672c4fe5c... |
You might want to use a modular addition instead. At least a theoretical issue with multiplies is that they have an absorbing element (zero). Although for u128 it's exceedingly unlikely to hit that element at random, assuming the hash function is good. |
This comment has been minimized.
This comment has been minimized.
a3841f0
to
2e695fd
Compare
This comment has been minimized.
This comment has been minimized.
2e695fd
to
6e33d3e
Compare
@bors try |
⌛ Trying commit 6e33d3e with merge 3c7d8f13f3bb90efbad4e21f0b2baf0bf274c42d... |
☀️ Try build successful - checks-actions |
Queued 3c7d8f13f3bb90efbad4e21f0b2baf0bf274c42d with parent 6bda5b3, future comparison URL. |
Finished benchmarking commit (3c7d8f13f3bb90efbad4e21f0b2baf0bf274c42d): comparison url. Summary: This change led to large relevant mixed results 🤷 in compiler performance.
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 led 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 |
Not too bad. I tried to use the same approach for more data structures, could I ask for another perf. run please? |
This comment has been minimized.
This comment has been minimized.
631e313
to
e86098b
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Testing commit 1f284b0 with merge aa63e5d0547b2df209cbcb4ce313c56abdf375a7... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
⌛ Testing commit 1f284b0 with merge 9fcd6dee4904d6ab8364f00d2bfca1a64f2e73f0... |
💥 Test timed out |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (daf2204): comparison url. Summary: This change led to large relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
Suggested by @the8472 here. I hope that I understood it right, I replaced the sort with modular multiplication, which should be commutative.
Can I ask for a perf. run? However, locally it didn't help at all. Creating the
StableHasher
all over again is probably slowing it down quite a lot. And usingFxHasher
is not straightforward, because the keys and values only implementHashStable
(and probably they shouldn't be just hashed viaHash
anyway for it to actually be stable).Maybe the
StableHash
interface could be changed somehow to better suppor these scenarios where the hasher is short-lived. Or theStableHasher
implementation could have variants with e.g. a shorter buffer for these scenarios.