slice: #[inline] a couple iterator methods.

[emilio]

The one I care about and actually saw in the wild not getting inlined is
clone(). We ended up doing a whole function call for something that just
copies two pointers.

I ended up marking as_slice / as_ref as well because make_slice is
inline(always) itself, and is also the kind of think that can kill
performance in hot loops if you expect it to get inlined. But happy to
undo those.

[rust-highfive]

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 r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. 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

[rust-highfive]

r? @Mark-Simulacrum

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

[Mark-Simulacrum]

Hm, I would have expected all of these methods to be generic and so automatically eligible for inlining. Do you have examples of code that doesn't see them inlined? Is this with incremental on perhaps?

r? @nikic to check if we should be filing such bugs upstream or otherwise investigating.

[emilio]

Hm, I would have expected all of these methods to be generic and so automatically eligible for inlining. Do you have examples of code that doesn't see them inlined? Is this with incremental on perhaps?

I hit this on a release Firefox build (which IIRC use -O2 for rust, and LTO). In particular it's this code, which calls the derived SelectorIter::clone, which gets inlined, but that has a slice::Iter as a member, whose clone() doesn't.

I think I saw it in a non-incremental build but I can test this again if needed (not sure if that's your question). In a somewhat simplified example it does get inlined: https://rust.godbolt.org/z/eoo7Wcrfj, but the real code is much more complicated so... :(

[Dylan-DPC]

@nikic this is ready for review

[nikic]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 93e587b with merge 4cb99581146765206ae6636357b2792e9bf77c12...

[bors]

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

[rust-timer]

Queued 4cb99581146765206ae6636357b2792e9bf77c12 with parent f382c27, future comparison URL.

[rust-timer]

Finished benchmarking commit (4cb99581146765206ae6636357b2792e9bf77c12): comparison URL.

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

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.5%, -0.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.0%	[-1.5%, -0.6%]	2

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.5%, -1.5%]	3
All ❌✅ (primary)	-	-	0

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)	4.7%	[4.4%, 5.1%]	2
Improvements ✅ (primary)	-2.2%	[-2.3%, -2.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.2%	[-2.3%, -2.0%]	3

Footnotes

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

2. number of relevant changes ↩ ↩² ↩³

[nikic]

@bors r+

See #102539 for why #[inline] on generic functions makes a difference. Though I'd generally expect them to be inlined in the final LTO result, maybe there's one more interaction I'm missing.

[bors]

📌 Commit 93e587b has been approved by nikic

It is now in the queue for this repository.

[bors]

⌛ Testing commit 93e587b with merge 0265a3e...

[bors]

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 0265a3e to master...

[rust-timer]

Finished benchmarking commit (0265a3e): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

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

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.3%, 0.7%]	4
Improvements ✅ (primary)	-0.8%	[-1.5%, -0.5%]	8
Improvements ✅ (secondary)	-1.4%	[-1.8%, -1.2%]	6
All ❌✅ (primary)	-0.8%	[-1.5%, -0.5%]	8

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)	2.7%	[2.0%, 3.5%]	3
Regressions ❌ (secondary)	2.6%	[0.9%, 4.8%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.9%	[-4.9%, -4.9%]	2
All ❌✅ (primary)	2.7%	[2.0%, 3.5%]	3

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)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-2.8%	[-3.9%, -2.3%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.8%	[-3.9%, -2.3%]	4

Footnotes

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

2. number of relevant changes ↩ ↩² ↩³

[nikic]

Perf results are more positive than negative, I think that's all that matters for this kind of change. The regressions are minor ones in secondary benchmarks.

@rustbot label: +perf-regression-triaged