fix(iter::skip): Optimize next and nth implementations of Skip

[austinabell]

This avoids calling nth/next or nth/nth to first skip elements and then get the next one (unless necessary due to usize overflow).

[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]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

Please see the contribution instructions for more information.

[the8472]

The issue here is that an extra element is loaded for skip when either nth or next is called on it. I get that this is a breaking change, because it iterates over one less element and the first is called with nth rather than next

What do you mean by "element is loaded"? Semantically this changes nothing because it still steps the iterator for n times. So the change should be ok, at least as far as the API is concerned.

Anyway, do you have assembly output or benchmarks that show that this is an improvement?

[SkiFire13]

I feel like this also fixes a potential bug:

rust/library/core/src/iter/adapters/skip.rs

Lines 35 to 38 in 5cdab3a

	if unlikely(self.n > 0) {
	self.iter.nth(crate::mem::take(&mut self.n) - 1);
	}
	self.iter.next()

Doesn't this call .next() even if nth returned None? There should have been a ? after the nth().

Other the other hand the change proposed to nth introduces the exact same bug it fixed in next 😂

[austinabell]

The issue here is that an extra element is loaded for skip when either nth or next is called on it. I get that this is a breaking change, because it iterates over one less element and the first is called with nth rather than next

What do you mean by "element is loaded"? Semantically this changes nothing because it still steps the iterator for n times. So the change should be ok, at least as far as the API is concerned.

Anyway, do you have assembly output or benchmarks that show that this is an improvement?

I haven't dug that deep into this. The optimization was more on the amount of items yielded by the iterator, but I can dig into comparing the assembly output or benchmarking now. I feel like it mainly depends on the internal iterator though, so potentially opinionated.

Was more opening to see if a possibility before digging in.

I feel like this also fixes a potential bug:
...

Doesn't this call .next() even if nth returned None? There should have been a ? after the nth().

Yeah, does seem that this would be a bug for non-fused iterators. I'll open a separate PR for this so it isn't attached to this change.

[austinabell]

nvm @SkiFire13 this was previously correct because iterator docs states "Individual iterator implementations may choose to resume iteration, and so calling next() again may or may not eventually start returning Some(Item) again at some point."

Going to revert the last commits changing this to exiting if the skip iterator returns None

[bors]

☔ The latest upstream changes (presumably #99451) made this pull request unmergeable. Please resolve the merge conflicts.

[yaahc]

I'm currently taking a break from the review rotation and didn't realize this one is still assigned to me. Sorry about that.

r? rust-lang/libs

[Mark-Simulacrum]

Please leave a comment with @rustbot ready when you're ready for a review.

[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

[austinabell]

@rustbot ready

[Mark-Simulacrum]

Please also avoid merge commits and instead rebase.

[Mark-Simulacrum]

@rustbot author

#96350 (comment) is also still missing a test, though I think the code has been adjusted in this PR.

[austinabell]

@rustbot ready

[Mark-Simulacrum]

I think the test referenced in #96350 (comment) is not yet added?

[austinabell]

@rustbot ready

Apologies for the lapse on the last fix (also, the comment for that specific test had not loaded for some reason)

[austinabell]

I will note that the logic of the skip iterator seems slightly broken no matter how it's implemented.

The reason I had previously not exited early when loading the skip element is that Iterator docs state that once an iterator returns None, it's possible that it will return Some in the future. The logic that existed and that I switched back to could technically be a bug since the skip nth load could return None, and it would return when the nth call after would have returned Some.

I totally get that avoiding panics for non-fused iterator is more important, but the reason I ask is I'm wondering if the documentation can be improved for Iterator or Skip to denote this?

[Mark-Simulacrum]

One more nit and please squash the commits.

[Mark-Simulacrum]

@bors r+ rollup=iffy

[bors]

📌 Commit 00bc9e8 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit 00bc9e8 with merge 80ed61f...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 80ed61f to master...

[rust-timer]

Finished benchmarking commit (80ed61f): comparison url.

Instruction count

• Primary benchmarks: ✅ relevant improvement found
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	-0.8%	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	-0.8%	1

Max RSS (memory usage)

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: ❌ relevant regressions found

	mean1	max	count2
Regressions ❌ (primary)	6.7%	6.7%	1
Regressions ❌ (secondary)	2.8%	3.5%	2
Improvements ✅ (primary)	-3.9%	-7.1%	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	-7.1%	4

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: ❌ relevant regression found

	mean1	max	count2
Regressions ❌ (primary)	3.4%	3.4%	1
Regressions ❌ (secondary)	2.6%	2.6%	1
Improvements ✅ (primary)	-2.8%	-2.8%	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	3.4%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

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

2. number of relevant changes ↩ ↩² ↩³