Tentatively #[inline] Option::from

[SoniEx2]

Probably not gonna have much of an impact because into can't be inlined. But let's try it?

(please perf run this whenever)

[rust-highfive]

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

[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

[joshtriplett]

@SoniEx2 I'd be mildly surprised that this isn't already being inlined. Do you have some code in release mode where this isn't being inlined?

Happy to put this through a perf run, but before doing so, do you have a godbolt link or similar showing that this isn't being inlined?

[SoniEx2]

we do not. we're just throwing stuff at the wall and seeing what sticks. tho we were surprised to find it not marked #[inline]. we wonder if it would do anything (maybe make things worse?).

[joshtriplett]

Usually LLVM manages to figure out inlining for tiny functions like this on its own; #[inline] and #[inline(always)] hints are useful when it doesn't figure things out.

I'd recommend trying some experiments with godbolt, typing code at rustc and seeing what the assembly looks like, and if you see a call to a tiny stub function that should be inlined but isn't, that's a good place to add #[inline].

[Noratrieb]

Since this is a generic function, it gets duplicated into the codegen units of the caller anyways, making inlining possible. The inlinehint from it doesn't have too much of an impact usually and LLVM will just inline it.

[bugadani]

@Nilstrieb in that case why was this useful?

[Noratrieb]

#[inline(always)] gives stronger hint to inline it. Also, I'm not saying that adding #[inline] to generic functions is always useless, but it usually is.

[bugadani]

I guess my question should have been phrased as "Why was that function not inlined in the first place (without an inline attribute), since this is also generic?". Apologies for the poor formulation. Is there a difference between blanket impls and generic impls (i.e. impl Foo for T vs impl Foo for Option<T>)?

[Noratrieb]

No, there is no difference between those. LLVMs inliner (and MIR inlining) just inline whatever their heuristics say should be inlined, which is often correct, but not always. Functions being generic just allows inlining in the first place (because the function implementation has to by copied into the user codegen unit), whether it actually happens is up to the inliner.

[thomcc]

@SoniEx2 I'd be mildly surprised that this isn't already being inlined. Do you have some code in release mode where this isn't being inlined?

A lot of our generic methods don't end up getting inlined when optimizing for size. At a previous job I was doing a lot of embedded stuff, and at -Copt-level=z (even with -Zbuild-std) we'd frequently see dozens of copies of tiny generic functions. FWIW, this would only happen on certain targets (I guess the set of LLVM passes we run depends somewhat on the target? Since not all targets exhibited this issue), which makes the issue more annoying.

Some (but not all) of these would be inlined anyway, but end up in the output regardless, but this is an LLVM bug (some discussion in #96624). This was never that big of a deal (cost was likely under a kilobyte of code total which didn't make a difference for us), so I never dug that deeply (also IMO ideally we wouldn't need to mark these with #[inline], and hopefully won't in the future).

While I never saw Option::from, I suspect this is just because it doesn't get used much in that code, since this is absolutely the kind of thing we'd see copies of.

[nikic]

Because this is a recurring problem, I've spent some time trying to understand just why #[inline] makes a difference for generic functions, even though it ostensibly shouldn't. This is my conclusion: #102539

Basically, if we ignore the opt-for-size case (where -Z share-generics causes extra issues), the relevant distinction is whether the function is instantiated per-crate or per-CGU. Generic functions are instantiated per-crate, while #[inline] functions are instantiated per-CGU.

[Kobzol]

@bors try @rust-timer queue

The queue is empty anyway, so it doesn't hurt to see what this does I suppose.

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 73fb318 with merge 71bfd8ddf728cd51898f6b895569e3ab104ff9e1...

[bors]

☀️ Try build successful - checks-actions
Build commit: 71bfd8ddf728cd51898f6b895569e3ab104ff9e1 (71bfd8ddf728cd51898f6b895569e3ab104ff9e1)

[rust-timer]

Queued 71bfd8ddf728cd51898f6b895569e3ab104ff9e1 with parent 744e397, future comparison URL.

[rust-timer]

Finished benchmarking commit (71bfd8ddf728cd51898f6b895569e3ab104ff9e1): comparison URL.

Overall result: no relevant changes - no 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.

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

Instruction count

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

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.

	mean1	range	count2
Regressions ❌ (primary)	6.9%	[3.2%, 13.5%]	4
Regressions ❌ (secondary)	2.6%	[1.9%, 3.9%]	5
Improvements ✅ (primary)	-8.7%	[-19.5%, -2.0%]	22
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-6.3%	[-19.5%, 13.5%]	26

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.1%	[-29.5%, -1.4%]	199
Improvements ✅ (secondary)	-4.3%	[-14.3%, -1.4%]	93
All ❌✅ (primary)	-6.1%	[-29.5%, -1.4%]	199

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²

[Kobzol]

The cycle and RSS aren't real, the perf. run has hit a period where the perf. machine has a different config, but it's not reflected in master perf. results yet.

Seeing as there are no instruction count improvements, I'd be inclined to close this.

[JohnCSimon]

@SoniEx2
Ping from triage: Can you please post the status of this PR?

Seeing as there are no instruction count improvements, I'd be inclined to close this.

Maybe close it?

Thank you.