-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[DO NOT MERGE] perf: test rustc-hash variant optimized for small strings. #120093
[DO NOT MERGE] perf: test rustc-hash variant optimized for small strings. #120093
Conversation
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi Some changes might have occurred in exhaustiveness checking cc @Nadrieril rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
This comment has been minimized.
This comment has been minimized.
You'll need to run |
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
I guess tidy doesn't like git dependencies. I checked it and it looks sane, maybe bors will allow it? @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…s, r=<try> [DO NOT MERGE] perf: test rustc-hash variant optimized for small strings. I would be glad if a perf run could be performed for the modification proposed in rust-lang/rustc-hash#30 which came out of FNV beating fxhash for small strings when used in Tantivy. (Note that this is not changing the structure of rustc-hash, it just tries to optimize the implementation for handling the remaining bytes below the word size which appears to dominate performance for short strings.)
Thank you! I could also force-push an amended version which also updates tidy's |
Oh yeah that would have been a saner solution x) Bors seems happy though, let's see what happens |
bors doesn't care about such lowly things as "tidy" or "code quality", it's fine |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (255c8f5): 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)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 665.735s -> 665.203s (-0.08%) |
Can I do this? @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
@Nilstrieb @Nadrieril It seems merging the hash rounds for 4, 2 and 1 remaining bytes decreases quality too much and hence yields regressions in rustc's benchmarks. The one regression mentioned in rust-lang/rustc-hash#30 which is most likely also due to this can be fixed by merging only the rounds for 2 and 1 remaining bytes but keeping the separate round for 4 remaining bytes as now proposed. Sadly, this again makes the resulting hash slower than FNV for very short strings so it does not really seem to solve the original problem. Could one of you nevertheless retrigger the perf run to verify that this understanding is correct? Sorry for wasting CI time and thank you! |
…s, r=<try> [DO NOT MERGE] perf: test rustc-hash variant optimized for small strings. I would be glad if a perf run could be performed for the modification proposed in rust-lang/rustc-hash#30 which came out of FNV beating fxhash for small strings when used in Tantivy. (Note that this is not changing the structure of rustc-hash, it just tries to optimize the implementation for handling the remaining bytes below the word size which appears to dominate performance for short strings.)
Funny that you'd have bors perms but not rust-timer perms, ask on Zulip for them if you want. I think we need a different invocation if we're not doing the combined bors+timer command. Let's see @rust-timer queue 376db7f |
This comment has been minimized.
This comment has been minimized.
Yeah, that's so I can self-approve rust-analyzer sync PRs, but I'm probably not supposed to r+ anything else. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@Nadrieril To benchmark an existing merge commit, it's |
Ah, I can't queue a given SHA? Do you think this'll work anyway? |
I profiled
That is precisely where I expected this PR to have an impact, so it's fair to say that for the workload of rustc, the first change decreased performance (but in a way that doesn't matter for most not-stress-test workloads). (The reason why I'm posting this detailed analysis is that rustc-perf can be noisy sometimes, and it's important to take a closer look like I just did) |
You can, but the command is |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
It's documented in the help page of perf: https://perf.rust-lang.org/help.html Should I also add it to the guide? Or maybe link to the help page there? |
Finished benchmarking commit (376db7f): 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 665.206s -> 665.092s (-0.02%) |
@Kobzol nice! yes, I think it's worth linking in the rustc dev guide |
Ok, so I guess this means that just scheduling changes to the tail part of fxhash will not help as fxhash really needs to extra rounds for quality. |
I would be glad if a perf run could be performed for the modification proposed in rust-lang/rustc-hash#30 which came out of FNV beating fxhash for small strings when used in Tantivy. (Note that this is not changing the structure of rustc-hash, it just tries to optimize the implementation for handling the remaining bytes below the word size which appears to dominate performance for short strings.)