Implement iterator specialization traits on more adapters by the8472 · Pull Request #85528 · rust-lang/rust

the8472 · 2021-05-20T20:58:07Z

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 · 2021-05-20T20:58:10Z

r? @yaahc

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

joshtriplett · 2021-05-20T22:10:28Z

@bors try @rust-timer queue

rust-timer · 2021-05-20T22:10:29Z

Awaiting bors try build completion.

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

bors · 2021-05-20T22:10:51Z

⌛ Trying commit 838b07beaca15f5614619bcee070ef5263a805ed with merge e056521d347c0ec9e2f6a0a140406f4b0a06c011...

bors · 2021-05-20T23:00:32Z

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

rust-timer · 2021-05-20T23:00:34Z

Queued e056521d347c0ec9e2f6a0a140406f4b0a06c011 with parent 99e3aef, future comparison URL.

rust-timer · 2021-05-21T02:53:23Z

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

the8472 · 2021-05-21T12:59:26Z

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

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

the8472 · 2021-06-12T09:18:17Z

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)

the8472 · 2021-06-12T09:27:06Z

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

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().

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?

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.

@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?

yaahc · 2021-06-21T22:13:26Z

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

bors · 2021-07-29T02:58:58Z

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

bors · 2023-07-28T02:47:15Z

⌛ Trying commit 3af552a with merge 1f4585dbcb1b7f4971a8c2a76d1b74dff98a774a...

bors · 2023-07-28T04:00:35Z

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

rust-timer · 2023-07-28T14:05:04Z

Finished benchmarking commit (1f4585dbcb1b7f4971a8c2a76d1b74dff98a774a): 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-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.

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	2.6%	[2.3%, 2.8%]	2
Improvements ✅ (primary)	-2.2%	[-4.9%, -0.5%]	3
Improvements ✅ (secondary)	-2.5%	[-2.9%, -2.1%]	2
All ❌✅ (primary)	-0.9%	[-4.9%, 2.8%]	4

Cycles

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

Binary size

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.0%	[0.0%, 0.1%]	10
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.1%, 0.1%]	13

Bootstrap: 652.994s -> 652s (-0.15%)

workingjubilee · 2023-07-28T20:17:58Z

Hell yeah, a needle wiggled!

I don't think anyone should derive much information from this, given that we don't know yet if this is a good suite of benchmarks, but it's admittedly more than zero!

workingjubilee · 2023-07-31T21:17:45Z

idk, despite my enthusiasm for the exercising of the runtime perf test suite, with what feels like minimal/unclear benefit, it's hard to argue that the addition of a bunch of hard-to-audit unsafe code is really an improvement for std in general?

workingjubilee · 2023-07-31T21:21:24Z

r? libs-api

Needs FCP due to inducing user-facing behavior changes in some iterator adapters in order to make the given microbenchmark go zoom. Behavior change is subtle, however, and doesn't seem to be earthshattering, see this comment.

Amanieu · 2023-08-04T12:09:27Z

This causes a minor behavior change, see #85528 (comment). However I don't expect this to be an issue in practice since this behavior was never guaranteeed.

@rfcbot fcp merge

rfcbot · 2023-08-04T12:09:28Z

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

dtolnay · 2023-09-28T03:24:40Z

This is waiting on 1 more checkbox to enter FCP, among them @the8472's, whose PR this is. I almost but not quite feel at liberty to check your box for you. Want to do the honors? 😉

the8472 · 2023-09-28T03:36:07Z

Sure, I can do that if that's ok? Approving one's own stuff always seems questionable. But I guess we'd have to tell the bot to remove one of the boxes, which we don't. 🤷

rfcbot · 2023-09-28T03:45:05Z

🔔 This is now entering its final comment period, as per the review above. 🔔

dtolnay · 2023-09-28T03:48:38Z

@the8472 "N–2 of the team thinks that this is a good idea (and no objections)" is the bar. In my experience it's always been by design that the PR author would be included in that count if they're a member of the relevant team. Actually, most often the PR author is the one who proposes the FCP in the first place in that situation, so then they automatically get counted.

rfcbot · 2023-10-08T03:55:33Z

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.

the8472 · 2024-01-10T18:11:11Z

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

Thank you! Nice work.

dtolnay · 2024-01-21T02:38:25Z

@bors r+

bors · 2024-01-21T02:38:27Z

📌 Commit 37d26c7 has been approved by dtolnay

It is now in the queue for this repository.

bors · 2024-01-21T03:50:18Z

⌛ Testing commit 37d26c7 with merge 7328588...

bors · 2024-01-21T08:05:18Z

💥 Test timed out

rust-log-analyzer · 2024-01-21T09:19:34Z

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

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

the8472 · 2024-01-21T09:23:36Z

Apple runner hanging.

@bors retry

bors · 2024-01-21T11:17:49Z

⌛ Testing commit 37d26c7 with merge fa40433...

bors · 2024-01-21T13:15:29Z

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

rust-timer · 2024-01-21T15:11:53Z

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%)

rust-highfive assigned yaahc May 20, 2021

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

the8472 force-pushed the iter-markers branch from ae55176 to 838b07b Compare May 20, 2021 21:01

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

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 21, 2021

yaahc reviewed Jun 3, 2021

View reviewed changes

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 force-pushed the iter-markers branch 2 times, most recently from 3b3e1bb to 5e1a4a3 Compare June 12, 2021 09:12

the8472 commented Jun 12, 2021

View reviewed changes

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

rust-highfive assigned m-ou-se and unassigned yaahc Jun 21, 2021

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

the8472 force-pushed the iter-markers branch from 5e1a4a3 to 634a9b0 Compare July 29, 2021 22:44

This comment has been minimized.

Sign in to view

the8472 added 4 commits January 10, 2024 18:55

implement TrustedLen for StepBy

a2a7caa

implement TrustedRandomAccess and TrustedLen for Skip

451a3b1

bench trustedrandomaccess specialization in zip

3aa7313

Implement in-place iteratation markers for iter::{Copied, Cloned}

37d26c7

dtolnay approved these changes Jan 21, 2024

View reviewed changes

Uh oh!

Conversation

the8472 commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 20, 2021

Uh oh!

joshtriplett commented May 20, 2021

Uh oh!

rust-timer commented May 20, 2021

Uh oh!

bors commented May 20, 2021

Uh oh!

bors commented May 20, 2021

Uh oh!

rust-timer commented May 20, 2021

Uh oh!

rust-timer commented May 21, 2021

Uh oh!

the8472 commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

the8472 commented Jun 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaahc commented Jun 21, 2021

Uh oh!

bors commented Jul 29, 2021

Uh oh!

This comment has been minimized.

bors commented Jul 28, 2023

Uh oh!

bors commented Jul 28, 2023

Uh oh!

This comment has been minimized.

rust-timer commented Jul 28, 2023

Overall result: no relevant changes - no action needed

Uh oh!

workingjubilee commented Jul 28, 2023

Uh oh!

workingjubilee commented Jul 31, 2023

Uh oh!

workingjubilee commented Jul 31, 2023

Uh oh!

Amanieu commented Aug 4, 2023

Uh oh!

rfcbot commented Aug 4, 2023 • edited by the8472 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtolnay commented Sep 28, 2023

Uh oh!

the8472 commented Sep 28, 2023

Uh oh!

rfcbot commented Sep 28, 2023

Uh oh!

dtolnay commented Sep 28, 2023

Uh oh!

rfcbot commented Oct 8, 2023

Uh oh!

the8472 commented Jan 10, 2024

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

dtolnay commented Jan 21, 2024

Uh oh!

bors commented Jan 21, 2024

Uh oh!

bors commented Jan 21, 2024

Uh oh!

bors commented Jan 21, 2024

the8472 commented May 20, 2021 •

edited

Loading

the8472 commented May 21, 2021 •

edited

Loading

rfcbot commented Aug 4, 2023 •

edited by the8472

Loading