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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

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
Collaborator

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
@timvermeulen timvermeulen mentioned this pull request Oct 6, 2020
Open
3 tasks
@ecstatic-morse
Copy link
Contributor

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 7, 2020

⌛ Trying commit 3f22d0d with merge e64db831273cd38b9834dbdc2be0b6dd02f22f70...

@bors
Copy link
Contributor

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

@timvermeulen @pickfire
Fix whitespace
dd58782 
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
@the8472 the8472 mentioned this pull request Jul 22, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 31, 2021
@bors
Auto merge of rust-lang#87387 - the8472:slice-iter-advance_by, r=scot…
6b0b07d 
…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
Reviewers

@pickfire pickfire pickfire left review comments

Assignees

@m-ou-se m-ou-se

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet