Add #[inline] to the Into for From impl

[saethlin]

I was skimming through the standard library MIR and I noticed a handful of very suspicious Into::into calls in alloc. Since this is a trivial wrapper function, #[inline(always)] seems appropriate.; #[inline] works too and is a lot less spooky.

r? @thomcc

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[thomcc]

I think the concern is that because of bottom-up inlining, U::from may already have been inlined into Into::into by the time this applies (the decision may be made because Into::into may be the only call-site of U::from). If U::from is large, then this may be a mistake, as the compiler's decision not to inline Into::into may be correct.

Unless the MIR inliner changes the decision here, I'm not sure we want to do this.

[thomcc]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 5f57df852125462741d6e29ab5d7e349f532f27f with merge 4a30962485784277f1078c279246fa2925a3ef67...

[bors]

☀️ Try build successful - checks-actions
Build commit: 4a30962485784277f1078c279246fa2925a3ef67 (4a30962485784277f1078c279246fa2925a3ef67)

[rust-timer]

Finished benchmarking commit (4a30962485784277f1078c279246fa2925a3ef67): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.8%, 1.0%]	2
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	1
Improvements ✅ (primary)	-0.6%	[-1.0%, -0.4%]	10
Improvements ✅ (secondary)	-0.9%	[-1.6%, -0.3%]	20
All ❌✅ (primary)	-0.4%	[-1.0%, 1.0%]	12

Max RSS (memory usage)

Results

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)	3.6%	[3.4%, 3.9%]	2
Regressions ❌ (secondary)	2.6%	[0.6%, 4.5%]	49
Improvements ✅ (primary)	-3.2%	[-5.5%, -0.1%]	4
Improvements ✅ (secondary)	-0.7%	[-0.7%, -0.7%]	1
All ❌✅ (primary)	-0.9%	[-5.5%, 3.9%]	6

Cycles

Results

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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-1.0%	[-2.1%, -0.5%]	14
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

[thomcc]

I'm somewhat inclined to accept this despite that logic. My reasoning is basically:

1. There are reasons this should slow down rustc. Specifically, it causes LLVM to potentially have to do more work.
2. In spite of that, the performance profile is mostly positive (both for instruction counts and for cycles).
3. This to me means it probably holds that it's a net win for user programs too.

[saethlin]

There are reasons this should slow down rustc. Specifically, it causes LLVM to potentially have to do more work.

Yeah, I think MIR optimizations really confuse this sort of consideration. Without any #[inline] attribute, this function is a MIR optimization barrier, and depending on how things look after the whole MIR opt pipeline, we can create more work for LLVM or less.

Should we try just #[inline]?

[thomcc]

Sure.

[saethlin]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 9bc06ffeb24baa44e7ab003ed691a42fc69eb3e0 with merge 3a40dc8f94334627847ad5dbcf56872324975ddb...

[cjgillot]

I think the concern is that because of bottom-up inlining, U::from may already have been inlined into Into::into by the time this applies (the decision may be made because Into::into may be the only call-site of U::from). If U::from is large, then this may be a mistake, as the compiler's decision not to inline Into::into may be correct.

Unless the MIR inliner changes the decision here, I'm not sure we want to do this.

That's not exactly how MIR inliner works. The MIR inliner works on polymorphic MIR. In this PR, it would try to inline <U as From<T>>::from. As this is polymorphic, it has no way to resolve to concrete MIR, so it does nothing. However, this discussion is only viable for calls to unresolved trait methods, and in general inline(always) should be used with care.

I don't know enough of LLVM inlining to discuss it.

[bors]

☀️ Try build successful - checks-actions
Build commit: 3a40dc8f94334627847ad5dbcf56872324975ddb (3a40dc8f94334627847ad5dbcf56872324975ddb)

[bors]

☀️ Try build successful - checks-actions
Build commit: 3a40dc8f94334627847ad5dbcf56872324975ddb (3a40dc8f94334627847ad5dbcf56872324975ddb)

[rust-timer]

Finished benchmarking commit (3a40dc8f94334627847ad5dbcf56872324975ddb): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.8%]	4
Regressions ❌ (secondary)	0.4%	[0.4%, 0.5%]	3
Improvements ✅ (primary)	-0.7%	[-1.4%, -0.3%]	16
Improvements ✅ (secondary)	-1.2%	[-1.6%, -0.8%]	14
All ❌✅ (primary)	-0.4%	[-1.4%, 0.8%]	20

Max RSS (memory usage)

Results

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.4%	[0.5%, 3.5%]	4
Improvements ✅ (primary)	-3.7%	[-6.6%, -0.1%]	4
Improvements ✅ (secondary)	-1.4%	[-4.1%, -0.5%]	8
All ❌✅ (primary)	-3.7%	[-6.6%, -0.1%]	4

Cycles

Results

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.6%	[0.6%, 0.6%]	2
Improvements ✅ (primary)	-1.1%	[-1.3%, -0.5%]	6
Improvements ✅ (secondary)	-1.1%	[-1.6%, -0.6%]	4
All ❌✅ (primary)	-1.1%	[-1.3%, -0.5%]	6

[scottmcm]

Those results look great 🚀

The PR is showing stdarch as changing. Is that intentional?

r=me without the submodule change.

(Though I hope we one day get a compiler feature that we don't need to put all these inlines in "obvious" places in core!)

[saethlin]

It was not intentional, oops. (I can't approve on rust-lang/rust)

I agree with your general point. In this particular case, #109247 would probably have taken care of this, but it is blocked by an issue I don't know how to debug.

[scottmcm]

@bors r+

[bors]

📌 Commit badfb17 has been approved by scottmcm

It is now in the queue for this repository.

[bors]

⌛ Testing commit badfb17 with merge 24a69af...

[bors]

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 24a69af to master...

[rust-timer]

Finished benchmarking commit (24a69af): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-1.5%, -0.4%]	15
Improvements ✅ (secondary)	-0.9%	[-1.4%, -0.3%]	19
All ❌✅ (primary)	-0.7%	[-1.5%, 0.8%]	16

Max RSS (memory usage)

Results

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.7%	[0.4%, 1.0%]	11
Improvements ✅ (primary)	-3.3%	[-6.5%, -0.1%]	4
Improvements ✅ (secondary)	-1.7%	[-2.3%, -0.7%]	3
All ❌✅ (primary)	-3.3%	[-6.5%, -0.1%]	4

Cycles

Results

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.8%	[0.4%, 2.1%]	6
Improvements ✅ (primary)	-1.3%	[-1.8%, -0.8%]	6
Improvements ✅ (secondary)	-1.7%	[-2.0%, -1.4%]	3
All ❌✅ (primary)	-1.3%	[-1.8%, -0.8%]	6