-
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
[Performance run] Switch rustc_hash -> ahash #78218
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @ghost |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 82d48f974c8e7e1aa41e046a15fe1156838dbed4 with merge 9e97a2afa35ac40a2800d40d03720c154b6b8b90... |
This was previously tried in #59592 |
@workingjubilee that PR doesn't compile; it never went through rust-timer. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-actions |
Impl-wise, I wonder if it would be easier to publish EDITM: hm, I am not actually sure if patch allows replacing crates... |
Hmm the failure is weird - this is on an old branch, maybe I need to rebase over an llvm update or something?
Building locally now to make sure it isn't some weirdness with CI. |
I'm getting errors locally, too. Not sure what's going on.
|
Does ahash pass the std tests ? I guess ahash produce different hash result than siphash. |
@lzutao there's no way to know; this error is while building libstd, and I can't run tests until the library is built. |
That totally looks like OOM kill. |
☔ The latest upstream changes (presumably #78528) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
This was previously tried in #69153 (comment) and led to 1-4% regressions. |
@jyn514 That PR was prior to support for specialization which made significant performance improvements. |
This still uses std::collections::HashMap since there were a lot of issues with AHashMap. This also enables the `specialize` feature in hopes of getting better performance.
I updated this to use Does anyone know the difference between @bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 4e8aae3 with merge bea8ef10039b512489034f1fa2590703aebc8582... |
💔 Test failed - checks-actions |
The CI failure looks like a segfault, which is weird - maybe it's using a lot more memory than FxHasher? Locally it panics instead:
Backtrace
|
I don't plan to follow up on this. @tkaitchuck let me know if you want help getting this to work - I'm not sure exactly what's going wrong with the CI failure, but I can answer general questions about rustc. |
cc tkaitchuck/aHash#53, #77996, @tkaitchuck