Shrink no-op drop elaboration - #157491
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
r? @mejrs rustbot has assigned @mejrs. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Shrink no-op drop elaboration
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0f76dc7): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.0%, secondary 0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 514.669s -> 516.17s (0.29%) |
|
r? me |
This comment was marked as low quality.
This comment was marked as low quality.
These would become simple gotos anyway, so save the cost of creating such blocks.
We are not modifying these blocks afterwards, so no need to create clean ones each time. Drive-by fix for async drops with enums.
c7f866c to
80705e8
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 5e91de6 (parent) -> 99576cf (this PR) Test differencesShow 6 test diffsStage 1
Stage 2
Additionally, 4 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 99576cf2ebc870c305225e9856b9423228872c56 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (99576cf): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.0%, secondary 0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 487.812s -> 488.508s (0.14%) |
…aethlin Drop elaboration: Only create a reset block if there are flags to reset. Follow-up to rust-lang#157491 I'm not totally convinced this is worth the effort. The generated pattern is trivially cleaned-up by SimplifyCfg, so we should not need to bother. r? @saethlin since you reviewed the earlier PR
…aethlin Drop elaboration: Only create a reset block if there are flags to reset. Follow-up to rust-lang#157491 I'm not totally convinced this is worth the effort. The generated pattern is trivially cleaned-up by SimplifyCfg, so we should not need to bother. r? @saethlin since you reviewed the earlier PR
…aethlin Drop elaboration: Only create a reset block if there are flags to reset. Follow-up to rust-lang#157491 I'm not totally convinced this is worth the effort. The generated pattern is trivially cleaned-up by SimplifyCfg, so we should not need to bother. r? @saethlin since you reviewed the earlier PR
…aethlin Drop elaboration: Only create a reset block if there are flags to reset. Follow-up to rust-lang#157491 I'm not totally convinced this is worth the effort. The generated pattern is trivially cleaned-up by SimplifyCfg, so we should not need to bother. r? @saethlin since you reviewed the earlier PR
Rollup merge of #159195 - cjgillot:elaborate-drop-flags, r=saethlin Drop elaboration: Only create a reset block if there are flags to reset. Follow-up to #157491 I'm not totally convinced this is worth the effort. The generated pattern is trivially cleaned-up by SimplifyCfg, so we should not need to bother. r? @saethlin since you reviewed the earlier PR
Drop elaboration can be very verbose, especially when locals are move-from by parts. This happens a lot with
?desugaring for instance.I start from the example given in #157463 and attempt to shrink the generated code.
The remaining empty
gotoblocks come from resetting drop flags. The cost/benefit of the refactor was less interesting.