cleanup: merge RvalueScopes into ScopeTree

[dianne]

This gets rid of RvalueCandidate, inlines the definition of RvalueScopes into ScopeTree, and removes two RvalueScopes-specific modules, consolidating the scoping logic a bit. Removing the extra step of going from RvalueCandidates to RvalueScopes and removing the duplication between them should also hopefully improve perf.

I've also taken the liberty of doing a bit of renaming and comment updates, changing some "rvalue scope"s to "extended temporary scope"s. This is a bit closer to the Reference's terminology and makes it clearer that it's specific to temporary lifetime extension. This isn't comprehensive. In particular, I've left record_rvalue_scope_if_borrow_expr untouched since #146098 gets rid of it.

Pulled out from #146098.

r? BoxyUwU as the reviewer of #146098 (though feel free to reassign/claim! this is just cleanup)

cc @dingxiangfei2009

[dianne]

@bors try @rust-timer queue

[rust-bors]

💥 Test timed out after 21600s

[dianne]

Retrying CI since #t-infra > CI keeps failing because of npm error seems to be fixed. Not sure what's up with the bors2 timeout but let's try that again too I guess?

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 03a6515 (03a6515b1ba7e8b2d5a4165ce136197e7d22feb3, parent: ceb7df7e6f17c92c7d49f7e4f02df0e68bc9b38b)

[rust-timer]

Finished benchmarking commit (03a6515): 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
Regressions ❌ (secondary)	0.4%	[0.0%, 0.6%]	4
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.1%]	4
Improvements ✅ (secondary)	-0.0%	[-0.1%, -0.0%]	5
All ❌✅ (primary)	-0.1%	[-0.2%, -0.1%]	4

Max RSS (memory usage)

Results (primary 4.3%, secondary -1.5%)

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

	mean	range	count
Regressions ❌ (primary)	4.3%	[1.3%, 7.2%]	2
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-3.5%, -2.0%]	4
All ❌✅ (primary)	4.3%	[1.3%, 7.2%]	2

Cycles

Results (primary 2.6%, secondary 6.0%)

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

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.1%, 3.0%]	4
Regressions ❌ (secondary)	6.0%	[5.6%, 6.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.1%, 3.0%]	4

Binary size

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

Bootstrap: 475.105s -> 475.535s (0.09%)
Artifact size: 390.80 MiB -> 390.76 MiB (-0.01%)

[dianne]

So that's where the deep-vector regression is from! To be honest, I'm not confident I know what's going on. I tried profiling locally to get a better idea of where the instruction count difference is, and it looks like the biggest diff by far is malloc_default with 11,972,450 more instructions(?), and the second biggest is emap_try_acquire_edata_neighbor with 2,270,678 more instructions(?). So apparently it's spending more instructions in the allocator? Maybe changing when the ScopeTree is built happens to be worse for deep-vector, somehow? For pathological cases like deep-vector, the scope tree's parent scope map is very large and it's not initialized with a capacity or anything so it probably needs a lot of reallocations; being later in compilation is probably bad for that? This also slightly changed the sizes of ScopeTree and TypeckResults, but that seems less likely to be it.

I do have an couple ideas that could improve perf both here and for realistic code, but they're not pretty. Reifying the scope hierarchy isn't really necessary, and there's a few ways to avoid it; e.g.:

• Currently, the ScopeTree's parent scope map is effectively a copy of the HIR for a typeck root but allocated in a hash map rather than an arena and more convenient for doing upwards traversals on. But we could do upwards traversals directly on the HIR. Upwards traversals on the HIR are unpleasant though, so I don't know whether it would be worth it.
• We could avoid both the upwards traversals and building a parent map if we do scope resolution in the same pass as building the THIR. We'd just be doing more bookkeeping on the way down. This would probably have the best perf, but it's not good for separating concerns. It's also a bit inconvenient for a few shadowing-related diagnostics/lints, which simpler to write when there's an easy way to inspect the scope hierarchy; getting rid of upwards traversals means they'd have to implement their own most likely.

Another option: we remove the TempLifetime from thir::Expr. The MIR Builder has access to the ScopeTree for getting variables' scopes, so why not get temporary scopes that way too? There may be a reason for this, but I'm not aware of it if so. Making thir::Expr smaller and doing less work per expression node would probably help. I may try this.

[dianne]

Looks like removing the TempLifetime from thir::Expr indeed helps. #148706 does that using this PR as a foundation, which turns the 0.6% regression in deep-vector to a 2.3-2.5% improvement (and also seems generally to improve perf for real code).

[cjgillot]

The perf regression is very small on a stress test. This PR is a good cleanup and allows for further simplifications.
@bors r+

[bors]

📌 Commit 0e50d5a has been approved by cjgillot

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0e50d5a with merge 25d319a...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 25d319a to main...

[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 2636cb4 (parent) -> 25d319a (this PR)

Test differences

Show 58 test diffs

58 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 25d319a0f656ee8faa7a534da299e76e96068a40 --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-gnu-debug: 4669.4s -> 4123.8s (-11.7%)
2. dist-x86_64-apple: 7644.7s -> 8529.8s (+11.6%)
3. dist-aarch64-msvc: 6162.2s -> 5517.8s (-10.5%)
4. aarch64-msvc-2: 5232.3s -> 4700.5s (-10.2%)
5. dist-ohos-armv7: 4513.6s -> 4068.9s (-9.9%)
6. x86_64-gnu-llvm-20-3: 6689.7s -> 6070.4s (-9.3%)
7. dist-ohos-x86_64: 4683.3s -> 4292.2s (-8.3%)
8. dist-aarch64-windows-gnullvm: 5179.3s -> 4771.5s (-7.9%)
9. dist-sparcv9-solaris: 4869.3s -> 5240.1s (+7.6%)
10. aarch64-gnu: 6953.3s -> 6452.2s (-7.2%)

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 (25d319a): 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)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	0.5%	[0.0%, 0.6%]	5
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	7
All ❌✅ (primary)	1.5%	[-0.1%, 3.1%]	2

Max RSS (memory usage)

Results (secondary 0.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)	3.3%	[2.1%, 4.2%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.7%, -2.0%]	7
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.8%, secondary -1.7%)

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

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-3.4%, -2.0%]	3
All ❌✅ (primary)	2.8%	[2.8%, 2.8%]	1

Binary size

Results (primary 1.1%)

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

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

Bootstrap: 478.438s -> 474.905s (-0.74%)
Artifact size: 391.34 MiB -> 391.28 MiB (-0.02%)