Fix performance regression introduced in #142531 by excluding Storage{Live,Dead} from CGU size estimation#155491
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
544ea18 to
bb8798b
Compare
| for (local, &head) in ssa.copy_classes().iter_enumerated() { | ||
| if local != head { | ||
| any_replacement = true; | ||
| storage_to_remove.insert(head); |
There was a problem hiding this comment.
Before, Replacer used unified to check which locals require storage removal - so this is fine
https://github.com/rust-lang/rust/pull/142531/changes#diff-0004c9822ecc282c9d6f6bcec22d1b50d49c6165e8093c1dbd2607086278dc19L96
bb8798b to
2389a3a
Compare
|
@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.
… r=<try> Avoid unneeded work for storage removal in non-opt builds in CopyProp and GVN
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (48af6e0): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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. @bors rollup=never Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary 4.2%, secondary -1.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.6%, secondary 6.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 492.524s -> 492.577s (0.01%) |
|
(I probably won't be able to do this, right @Kobzol ?) @bors try @rust-timer queue |
|
@ohadravid: 🔑 Insufficient privileges: not in try users |
This comment has been minimized.
This comment has been minimized.
|
@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.
… r=<try> Avoid unneeded work for storage removal in non-opt builds in CopyProp and GVN
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9ab524a): 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.7%, secondary -1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.4%, secondary -4.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.2%, secondary -0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 490.129s -> 492.074s (0.40%) |
|
Can you update the PR description to include the second commit as well? Then r=me. It's up to you if you want to include the initial optimization as well, I think it makes logical sense and doesn't introduce too much complexity so it would be okay to include it. Even though it doesn't seem very impactful. |
Storage{Live,Dead} from CGU size estimation
Storage{Live,Dead} from CGU size estimationStorage{Live,Dead} from CGU size estimation
|
@saethlin done 🙏 |
Fix performance regression introduced in #142531 (rust-timer comment) by excluding
Storage{Live,Dead}from CGU size estimation.Also, avoid unneeded work for storage removal in non-opt builds in CopyProp and GVN
by allocating local sets for the storage accounting only when
tcx.sess.emit_lifetime_markers().r? saethlin