Cache block seeking results for initialized_at_terminator and _exit - #162450
Cache block seeking results for initialized_at_terminator and _exit#162450jackh726 wants to merge 1 commit into
Conversation
rustbot
added
S-waiting-on-review
|
Disclosure: An LLM was used to identify this perf effect and to check the performance of the solution. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
Cache block seeking results for initialized_at_terminator and _exit
| if self.cx.initialized_at_terminator(location.block, mpi) { | ||
| let inserted = self.drop_live_at.insert(drop_point); | ||
| // Right now, we should visit a drop_point twice. | ||
| // If we do, this trigger a debug assert so we need we can optimize. | ||
| debug_assert!(inserted, "drop point should not have been visited yet"); |
There was a problem hiding this comment.
This is technically completely unrelated - but sticking it in as a drive-by pass. I had a though that we might be visiting drop points twice (and thus could switch these conditions for a perf benefit) - but the iteration here is unique.
| // Ensure self.flow_inits is initialized | ||
| let _ = self.flow_inits(); |
There was a problem hiding this comment.
I thought about changing fn flow_inits to be something like fn ensure_flow_inits(&mut _). Then this could get moved into initialized_at too.
| block, | ||
| mpi, | ||
| |flow_inits: &mut ResultsCursor<'_, 'tcx, MaybeInitializedPlaces<'_, 'tcx>>| { | ||
| let terminator_location = self.typeck.body.terminator_loc(block); |
There was a problem hiding this comment.
Another alternative: pass self.typeck into initialized_at and call this there.
| flow_inits: &mut ResultsCursor<'_, 'tcx, MaybeInitializedPlaces<'_, 'tcx>>, | ||
| block: BasicBlock, | ||
| mpi: MovePathIndex, | ||
| seek: impl FnOnce(&mut ResultsCursor<'_, 'tcx, MaybeInitializedPlaces<'_, 'tcx>>), |
There was a problem hiding this comment.
Another option that may work: a fn(&mut ResultsCursor<'_, 'tcx, MaybeInitializedPlaces<'_, 'tcx>>, Location). Then I think you could just pass e.g. ResultsCursor::seek_before_primary_effect instead of a closure. Not sure if lifetimes allow though.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (2bf7717): comparison URL. Overall result: ✅ improvements - 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 rustc-perf 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 -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -1.4%, secondary -0.3%)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: 482.327s -> 479.101s (-0.67%) |
rustbot
removed
the
S-waiting-on-perf
4 participants
Caching the block values post-seek results in significant performance wins (especially on pathological benchmarks). These wins extend to NLL, too.
Possibly, this caching could be done further up the stack (more generically), but figured this is a "safer" approach - rather than dealing with other potential fallout.
r? lqd
LLM-generated perf summary table