Cheaper dump_mir
#105083
Cheaper dump_mir
#105083
Conversation
`dump_mir_for_pass` and `dump_mir_for_phase_change` do some allocations even when `-Zdump-mir` isn't used, and the cost is enough to be noticeable. This commit rearranges things to avoid the cost. - `dump_mir` now dumps MIR unconditionally. - The new `maybe_dump_mir` functions dumps MIR if `-Zdump-mir` is set appropriately. - `dump_enabled` is replaced with `pass_name_matches_dump_filters`, which just does the second part of the test. - Most places use `maybe_dump_mir`, but `dump_mir_for_pass` and `dump_mir_for_phase_change` do their own tests to avoid the allocations in the common cases where no MIR dumping occurs. This is not the most elegant code ever written, but hopefully it's good enough. I also tried using closures for `pass_num` and `pass_name` but that ended up being uglier than this approach.
It has a single call site.
|
Failed to set assignee to
|
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
Best reviewed one commit at a time. Commit logs explain what's going on. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 4e61953 with merge ba96534cdc0ce369c29afc119e717324c5c88c44... |
| let Some(ref filters) = tcx.sess.opts.unstable_opts.dump_mir else { | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
is it actually perf relevant to do this at every call site instead of inside the function?
There was a problem hiding this comment.
It is, as the perf CI results show.
There is a two-step check when deciding whether to dump MIR.
- Is
-Zdump-mirspecified? - If so, does the MIR pass name match the
-Zdump-mirvalue?
If both those conditions pass, then the MIR is dumped, which involves printing pass_name and pass_num.
For most of the dump_mir callsites pass_name and pass_num are simple scalars, which poses no problem.
But at at two of the dump_mir call sites (in dump_mir_for_phase_change and dump_mir_for_pass) pass_name and pass_num are constructed in a way that is moderately expensive, and this happens before the two-step check.
The PR changes things so that, at two of these call sites, pass_name is only constructed if the first part of the two-step check succeeds, and pass_num is only constructed if the second part of the two-step check succeeds.
There was a problem hiding this comment.
I tried building an alternative design (#105121) that afaict should end up with similar perf improvements.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ba96534cdc0ce369c29afc119e717324c5c88c44): 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)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. |
rustbot
added
perf-regression
|
Perf results are good. I think the |
…nnethercote Cheaper `dump_mir` take two alternative to rust-lang#105083 r? `@nnethercote`
|
Superseded by #105121. |
…nnethercote Cheaper `dump_mir` take two alternative to rust-lang#105083 r? `@nnethercote`
r? @oli-obk