Try Carbon's new hashing strategy in the compiler #117079
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
S-waiting-on-review
|
@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
Oct 23, 2023
Try Carbon's new hashing strategy in the compiler A hopefully correct port of carbon-language/carbon-lang#3327 Carbon's hashing doesn't seem to use the "hash each compoment of the struct" strategy the way Rust does, but we may still benefit from it. the compiler bootstraps locally and doesn't take too much time, so it must be good! it even passes UI tests, except the ones that seem to iterate over hashmaps >.< r? `@ghost`
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
|
@rust-timer build f06e258 |
|
Shouldn't all of those functions have a |
yes, they should, i forgot about that. that said, PGO+ThinLTO should hopefully inline them anyways. but since perf is stuck, i'm just gonna add them now to be sure |
It's not needed everywhere, but just to be sure that I didn't forget anything.
|
@bors try |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 23, 2023
Try Carbon's new hashing strategy in the compiler A hopefully correct port of carbon-language/carbon-lang#3327 Carbon's hashing doesn't seem to use the "hash each compoment of the struct" strategy the way Rust does, but we may still benefit from it. the compiler bootstraps locally and doesn't take too much time, so it must be good! it even passes UI tests, except the ones that seem to iterate over hashmaps >.< r? `@ghost`
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@rust-timer build f06e258 |
This comment was marked as resolved.
This comment was marked as resolved.
|
☀️ Try build successful - checks-actions |
|
@rust-timer build 53d3149 |
|
I saw the right commit in the queue before but now it's the old one again. |
|
Still not correct. Whatever, let's just wait for the commit to come through. Maybe thinlto did everything correctly and we'll get something anyways. I will build the second commit afterwards. |
chandlerc
reviewed
Oct 23, 2023
There was a problem hiding this comment.
Heh, that was fast. =D
Two comments, that may be obvious, but just to be sure... First, you'll need to be using a hash table like HashBrown for this to work well in practice. And two, if Fx is just an xor hash (which is all I saw?) then it'll certainly be faster. That's even further on the "speed but forget quality" spectrum than this version.
| self.hash_bytes_large(bytes); | ||
| } | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
I'd encourage this to not be inline FWIW. The code is quite large and doesn't seem to benefit much from it.
The
https://github.com/rust-lang/rustc-hash/blob/786ccda70fce97a3177d6088f21a22ac7f0f2f85/src/lib.rs#L79 Basically for every input byte shift by a constant amount, xor by the input byte and then multiply by a constant. |
So, I expect that to be a bit faster for small inputs and slower for large ones. But I also expect that to produce very bad results when hashing common cases like pointers. Not just "low quality" but "extraordinarily low quality" unless there is some other shifting or rotating done to pointers. And if there is, then you can get rid of that with this hash function and likely have it be same-or-faster. |
|
We don't often hash large inputs (compared to how often we hash u32s for example ^^). There are more details about fxhash and how it's used, (some of) what was tried to improve it, and more in this post btw: https://nnethercote.github.io/2021/12/08/a-brutally-effective-hash-function-in-rust.html |
|
Finished benchmarking commit (f06e258): 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)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
rustbot
added
perf-regression
|
Wall time is probably the most relevant metric here as this hasher is supposed to have better latency. The results are very mixed: https://perf.rust-lang.org/compare.html?start=6bb4ad6dfb7d373c2f19b6cc8c0f05bd73f6d3cc&end=f06e2588ccd72e05fc8a5b13d79001f4087b525b&stat=wall-time&tab=compile&nonRelevant=true Some benchmarks have a two-digit improvement while others have a two-digit regression. On average I think it tends more towards a regression than an improvement. |
|
@rust-timer build 53d3149 Checking with the inline |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (53d3149): 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
|
instructions are negative, cycles are negative and wall time is noisy. just as you predicted, this is indeed slower. fxhash wins once again! I assume you mean shifting away the alignment bits, since they don't have any entropy? |
jackh726
added
S-experimental
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.
A hopefully correct port of carbon-language/carbon-lang#3327
Carbon's hashing doesn't seem to use the "hash each compoment of the struct" strategy the way Rust does, but we may still benefit from it.
the compiler bootstraps locally and doesn't take too much time, so it must be good! it even passes UI tests, except the ones that seem to iterate over hashmaps >.<
r? @ghost