perf: Cache already-checked types in the privacy visitor - #160317
Conversation
|
Something similar was tried in #147486, but the caching overhead was found to be too large in most cases. |
|
@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.
Needs a rebase first. |
This comment has been minimized.
This comment has been minimized.
0929cce to
6511e1d
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.
…<try> perf: Cache already-checked types in the privacy visitor
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (278a694): 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%, secondary -0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.0%, secondary 0.7%)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: 488.148s -> 489.283s (0.23%) |
|
I see how the type caching can be useful, but I'm not sure how much the whole |
|
Reminder, once the PR becomes ready for a review, use |
6511e1d to
da4563a
Compare
|
You are right, I measured a bit more and the majority of the win is type caching. I updated the PR to type cache only. |
|
@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.
…<try> perf: Cache already-checked types in the privacy visitor
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5b9f7d9): 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.9%, secondary -0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.3%, secondary -0.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: 491.018s -> 490.335s (-0.14%) |
|
Mostly the same as with Could you make a PR with |
|
@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)
Rollup merge of #160317 - xmakro:perf/privacy-accessible-type-cache, r=petrochenkov perf: Cache already-checked types in the privacy visitor The privacy checker walks the full type of every expression and pattern in a module, re-walking the same type once per node it appears on. This caches the types that walked clean (no privacy error) and skips them next time. A walk's result depends only on the interned type and the fixed module being checked, so a type that walks clean once walks clean everywhere. Only clean walks are cached, so nothing is lost: a type that errors is never cached and still fires at every span, and no dep-graph edges are dropped since the full walk already ran once in the same query.
|
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 (76908bc): 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.7%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.3%, secondary -0.1%)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 -> 496.509s (1.42%) |
I benchmarked the GenericArgs cache on top of the merged type cache with a local rustc-perf run. The result was neutral, nothing moved significantly. Most of what the cache hits is the interned empty GenericArgs list, whose walk is nearly free. So this is not worth a PR. Thanks for spotting this in the original PR. |
View all comments
The privacy checker walks the full type of every expression and pattern in a module, re-walking the same type once per node it appears on. This caches the types that walked clean (no privacy error) and skips them next time. A walk's result depends only on the interned type and the fixed module being checked, so a type that walks clean once walks clean everywhere.
Only clean walks are cached, so nothing is lost: a type that errors is never cached and still fires at every span, and no dep-graph edges are dropped since the full walk already ran once in the same query.