Skip to content

perf: Cache already-checked types in the privacy visitor - #160317

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
xmakro:perf/privacy-accessible-type-cache
Aug 4, 2026
Merged

perf: Cache already-checked types in the privacy visitor#160317
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
xmakro:perf/privacy-accessible-type-cache

Conversation

@xmakro

@xmakro xmakro commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

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.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 1, 2026
@petrochenkov petrochenkov self-assigned this Aug 1, 2026
@petrochenkov

Copy link
Copy Markdown
Contributor

Something similar was tried in #147486, but the caching overhead was found to be too large in most cases.

@petrochenkov

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 1, 2026
@petrochenkov

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

Needs a rebase first.

@rust-timer

This comment has been minimized.

@petrochenkov petrochenkov removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 1, 2026
@xmakro
xmakro force-pushed the perf/privacy-accessible-type-cache branch from 0929cce to 6511e1d Compare August 1, 2026 17:25
@petrochenkov

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Aug 2, 2026
…<try>

perf: Cache already-checked types in the privacy visitor
@rust-bors

rust-bors Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 278a694 (278a6943d9a56361f343eea016b348202494ff1a)
Base parent: 8c3a200 (8c3a200b704adb5f110249b68daa4ff386b36e29)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (278a694): comparison URL.

Overall result: ✅ improvements - no action needed

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.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-1.1%, -0.2%] 55
Improvements ✅
(secondary)
-0.5% [-0.9%, -0.2%] 67
All ❌✅ (primary) -0.5% [-1.1%, -0.2%] 55

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.

mean range count
Regressions ❌
(primary)
2.7% [2.7%, 2.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-0.9%, -0.5%] 2
Improvements ✅
(secondary)
-0.8% [-2.7%, -0.4%] 13
All ❌✅ (primary) 0.4% [-0.9%, 2.7%] 3

Cycles

Results (primary 0.0%, secondary 0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.8% [0.4%, 1.7%] 5
Regressions ❌
(secondary)
1.2% [0.5%, 2.7%] 9
Improvements ✅
(primary)
-0.7% [-1.0%, -0.5%] 5
Improvements ✅
(secondary)
-0.5% [-0.7%, -0.5%] 4
All ❌✅ (primary) 0.0% [-1.0%, 1.7%] 10

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 488.148s -> 489.283s (0.23%)
Artifact size: 390.38 MiB -> 390.33 MiB (-0.01%)

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 3, 2026
@petrochenkov
petrochenkov marked this pull request as ready for review August 3, 2026 10:58
@petrochenkov

Copy link
Copy Markdown
Contributor

I see how the type caching can be useful, but I'm not sure how much the whole GenericArgs caching contributes to the results.
Could we benchmark them separately, starting with the types?
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2026
@rustbot

rustbot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@xmakro
xmakro force-pushed the perf/privacy-accessible-type-cache branch from 6511e1d to da4563a Compare August 3, 2026 18:04
@xmakro

xmakro commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

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.

@petrochenkov

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 3, 2026
rust-bors Bot pushed a commit that referenced this pull request Aug 3, 2026
…<try>

perf: Cache already-checked types in the privacy visitor
@rust-bors

rust-bors Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 5b9f7d9 (5b9f7d9cc424d1f848e527a01fff9f8a090d64d0)
Base parent: 7c329d6 (7c329d6c76e11ca40c5673818ab0439c1be8962c)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (5b9f7d9): comparison URL.

Overall result: ✅ improvements - no action needed

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.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-1.1%, -0.2%] 58
Improvements ✅
(secondary)
-0.4% [-0.9%, -0.2%] 71
All ❌✅ (primary) -0.5% [-1.1%, -0.2%] 58

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.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.5%] 2
Regressions ❌
(secondary)
0.5% [0.4%, 0.6%] 3
Improvements ✅
(primary)
-3.6% [-3.6%, -3.6%] 1
Improvements ✅
(secondary)
-1.9% [-4.7%, -0.5%] 3
All ❌✅ (primary) -0.9% [-3.6%, 0.5%] 3

Cycles

Results (primary -0.3%, secondary -0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.8% [0.5%, 1.0%] 4
Regressions ❌
(secondary)
1.2% [0.7%, 1.6%] 4
Improvements ✅
(primary)
-1.4% [-2.4%, -0.6%] 4
Improvements ✅
(secondary)
-1.2% [-3.6%, -0.4%] 5
All ❌✅ (primary) -0.3% [-2.4%, 1.0%] 8

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 491.018s -> 490.335s (-0.14%)
Artifact size: 390.29 MiB -> 390.25 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 3, 2026
@petrochenkov

Copy link
Copy Markdown
Contributor

Mostly the same as with GenericArgs.
@bors r+

Could you make a PR with GenericArgs caching once this one merges, we'll benchmark it separately and land it too if it's above the noise.

@rust-bors

rust-bors Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

📌 Commit da4563a has been approved by petrochenkov

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 10. This pull request will be tested once the tree is reopened.

Reason for tree closure: manually handling queue due to backlog

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 4, 2026
@JonathanBrouwer

Copy link
Copy Markdown
Contributor

@bors rollup=iffy
Including this in a special perf-sensitive rollup

rust-bors Bot pushed a commit that referenced this pull request Aug 4, 2026
…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)
@rust-bors
rust-bors Bot merged commit d71ca43 into rust-lang:main Aug 4, 2026
14 checks passed
@rustbot rustbot added this to the 1.99.0 milestone Aug 4, 2026
rust-timer added a commit that referenced this pull request Aug 4, 2026
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.
@JonathanBrouwer

Copy link
Copy Markdown
Contributor

Verifying that actual perf results after merge match expected results
@rust-timer build 76908bc

@rust-timer

This comment has been minimized.

WhySoBad pushed a commit to WhySoBad/miri that referenced this pull request Aug 5, 2026
…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)
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (76908bc): comparison URL.

Overall result: ✅ improvements - no action needed

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.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-1.1%, -0.2%] 57
Improvements ✅
(secondary)
-0.4% [-0.9%, -0.1%] 72
All ❌✅ (primary) -0.5% [-1.1%, -0.2%] 57

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.

mean range count
Regressions ❌
(primary)
0.5% [0.5%, 0.5%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.1% [-3.1%, -3.1%] 1
Improvements ✅
(secondary)
-2.0% [-3.9%, -0.6%] 5
All ❌✅ (primary) -0.7% [-3.1%, 0.5%] 3

Cycles

Results (primary 0.3%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.9% [0.4%, 1.6%] 6
Regressions ❌
(secondary)
0.7% [0.4%, 1.0%] 6
Improvements ✅
(primary)
-0.7% [-1.3%, -0.5%] 4
Improvements ✅
(secondary)
-0.9% [-2.2%, -0.4%] 6
All ❌✅ (primary) 0.3% [-1.3%, 1.6%] 10

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.577s -> 496.509s (1.42%)
Artifact size: 390.24 MiB -> 390.30 MiB (0.02%)

@xmakro

xmakro commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Could you make a PR with GenericArgs caching once this one merges, we'll benchmark it separately and land it too if it's above the noise.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants