Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement iterator specialization traits on more adapters #85528

Merged
merged 4 commits into from
Jan 21, 2024

Conversation

the8472
Copy link
Member

@the8472 the8472 commented May 20, 2021

This adds

  • TrustedLen to Skip and StepBy
  • TrustedRandomAccess to Skip
  • InPlaceIterable and SourceIter to Copied and Cloned

The first two might improve performance in the compiler itself since skip is used in several places. Constellations that would exercise the last point are probably rare since it would require an owning iterator that has references as Items somewhere in its iterator pipeline.

Improvements for Skip:

# old
test iter::bench_skip_trusted_random_access                     ... bench:       8,335 ns/iter (+/- 90)

# new
test iter::bench_skip_trusted_random_access                     ... bench:       2,753 ns/iter (+/- 27)

@rust-highfive
Copy link
Collaborator

r? @yaahc

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2021
@joshtriplett
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 20, 2021
@bors
Copy link
Contributor

bors commented May 20, 2021

⌛ Trying commit 838b07beaca15f5614619bcee070ef5263a805ed with merge e056521d347c0ec9e2f6a0a140406f4b0a06c011...

@bors
Copy link
Contributor

bors commented May 20, 2021

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

@rust-timer
Copy link
Collaborator

Queued e056521d347c0ec9e2f6a0a140406f4b0a06c011 with parent 99e3aef, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (e056521d347c0ec9e2f6a0a140406f4b0a06c011): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 21, 2021
@the8472
Copy link
Member Author

the8472 commented May 21, 2021

Looks like noise. Or if it isn't then it's tiny impact at least. I guess the skip uses in the compiler aren't critical then.

I'll write some benchmarks to see if these actually improve some assembly.

library/core/src/iter/adapters/cloned.rs Outdated Show resolved Hide resolved
library/core/src/iter/adapters/skip.rs Outdated Show resolved Hide resolved
@the8472 the8472 force-pushed the iter-markers branch 2 times, most recently from 3b3e1bb to 5e1a4a3 Compare June 12, 2021 09:12
@the8472
Copy link
Member Author

the8472 commented Jun 12, 2021

I have added a benchmark

# old
test iter::bench_skip_trusted_random_access                     ... bench:       8,335 ns/iter (+/- 90)

# new
test iter::bench_skip_trusted_random_access                     ... bench:       2,753 ns/iter (+/- 27)

if Self::MAY_HAVE_SIDE_EFFECT && idx == 0 {
for skipped_idx in 0..self.n {
drop(try_get_unchecked(&mut self.iter, skipped_idx));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In light of #85969 I must note that this alters (undocumented) side-effects for backwards iteration, albeit in less surprising ways.

This block is written to preserve side-effects of the skipped portion of the iterator during forward iteration. During backward iteration it will lead to the whole skipped portion being drained on the last (backwards) step instead of only the first item beyond the skipped portion, i.e. it behaves as if the last item were obtained by a next() instead of next_back().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so, my interpretation of what you're saying: we're picking idx == 0, which might not actually be the first index the user iterates over (and won't be in backwards iteration).

But I think my interpretation is probably wrong -- I've tried several times and I cannot manage to find a case where your patch causes different behavior in map(|i| dbg!(i)).skip(n) behavior in forward or backward iteration. Do you have an example where it makes causes an observable difference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next_back doesn't touch the inner's skipped prefix. But anything that goes backwards via __iterator_get_unchecked (e.g. Zip) would once it hits index 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@the8472 Okay, this is a slight behavioral change to behavior we never made a promise about. I think it's a change we're allowed to make, but to be safe, that decision should be left to t-libs-api FCP.

Do you mind writing up the details about before/after differences so that that can be done?

@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 19, 2021
@yaahc
Copy link
Member

yaahc commented Jun 21, 2021

Going to pass this one off because I'm not familiar enough with this part of std to approve this by myself.

r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned yaahc Jun 21, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2021
@bors
Copy link
Contributor

bors commented Jul 29, 2021

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

@rust-log-analyzer

This comment has been minimized.

@rfcbot
Copy link

rfcbot commented Oct 8, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 12, 2023
@the8472
Copy link
Member Author

the8472 commented Jan 10, 2024

FCP complete, this should be ready to merge. ping @Amanieu or reassign to a libs reviewer now that the libs-api question is settled.

@dtolnay dtolnay assigned dtolnay and unassigned Amanieu Jan 21, 2024
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Nice work.

@dtolnay
Copy link
Member

dtolnay commented Jan 21, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 21, 2024

📌 Commit 37d26c7 has been approved by dtolnay

It is now in the queue for this repository.

@bors 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 Jan 21, 2024
@bors
Copy link
Contributor

bors commented Jan 21, 2024

⌛ Testing commit 37d26c7 with merge 7328588...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2024
Implement iterator specialization traits on more adapters

This adds

* `TrustedLen` to `Skip` and `StepBy`
* `TrustedRandomAccess` to `Skip`
* `InPlaceIterable` and `SourceIter` to  `Copied` and `Cloned`

The first two might improve performance in the compiler itself since `skip` is used in several places. Constellations that would exercise the last point are probably rare since it would require an owning iterator that has references as Items somewhere in its iterator pipeline.

Improvements for `Skip`:

```
# old
test iter::bench_skip_trusted_random_access                     ... bench:       8,335 ns/iter (+/- 90)

# new
test iter::bench_skip_trusted_random_access                     ... bench:       2,753 ns/iter (+/- 27)
```
@bors
Copy link
Contributor

bors commented Jan 21, 2024

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 21, 2024
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@the8472
Copy link
Member Author

the8472 commented Jan 21, 2024

Apple runner hanging.

@bors retry

@bors 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 Jan 21, 2024
@bors
Copy link
Contributor

bors commented Jan 21, 2024

⌛ Testing commit 37d26c7 with merge fa40433...

@bors
Copy link
Contributor

bors commented Jan 21, 2024

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing fa40433 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 21, 2024
@bors bors merged commit fa40433 into rust-lang:master Jan 21, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 21, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fa40433): 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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
-1.0% [-1.5%, -0.5%] 2
All ❌✅ (primary) -0.5% [-0.5%, -0.5%] 1

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)
2.7% [2.7%, 2.7%] 1
Regressions ❌
(secondary)
3.3% [3.3%, 3.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.7% [2.7%, 2.7%] 1

Cycles

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

Binary size

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

Bootstrap: 662.48s -> 663.269s (0.12%)
Artifact size: 308.40 MiB -> 308.35 MiB (-0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet