privacy: Introduce some caching to type visiting in DefIdVisitorSkeleton

[petrochenkov]

The caching fixes compilation speed issues in special cases like #145741, without introducing too much overhead in general cases.

I tried to cache more, but it caused regressions from the caching overhead, like it can be seen from benchmark runs below.

Inspired by #146128.
Closes #145741.

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[petrochenkov]

@bors2 try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 665fb0c (665fb0c6194a924362c4847361764e762a482668, parent: 910617d84d611e9ba508fd57a058c59b8a767697)

[rust-timer]

Finished benchmarking commit (665fb0c): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@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.4%	[0.1%, 1.0%]	141
Regressions ❌ (secondary)	0.4%	[0.1%, 1.0%]	77
Improvements ✅ (primary)	-1.3%	[-2.8%, -0.2%]	20
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	0.2%	[-2.8%, 1.0%]	161

Max RSS (memory usage)

Results (primary 0.9%)

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

	mean	range	count
Regressions ❌ (primary)	2.6%	[0.8%, 4.4%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[-2.6%, 4.4%]	3

Cycles

Results (secondary -2.9%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-2.9%, -2.9%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 473.206s -> 472.658s (-0.12%)
Artifact size: 388.42 MiB -> 388.39 MiB (-0.01%)

[petrochenkov]

Interesting, per-DefId caching is a small regression in majority of cases, but provides larger speedups in some cases.
(From local testing, caching at DefId level doesn't help with #145741.)

I'll test per-Ty and per-TraitRef caching next.

[petrochenkov]

@bors2 try @rust-timer queue

[petrochenkov]

The Ty-caching version helps with #145741, and reduces the compilation time from 35 to 16 seconds on my machine.

[rust-bors]

☀️ Try build successful (CI)
Build commit: f8e844a (f8e844a5ff3d4e587399f1873c352cd8bb5ba8fd, parent: acf243778e6c54cb7d54bee4be88e510e4be123e)

[rust-timer]

Finished benchmarking commit (f8e844a): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@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.6%	[0.1%, 1.5%]	148
Regressions ❌ (secondary)	0.8%	[0.1%, 3.1%]	130
Improvements ✅ (primary)	-0.5%	[-0.7%, -0.1%]	10
Improvements ✅ (secondary)	-0.5%	[-0.8%, -0.2%]	7
All ❌✅ (primary)	0.5%	[-0.7%, 1.5%]	158

Max RSS (memory usage)

Results (primary 1.4%)

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

	mean	range	count
Regressions ❌ (primary)	3.8%	[3.8%, 3.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[-1.0%, 3.8%]	2

Cycles

Results (secondary 3.0%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.0%	[2.0%, 4.9%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 471.876s -> 472.593s (0.15%)
Artifact size: 388.40 MiB -> 388.36 MiB (-0.01%)

[petrochenkov]

#146128 (comment)
@rustbot blocked

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[petrochenkov]

@rustbot ready

[lcnr]

@bors r+

[bors]

📌 Commit f9464f8 has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit f9464f8 with merge 6501e64...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 6501e64 to master...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 11d2046 (parent) -> 6501e64 (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 6501e64fcb02d22b49d6e59d10a7692ec8095619 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. aarch64-apple: 7569.5s -> 13800.0s (82.3%)
2. dist-aarch64-apple: 6319.1s -> 7480.5s (18.4%)
3. dist-apple-various: 3595.5s -> 4124.5s (14.7%)
4. dist-various-1: 4287.2s -> 3727.6s (-13.1%)
5. dist-x86_64-msvc-alt: 9540.9s -> 8621.8s (-9.6%)
6. x86_64-mingw-1: 9924.8s -> 9031.0s (-9.0%)
7. i686-msvc-2: 7654.1s -> 8181.6s (6.9%)
8. dist-x86_64-apple: 7072.6s -> 7494.8s (6.0%)
9. x86_64-msvc-ext2: 6166.5s -> 5798.5s (-6.0%)
10. i686-msvc-1: 10165.4s -> 10733.7s (5.6%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (6501e64): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2%	[0.0%, 0.4%]	17
Regressions ❌ (secondary)	0.7%	[0.1%, 1.2%]	6
Improvements ✅ (primary)	-0.4%	[-0.5%, -0.2%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.5%, 0.4%]	24

Max RSS (memory usage)

Results (primary -2.5%, secondary -4.4%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-4.4%	[-4.4%, -4.4%]	1
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Cycles

Results (secondary -1.9%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-2.5%, -1.4%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 474.995s -> 476.496s (0.32%)
Artifact size: 390.51 MiB -> 390.49 MiB (-0.01%)

[panstromek]

perf triage:

This PR fixes exponential blowup. Small regressions from caching overhead are expected.

@rustbot label: +perf-regression-triaged

@petrochenkov It doesn't seem to have such a positive impact in the current benchmark suite. Do you think we should have a reproducer for the original issue in secondary benchmarks, because somebody could accidentaly remove this fix?

[petrochenkov]

Do you think we should have a reproducer for the original issue in secondary benchmarks, because somebody could accidentaly remove this fix?

#146128 (comment)