Skip to content

Conversation

timvermeulen
Copy link
Contributor

Part of #77404.

Would be nice if we can get away with getting rid of nth[_back] altogether, but if not, we can keep nth and advance_by alongside each other.

Also see #76909 (comment).

cc @ecstatic-morse @scottmcm

@rust-highfive
Copy link
Contributor

r? @withoutboats

(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 Oct 6, 2020
@ecstatic-morse
Copy link
Contributor

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Oct 7, 2020

⌛ Trying commit 3f22d0d with merge e64db831273cd38b9834dbdc2be0b6dd02f22f70...

@bors
Copy link
Collaborator

bors commented Oct 7, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: e64db831273cd38b9834dbdc2be0b6dd02f22f70 (e64db831273cd38b9834dbdc2be0b6dd02f22f70)

@rust-timer
Copy link
Collaborator

Queued e64db831273cd38b9834dbdc2be0b6dd02f22f70 with parent 98edd1f, future comparison URL.

@scottmcm
Copy link
Member

scottmcm commented Oct 7, 2020

We'll see what perf says, but honestly I'd recommend leaving the specific nth -- on normal user iterators it would be fine to just have advance_by, but slice iterators tend to implement all of them rather leaving it to the optimizer (to fold the checks between advance_by+next, for example) because they're used so pervasively that making LLVM's life easier helps compile times.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (e64db831273cd38b9834dbdc2be0b6dd02f22f70): 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

Co-authored-by: Ivan Tham <[email protected]>
@timvermeulen
Copy link
Contributor Author

@scottmcm I'm totally happy to have it either way, I know how picky slice::Iter can be. Right now I don't really have a reason to assume that advance_by followed by a next is less efficient in any way than the current nth implementation, which kind of does both? I could try adding some benchmarks to see if I can get them to perform differently.

@camelid camelid added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 23, 2020
@crlf0710
Copy link
Member

Triage: What's the status on this?

@JohnCSimon
Copy link
Member

@timvermeulen Ping from Triage: What's the status on this?

@Dylan-DPC-zz
Copy link

r? @KodrAus

@rust-highfive rust-highfive assigned KodrAus and unassigned withoutboats Dec 1, 2020
@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 Dec 18, 2020
@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 Jan 11, 2021
@Dylan-DPC-zz
Copy link

r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned KodrAus Jan 13, 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 Feb 1, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Feb 2, 2021

I'm totally happy to have it either way, I know how picky slice::Iter can be. Right now I don't really have a reason to assume that advance_by followed by a next is less efficient in any way than the current nth implementation, which kind of does both? I could try adding some benchmarks to see if I can get them to perform differently.

Adding an implementation for advance_by is fine, but removing the nth implementation to use the default advance_by + next needs a bit more convincing, since this is a very important iterator. Does it result in the same assembly? Is the performance the same as before?

(Or if you change the PR to only add advance_[back_]_by: r=me.)

@m-ou-se m-ou-se added T-libs Relevant to the library team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-iterators Area: Iterators and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 2, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2021
@JohnCSimon
Copy link
Member

@timvermeulen Ping again from Triage: Can you please post the status on this?

@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Feb 22, 2021
@JohnCSimon
Copy link
Member

I'm closing this as inactive. If you like, you can reopen this MR and continue.

@JohnCSimon JohnCSimon closed this Feb 22, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 31, 2021
…tmcm

Implement advance_by, advance_back_by for slice::{Iter, IterMut}

Part of rust-lang#77404.

Picking up where rust-lang#77633 was closed.

I have addressed rust-lang#77633 (comment) by restoring `nth` and `nth_back`. So according to that comment this should already be r=m-ou-se, but it has been sitting for a while.
@timvermeulen timvermeulen deleted the slice_iter_advance_by branch July 8, 2022 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.