Speed up EverInitializedPlaces - #160033
Conversation
|
@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.
Speed up `EverInitializedPlaces`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f89e821): 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 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 -2.7%, secondary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.6%, secondary -7.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 487.844s -> 486.33s (-0.31%) |
|
A case where looking at cycles/wall-time is better than instruction counts. For cycles the two best results are -5% for cranelift-codegen and -10% for match-stress. I think the discrepancy has something to do with the old code using @rustbot label: +perf-regression-triaged |
|
I'm interested in this PR even if #160193 reduces the expected perf benefit. What makes you keep it as draft? |
|
I'm waiting for #160193 to land so we can re-run perf. It's quite possible that the speed-up disappears completely. And I'm also a bit unhappy with the extra complexity in the second commit. |
|
Hi @nnethercote !
and the separate temporary counters showed so at least for the #159944 reproducer the performance appears to remain substantial after #160193. |
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
|
@mehdiakiki, @zozo123: these comments are not helpful. Please stop. |
e7339de to
4b3859f
Compare
|
#160193 merged, let's re-measure perf. @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.
Speed up `EverInitializedPlaces`
| // Must be a call terminator. Genned only by `apply_call_return_effect`, i.e. along | ||
| // the call's return edge. | ||
| let TerminatorKind::Call { target: call_target, .. } = | ||
| init_block_data.terminator().kind | ||
| else { | ||
| bug!("`NonPanicPathOnly` should only be seen on a `Call` terminator"); | ||
| }; | ||
| queue.extend(call_target); |
There was a problem hiding this comment.
apply_call_return_effect is also called on Yield and InlineAsm.
| // Must be a call terminator. Genned only by `apply_call_return_effect`, i.e. along | |
| // the call's return edge. | |
| let TerminatorKind::Call { target: call_target, .. } = | |
| init_block_data.terminator().kind | |
| else { | |
| bug!("`NonPanicPathOnly` should only be seen on a `Call` terminator"); | |
| }; | |
| queue.extend(call_target); | |
| // Genned only by `apply_call_return_effect`, i.e. along an assigning return edge. | |
| let TerminatorEdges::AssignOnReturn { return_, .. } = | |
| init_block_data.terminator().edges() | |
| else { | |
| bug!("`NonPanicPathOnly` should only be seen on a returning terminators"); | |
| }; | |
| queue.extend(return_); |
Currently it tracks a bit for every `Init`, but it only uses the tracked data for locals. This means it is tracking data for projections that is unused. This commit shrinks the domain to only track data for `Local`s, going from `MixedBitSet<InitIndex>` to `DenseBitSet<Local>`. This does regress the error messages in one test: liveness-assign-imm-local-notes.rs. The next commit will fix that. The commit also removes some `debug` statements which probably haven't seen use in a long time. They can be re-added easily if anyone needs them in the future.
This commit fixes the error message regression in the previous commit by recomputing flow information at error-reporting time. `is_local_ever_initialized` is replaced in two ways. - In `check_access_permissions`: by the new `first_reaching_init` function which picks the first init that can reach the error location along a path that doesn't cross `StorageDead(local)`. - In `add_used_mut`: by a simple `contains` test.
4b3859f to
3a7e145
Compare
|
I made a few changes.
|
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| } | ||
| state.gen_all(init_loc_map[call_loc].iter().copied().filter_map(|ii| { | ||
| let init = &move_data.inits[ii]; | ||
| if init.kind == InitKind::NonPanicPathOnly { |
There was a problem hiding this comment.
Why do we check NonPanicPathOnly now?
There was a problem hiding this comment.
Look at Forward::apply_effects_in_block:
- It calls
apply_primary_terminator_effectunconditionally on the terminator. - It calls
apply_call_return_effectfor anAssignOnReturnterminator with at least one return edge (Call,Yield,InlineAsm). - So there is double handling of some inits, specifically
Deepinits forYield/InlineAsm. apply_primary_terminator_effectalready filters out theNonPanicPathOnlycases, soapply_call_return_effectcan apply the inverse filter to avoid the double handling.
Double handling doesn't really matter, because gen is an idempotent operation. However, removing the double handling makes the if/else if/else in init_reaches_location cleanly mirror apply_primary_statement_effect / apply_call_return_effect / apply_primary_terminator_effect, as per the comments in init_reaches_location.
|
@bors r+ |
|
@bors rollup=iffy |
…uwer Rollup of 10 perf-sensitive pull requests Successful merges: - #157281 (perf: skip irrelevant foreign impls when building the specialization graph) - #159403 (Next steps for FnDef binder changes (instantiate most FnDef binders)) - #159763 (Optimize crate resolution for large workspace) - #160033 (Speed up `EverInitializedPlaces`) - #160268 (perf: store the fulfillment engine inline in ObligationCtxt) - #160317 (perf: Cache already-checked types in the privacy visitor) - #160399 (interpret: skip deref-projection validity checks when they are not needed) - #160451 (Deduplicate target and host filesearch) - #160453 (Add fast path to `escape_string_symbol`) - #160454 (Add offload guard flags to typeck to prevent perf regressions)
|
Verifying that actual perf results after merge match expected results |
This comment has been minimized.
This comment has been minimized.
…uwer Rollup of 10 perf-sensitive pull requests Successful merges: - rust-lang/rust#157281 (perf: skip irrelevant foreign impls when building the specialization graph) - rust-lang/rust#159403 (Next steps for FnDef binder changes (instantiate most FnDef binders)) - rust-lang/rust#159763 (Optimize crate resolution for large workspace) - rust-lang/rust#160033 (Speed up `EverInitializedPlaces`) - rust-lang/rust#160268 (perf: store the fulfillment engine inline in ObligationCtxt) - rust-lang/rust#160317 (perf: Cache already-checked types in the privacy visitor) - rust-lang/rust#160399 (interpret: skip deref-projection validity checks when they are not needed) - rust-lang/rust#160451 (Deduplicate target and host filesearch) - rust-lang/rust#160453 (Add fast path to `escape_string_symbol`) - rust-lang/rust#160454 (Add offload guard flags to typeck to prevent perf regressions)
|
Finished benchmarking commit (65b521b): 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 1.0%, secondary 2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.5%, secondary -3.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: 489.577s -> 489.996s (0.09%) |
View all comments
By simplifying its domain. Details in individual commits.
r? @cjgillot