Inline `FnOnce`/`FnMut`/`Fn` shims once again #137907

compiler-errors · 2025-03-03T02:13:10Z

This PR fixes the argument checking for extern "rust-call" ABI functions with a spread arg, which do no expect their arguments to be exploded from a tuple like closures do.

Secondly, it removes the hack that prevented them from being inlined. This results in more work done by the compiler, but it does end up allowing us to inline functions we didn't before.

Fixes #137901

compiler-errors · 2025-03-03T02:13:15Z

@bors try @rust-timer queue

Inline `FnOnce`/`FnMut`/`Fn` shims once again cc rust-lang#137901 r? `@ghost`

bors · 2025-03-03T02:14:26Z

⌛ Trying commit 3be05c2 with merge 66b5f9d...

compiler/rustc_mir_transform/src/inline.rs

compiler-errors · 2025-03-03T03:18:27Z

@bors try @rust-timer queue

bors · 2025-03-03T03:19:38Z

⌛ Trying commit 2091084 with merge 633435e...

Inline `FnOnce`/`FnMut`/`Fn` shims once again cc rust-lang#137901 r? `@ghost`

bors · 2025-03-03T05:19:05Z

☀️ Try build successful - checks-actions
Build commit: 633435e (633435edd7c1b4e80999da27eba46fd6726a55b5)

compiler-errors · 2025-03-03T05:32:30Z

Previously, this ended up making things kinda slow in the compiler: #110833 (comment).

…, r=<try> [do not merge] Inliner experiment 2 cc rust-lang#137907 (comment) r? `@ghost`

rust-timer · 2025-03-03T09:40:18Z

Finished benchmarking commit (633435e): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	13.7%	[0.5%, 59.3%]	5
Regressions ❌ (secondary)	0.7%	[0.5%, 1.9%]	7
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.2%]	16
Improvements ✅ (secondary)	-0.8%	[-3.4%, -0.2%]	24
All ❌✅ (primary)	3.0%	[-0.9%, 59.3%]	21

Max RSS (memory usage)

Results (primary 0.4%, secondary 2.2%)

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)	8.7%	[3.9%, 15.3%]	3
Regressions ❌ (secondary)	4.4%	[2.6%, 6.3%]	2
Improvements ✅ (primary)	-3.1%	[-4.8%, -1.7%]	7
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	0.4%	[-4.8%, 15.3%]	10

Cycles

Results (primary 12.8%, secondary -4.9%)

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)	33.9%	[5.9%, 61.9%]	2
Regressions ❌ (secondary)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-1.3%	[-2.1%, -0.8%]	3
Improvements ✅ (secondary)	-7.0%	[-9.9%, -3.4%]	3
All ❌✅ (primary)	12.8%	[-2.1%, 61.9%]	5

Binary size

Results (primary -0.1%, secondary -0.1%)

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.2%	[0.0%, 1.0%]	10
Regressions ❌ (secondary)	0.5%	[0.2%, 1.1%]	4
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.0%]	66
Improvements ✅ (secondary)	-0.2%	[-1.3%, -0.1%]	38
All ❌✅ (primary)	-0.1%	[-0.6%, 1.0%]	76

Bootstrap: missing data
Artifact size: 361.95 MiB -> 361.94 MiB (-0.00%)

…, r=<try> [do not merge] Inliner experiment 2 cc rust-lang#137907 (comment) r? `@ghost`

Noratrieb · 2025-03-03T19:43:07Z

For the clap regression: I ran cachegrind and confirmed that yes, this is indeed all in LLVM (as the codegen split in rustc-perf already suggested). Give that this is incr-patched, it seems likely that this is because of a lack of CGU partitioning luck due to the extra inlining.
Given that that most other benchmarks are positive (even some incr-patched) I think that this can be merged despite this large regression. It does look like clap is at a local minimum here, but given that CGU partitioning is an inherently chaotic system, I don't think such local minima have to constrain our space here. It seems likely that if someone does future changes to CGU partitioning, the chaos will reshuffle and clap might get the perf back.
I haven't looked into serde_derive but I suspect it's the same.

But I think @saethlin would have a more informed opinion on this

rustbot · 2025-03-03T20:25:58Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2025-03-03T20:26:03Z

r? saethlin or reassign

compiler-errors · 2025-03-03T20:31:31Z

agh didn't mean to push that last commit

saethlin · 2025-03-03T23:25:22Z

There is also no data for the runtime benchmarks.

compiler-errors · 2025-03-03T23:29:41Z

Let me fix CI (there seems to be some platform-dependent inliner instability) then I can queue another perf run.

compiler-errors · 2025-03-03T23:30:41Z

@bors try @rust-timer queue

bors · 2025-03-03T23:31:53Z

⌛ Trying commit d33946c with merge 76aedcd...

Inline `FnOnce`/`FnMut`/`Fn` shims once again This PR fixes the argument checking for `extern "rust-call"` ABI functions with a spread arg, which do no expect their arguments to be exploded from a tuple like closures do. Secondly, it removes the hack that prevented them from being inlined. This results in more work done by the compiler, but it does end up allowing us to inline functions we didn't before. Fixes rust-lang#137901

saethlin · 2025-03-03T23:52:12Z

But I think @saethlin would have a more informed opinion on this

Slightly more informed. I think opt-incr-patched benchmarks do not deserve the same weight as the rest of the benchmark suite because our compilation pipeline very deeply does not support optimizations and incrementality at the same time. Maybe we could make it do that, but swings like this are an indicator that we haven't made the investment, not that this PR is bad.

I know I've pointed at CGU partitioning as the cause of this in the past, and I believe that's technically wrong so I just want to say this before it gets too widespread. The actual partitioning part of the algorithm is very stable to perturbations, what is unstable is the instantiation mode selection, which selects which Instances are partitioned explicitly vs lazily.

It would be pretty cool for these incr-patched benchmarks to identify the number of dirtied CGUs. I think that's basically the number of LLVM_module_codegen_emit_obj executions, which in this PR for clap jumps from 10 to 31. Of course figuring out why this edit now dirties 31 CGUs is harder problem.

saethlin · 2025-03-03T23:58:19Z

compiler/rustc_mir_transform/src/inline.rs

        InstanceKind::Intrinsic(_) | InstanceKind::Virtual(..) => {
            debug!("instance without MIR (intrinsic / virtual)");
-            return Err("implementation limitation");
+            return Err("implementation limitation -- cannot inline intrinsic");


Actually we can inline intrinsics, but we don't want to.

rust/compiler/rustc_mir_transform/src/cross_crate_inline.rs

Lines 37 to 43 in e16a049

if tcx.has_attr(def_id, sym::rustc_intrinsic) {

// Intrinsic fallback bodies are always cross-crate inlineable.

// To ensure that the MIR inliner doesn't cluelessly try to inline fallback

// bodies even when the backend would implement something better, we stop

// the MIR inliner from ever inlining an intrinsic.

return true;

}

saethlin · 2025-03-04T00:01:39Z

Phew, that inliner clause was something. Thank you so much for fixing this.

r=me when CI passes

bors · 2025-03-04T01:29:14Z

☀️ Try build successful - checks-actions
Build commit: 76aedcd (76aedcdeb6b4edbc45a7ef7b705993bf0eb42b8d)

rust-timer · 2025-03-04T02:45:26Z

Finished benchmarking commit (76aedcd): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	16.9%	[0.5%, 59.2%]	4
Regressions ❌ (secondary)	0.6%	[0.4%, 1.8%]	7
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.1%]	22
Improvements ✅ (secondary)	-0.7%	[-3.6%, -0.2%]	30
All ❌✅ (primary)	2.3%	[-0.9%, 59.2%]	26

Max RSS (memory usage)

Results (primary 0.6%, secondary 1.6%)

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)	8.4%	[3.5%, 14.5%]	3
Regressions ❌ (secondary)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-3.2%	[-5.0%, -1.7%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[-5.0%, 14.5%]	9

Cycles

Results (primary 22.2%, 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)	33.9%	[5.8%, 61.9%]	2
Regressions ❌ (secondary)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-2.4%	[-3.6%, -1.0%]	3
All ❌✅ (primary)	22.2%	[-1.0%, 61.9%]	3

Binary size

Results (primary -0.1%, secondary -0.1%)

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.2%	[0.0%, 1.0%]	10
Regressions ❌ (secondary)	0.5%	[0.2%, 1.1%]	4
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.0%]	68
Improvements ✅ (secondary)	-0.2%	[-1.3%, -0.1%]	38
All ❌✅ (primary)	-0.1%	[-0.6%, 1.0%]	78

Bootstrap: 773.808s -> 774.314s (0.07%)
Artifact size: 361.97 MiB -> 361.95 MiB (-0.01%)

compiler-errors · 2025-03-04T03:46:49Z

@bors r=saethlin rollup=never

bors · 2025-03-04T03:46:52Z

📌 Commit d33946c has been approved by saethlin

It is now in the queue for this repository.

scottmcm · 2025-03-05T17:45:38Z

Bumping this over my other never PRs since this affects how library PRs are written
@bors p=1

bors · 2025-03-05T18:39:20Z

⌛ Testing commit d33946c with merge 30f168e...

bors · 2025-03-05T22:07:34Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 30f168e to master...

rust-timer · 2025-03-06T00:30:03Z

Finished benchmarking commit (30f168e): 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

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)	13.6%	[0.4%, 59.2%]	5
Regressions ❌ (secondary)	0.6%	[0.4%, 1.9%]	7
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.1%]	25
Improvements ✅ (secondary)	-0.7%	[-3.4%, -0.1%]	41
All ❌✅ (primary)	2.0%	[-0.9%, 59.2%]	30

Max RSS (memory usage)

Results (primary 2.9%, secondary 2.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.3%	[0.5%, 15.0%]	13
Regressions ❌ (secondary)	2.5%	[0.7%, 4.6%]	59
Improvements ✅ (primary)	-3.4%	[-4.7%, -2.7%]	3
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	2.9%	[-4.7%, 15.0%]	16

Cycles

Results (primary 34.1%, secondary -0.7%)

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)	34.1%	[6.2%, 62.0%]	2
Regressions ❌ (secondary)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	34.1%	[6.2%, 62.0%]	2

Binary size

Results (primary -0.1%, secondary -0.1%)

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.2%	[0.0%, 0.9%]	10
Regressions ❌ (secondary)	0.5%	[0.2%, 1.1%]	4
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.0%]	66
Improvements ✅ (secondary)	-0.2%	[-1.3%, -0.1%]	38
All ❌✅ (primary)	-0.1%	[-0.6%, 0.9%]	76

Bootstrap: 775.73s -> 773.363s (-0.31%)
Artifact size: 362.10 MiB -> 362.14 MiB (0.01%)

Kobzol · 2025-03-11T08:19:57Z

Overall there are more wins than regressions, and this change should help optimizations. The single large regression is unlucky CGU scheduling on an optimized incremental build, which is not very common.

@rustbot label: +perf-regression-triaged

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 3, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 3, 2025

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2025

Auto merge of rust-lang#137907 - compiler-errors:inline-fnonce, r=<try>

66b5f9d

Inline `FnOnce`/`FnMut`/`Fn` shims once again cc rust-lang#137901 r? `@ghost`

compiler-errors commented Mar 3, 2025

View reviewed changes

compiler/rustc_mir_transform/src/inline.rs Outdated Show resolved Hide resolved

compiler-errors force-pushed the inline-fnonce branch from 3be05c2 to 2091084 Compare March 3, 2025 03:18

This comment has been minimized.

Sign in to view

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2025

Auto merge of rust-lang#137907 - compiler-errors:inline-fnonce, r=<try>

633435e

Inline `FnOnce`/`FnMut`/`Fn` shims once again cc rust-lang#137901 r? `@ghost`

This comment has been minimized.

Sign in to view

compiler-errors mentioned this pull request Mar 3, 2025

[do not merge] Inliner experiment 2 #137915

Closed

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2025

Auto merge of rust-lang#137915 - compiler-errors:inliner-experiment-2…

238ff75

…, r=<try> [do not merge] Inliner experiment 2 cc rust-lang#137907 (comment) r? `@ghost`

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 3, 2025

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2025

Auto merge of rust-lang#137915 - compiler-errors:inliner-experiment-2…

0f91b86

…, r=<try> [do not merge] Inliner experiment 2 cc rust-lang#137907 (comment) r? `@ghost`

compiler-errors force-pushed the inline-fnonce branch from 2091084 to 8cdcf3f Compare March 3, 2025 20:24

compiler-errors marked this pull request as ready for review March 3, 2025 20:25

rustbot assigned saethlin Mar 3, 2025

compiler-errors force-pushed the inline-fnonce branch from 8cdcf3f to 0931ef1 Compare March 3, 2025 20:31

Inline FnOnce once again

d33946c

compiler-errors force-pushed the inline-fnonce branch from f8f376e to d33946c Compare March 3, 2025 23:30

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 3, 2025

saethlin reviewed Mar 4, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 4, 2025

bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2025

scottmcm mentioned this pull request Mar 5, 2025

Improve the generic MIR in the default PartialOrd::le and friends #137904

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2025

bors merged commit 30f168e into rust-lang:master Mar 5, 2025
7 checks passed

rustbot added this to the 1.87.0 milestone Mar 5, 2025

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline `FnOnce`/`FnMut`/`Fn` shims once again #137907

Inline `FnOnce`/`FnMut`/`Fn` shims once again #137907

compiler-errors commented Mar 3, 2025 •

edited

Loading

compiler-errors commented Mar 3, 2025

This comment has been minimized.

bors commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

This comment has been minimized.

bors commented Mar 3, 2025

bors commented Mar 3, 2025

This comment has been minimized.

compiler-errors commented Mar 3, 2025 •

edited

Loading

rust-timer commented Mar 3, 2025

Noratrieb commented Mar 3, 2025

rustbot commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

saethlin commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

This comment has been minimized.

bors commented Mar 3, 2025

saethlin commented Mar 3, 2025 •

edited

Loading

saethlin Mar 3, 2025

saethlin commented Mar 4, 2025

bors commented Mar 4, 2025

This comment has been minimized.

rust-timer commented Mar 4, 2025

compiler-errors commented Mar 4, 2025

bors commented Mar 4, 2025

scottmcm commented Mar 5, 2025

bors commented Mar 5, 2025

bors commented Mar 5, 2025

rust-timer commented Mar 6, 2025

Kobzol commented Mar 11, 2025

	if tcx.has_attr(def_id, sym::rustc_intrinsic) {
	// Intrinsic fallback bodies are always cross-crate inlineable.
	// To ensure that the MIR inliner doesn't cluelessly try to inline fallback
	// bodies even when the backend would implement something better, we stop
	// the MIR inliner from ever inlining an intrinsic.
	return true;
	}

Inline FnOnce/FnMut/Fn shims once again #137907

Inline FnOnce/FnMut/Fn shims once again #137907

Conversation

compiler-errors commented Mar 3, 2025 • edited Loading

compiler-errors commented Mar 3, 2025

This comment has been minimized.

bors commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

This comment has been minimized.

bors commented Mar 3, 2025

bors commented Mar 3, 2025

This comment has been minimized.

compiler-errors commented Mar 3, 2025 • edited Loading

rust-timer commented Mar 3, 2025

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

Noratrieb commented Mar 3, 2025

rustbot commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

saethlin commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

compiler-errors commented Mar 3, 2025

This comment has been minimized.

bors commented Mar 3, 2025

saethlin commented Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

saethlin commented Mar 4, 2025

bors commented Mar 4, 2025

This comment has been minimized.

rust-timer commented Mar 4, 2025

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

compiler-errors commented Mar 4, 2025

bors commented Mar 4, 2025

scottmcm commented Mar 5, 2025

bors commented Mar 5, 2025

bors commented Mar 5, 2025

rust-timer commented Mar 6, 2025

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

Kobzol commented Mar 11, 2025

Inline `FnOnce`/`FnMut`/`Fn` shims once again #137907

Inline `FnOnce`/`FnMut`/`Fn` shims once again #137907

compiler-errors commented Mar 3, 2025 •

edited

Loading

compiler-errors commented Mar 3, 2025 •

edited

Loading

saethlin commented Mar 3, 2025 •

edited

Loading