Ergonomic ref counting: optimize away clones when possible #139088

spastorino · 2025-03-28T21:37:07Z

This PR build on top of #134797. It optimizes codegen of ergonomic ref-counting when the type being used is only known to be copy after monomorphization. We avoid codening a clone and generate bitwise copy instead.

RFC: rust-lang/rfcs#3680
Tracking issue: #132290
Project goal: rust-lang/rust-project-goals#107

r? @nikomatsakis

This PR could better sit on top of #131650 but as it did not land yet I've decided to just do minimal changes. It may be the case that doing what I'm doing regress the performance and we may need to go the full route of #131650.
cc @saethlin in this regard.

compiler/rustc_codegen_ssa/src/mir/mod.rs

spastorino · 2025-03-28T21:42:56Z

compiler/rustc_codegen_ssa/src/mir/mod.rs

+    let mir = instance.instantiate_mir_and_normalize_erasing_regions(
+        tcx,
+        ty::TypingEnv::fully_monomorphized(),
+        ty::EarlyBinder::bind(mir.clone()),


We need to clone the mir body here, unsure how bad this could be.

Maybe in order to avoid affecting performance of things that do not involve use, we can just execute this when tcx.features().ergonomic_clones() is on.

tests/codegen/ergonomic-clones/closure.rs

spastorino · 2025-03-31T15:05:03Z

tests/crashes/129372.rs

@@ -1,52 +0,0 @@
-//@ known-bug: #129372


Would need to properly decide exactly what to do with this test. If just removing it is fine or if there's some variation that needs to be placed in tests/ui. Didn't bother to check this for now.

rustbot · 2025-03-31T15:41:12Z

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

RalfJung · 2025-03-31T16:21:07Z

(Procedurally, it'd be nice to reference the tracking issue in every PR related to that feature. Makes it much easier to navigate around for someone who's not following every single PR in this project.)

spastorino · 2025-04-07T19:59:52Z

I've just pushed a new commit that avoids cloning mir if feature ergonomic_clones is off.

@bors try @rust-timer queue

bors · 2025-04-07T20:01:03Z

⌛ Trying commit 4a0ea02 with merge aa03ab8...

… r=<try> Ergonomic ref counting: optimize away clones when possible This PR build on top of rust-lang#134797. It optimizes codegen of ergonomic ref-counting when the type being `use`d is only known to be copy after monomorphization. We avoid codening a clone and generate bitwise copy instead. RFC: rust-lang/rfcs#3680 Tracking issue: rust-lang#132290 Project goal: rust-lang/rust-project-goals#107 r? `@nikomatsakis` This PR could better sit on top of rust-lang#131650 but as it did not land yet I've decided to just do minimal changes. It may be the case that doing what I'm doing regress the performance and we may need to go the full route of rust-lang#131650. cc `@saethlin` in this regard.

bors · 2025-04-07T22:08:23Z

☀️ Try build successful - checks-actions
Build commit: aa03ab8 (aa03ab869e0acf922856a00266556216ea994378)

rust-timer · 2025-04-07T23:25:10Z

Finished benchmarking commit (aa03ab8): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	3
Regressions ❌ (secondary)	0.3%	[0.2%, 0.6%]	20
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	3

Max RSS (memory usage)

Results (primary -0.4%, secondary -1.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.0%	[4.0%, 4.0%]	1
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	2
Improvements ✅ (primary)	-1.8%	[-2.7%, -0.5%]	3
Improvements ✅ (secondary)	-2.6%	[-3.8%, -2.0%]	6
All ❌✅ (primary)	-0.4%	[-2.7%, 4.0%]	4

Cycles

Results (secondary 1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

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

Bootstrap: 778.658s -> 780.235s (0.20%)
Artifact size: 366.00 MiB -> 365.98 MiB (-0.01%)

nikomatsakis

This seems like a good first option until #131650 lands. There are more efficient ways to do this if compilation time becomes an issue later on.

nikomatsakis · 2025-04-09T13:43:40Z

@bors r+

bors · 2025-04-09T13:43:43Z

📌 Commit 4a0ea02 has been approved by nikomatsakis

It is now in the queue for this repository.

spastorino · 2025-04-09T20:19:00Z

Removed perf regression label as the latest perf run didn't show regressions. Anyway, we are keeping track of potential perf changes.

Zalathar · 2025-04-10T08:00:16Z

Scheduling: If the current rollup fails, move on to a never PR instead of an iffy PR.

@bors p=1

bors · 2025-04-10T09:08:26Z

⌛ Testing commit 4a0ea02 with merge 7d7de5b...

bors · 2025-04-10T12:17:15Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 7d7de5b to master...

github-actions · 2025-04-10T12:19:02Z

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 9d28fe3 (parent) -> 7d7de5b (this PR)

Test differences

Show 10 test diffs

Stage 1

[codegen] tests/codegen/ergonomic-clones/closure.rs: [missing] -> pass (J1)
[crashes] tests/crashes/129372.rs: pass -> [missing] (J1)
[mir-opt] tests/mir-opt/ergonomic-clones/closure.rs: [missing] -> pass (J1)

Stage 2

[codegen] tests/codegen/ergonomic-clones/closure.rs: [missing] -> pass (J0)
[crashes] tests/crashes/129372.rs: pass -> [missing] (J0)
[mir-opt] tests/mir-opt/ergonomic-clones/closure.rs: [missing] -> pass (J0)

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

Job group index

Job duration changes

dist-apple-various: 6377.4s -> 8558.3s (34.2%)
dist-x86_64-apple: 12204.8s -> 9057.1s (-25.8%)
aarch64-apple: 3464.1s -> 3993.0s (15.3%)
x86_64-gnu-nopt: 6506.5s -> 5556.9s (-14.6%)
dist-aarch64-apple: 4331.3s -> 4960.5s (14.5%)
dist-loongarch64-linux: 5924.9s -> 6381.1s (7.7%)
x86_64-gnu-aux: 5927.1s -> 6285.1s (6.0%)
x86_64-apple-1: 7968.7s -> 7565.1s (-5.1%)
dist-aarch64-linux: 5573.8s -> 5338.8s (-4.2%)
x86_64-msvc-ext2: 5959.5s -> 5708.9s (-4.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 · 2025-04-10T13:55:15Z

Finished benchmarking commit (7d7de5b): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (secondary -7.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

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

Binary size

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

Bootstrap: 780.245s -> 782.463s (0.28%)
Artifact size: 366.19 MiB -> 366.23 MiB (0.01%)

rustbot assigned nikomatsakis Mar 28, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 28, 2025

spastorino commented Mar 28, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/mod.rs Outdated Show resolved Hide resolved

spastorino commented Mar 28, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/mod.rs Outdated Show resolved Hide resolved

spastorino commented Mar 28, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/mod.rs Outdated Show resolved Hide resolved

spastorino commented Mar 28, 2025

View reviewed changes