-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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 run for only mul/add-reordering parts of #136095 #136106
[DO NOT MERGE] perf run for only mul/add-reordering parts of #136095 #136106
Conversation
rustbot has assigned @Mark-Simulacrum. Use |
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 rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi Some changes occurred in exhaustiveness checking cc @Nadrieril These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…-changed-ordering-perf, r=<try> [DO NOT MERGE] perf run for only mul/add-reordering parts of rust-lang#136095 See rust-lang#136095 (comment) --- *For good comparability with the performance of rust-lang#136095, I'm keeping the `rustc-hasher` in `rustc_type_ir` at the "newly modified 2.*"… just noting this down in case it might be a difference from master that matters much.* --- ([link to the `rustc-hash` commits for context](rust-lang/rustc-hash@43e1790...1028035))
I'm not sure this'd fix the collisions issue in v2 🤔. I guess we can wait for perf but maybe you can also try it on the dataset from the issue. |
@lqd The point of this is the inverse; to see if the re-ordering part of the collisions fix was part of the performance-regression in #136095 this PR doesn't solve anything, but the goal is to get a better understanding & rule out potential oversights or misinterpretations, regarding the regressed performance observable in #136095's perf-run The collision fix used in #136095 kind-of requires the re-ordering trick anyway (if you don't want to spend time on an additional multiplication). This also means: In case it does turn out very relevant, such a potential insight of "the re-ordering was the problem" doesn't give any immediate solutions; but at least it could help avoid spending time&focus on the wrong details. |
I see, ok. |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c48f26f): comparison URL. Overall result: no relevant changes - no 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. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary 1.7%)This 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: 773.224s -> 772.081s (-0.15%) |
Good, this seems to confirm that there are no performance problems from the reordering, so it's indeed alright to just focus on the performance of the finalizer, in any potential variants of #136095 :-) |
See #136095 (comment)
For good comparability with the performance of #136095, I'm keeping the
rustc-hasher
inrustc_type_ir
at the "newly modified 2.*"… just noting this down in case it might be a difference from master that matters much.(link to the
rustc-hash
commits for context)