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

Slow performance of std::iter::Rev with iterator adapters using std::iter::Iterator::nth() #54054

Open
22 of 25 tasks
koalatux opened this issue Sep 8, 2018 · 6 comments
Open
22 of 25 tasks
Labels
A-iterators Area: Iterators C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@koalatux
Copy link
Contributor

koalatux commented Sep 8, 2018

The following example shows a benchmark of the iterator adapter step_by(). Once using step_by() directly on the range and once with a redirection via rev().

use test::Bencher;

#[bench]
fn bench_forward_skip(b: &mut Bencher) {
    b.iter(|| (0..10001).step_by(100).sum::<i32>());
}

#[bench]
fn bench_reverse_skip(b: &mut Bencher) {
    b.iter(|| (0..10001).rev().step_by(100).sum::<i32>());
}

Running this benchmark with the current nightly shows these results:

test tests::bench_forward_skip ... bench:         137 ns/iter (+/- 6)
test tests::bench_reverse_skip ... bench:       3,878 ns/iter (+/- 327)

step_by() makes use of nth() of the adapted iterator. A range provides an optimized version of nth(), but by using rev() we get to use the default implementation of nth().

We should Extend std::iter::DoubleEndedIterator to provide a new method maybe nth_back() or rnth() with a default implementation which then can get adapted by rev(). Similar to the already existing next_back(), try_rfold(), rfold() and rfind().

Update:

nth_back() has been merged in #56802. Types which have a specialized nth() and implement DoubleEndedIterator are candidates for a specialized nth_back(). The following list shows these candidates and their implementation status:

@csmoe csmoe added I-slow Issue: Problems and improvements with respect to performance of generated code. NLL-performant Working towards the "performance is good" goal labels Sep 8, 2018
@orium orium removed the NLL-performant Working towards the "performance is good" goal label Sep 9, 2018
@clarfonthey
Copy link
Contributor

I would probably call this nth_back, personally. But I agree that it's a good method to have.

@robsmith11
Copy link

robsmith11 commented Dec 13, 2018

I ran into this issue as well I think.

I was expecting vec.iter().rev().skip(n) to be efficient, but it runs 10x slower than vec[0..(vec.len() - n)].iter().rev(), which I would have expected to be similar.

@clarfonthey
Copy link
Contributor

If I get the time later, I'll make a PR to add an nth_back method to DoubleEndedIterator with a few basic overrides. I'll try to get to it by tomorrow at least.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 21, 2018
Add DoubleEndedIterator::nth_back

As suggested by rust-lang#54054. This doesn't fix that issue, as this doesn't add enough implementations to optimise that specific use case, but it adds the method and a few (relatively) trivial overrides to work as an initial implementation.

It's probably going to be a lot of work adding `nth_back` implementations everywhere, and I don't have the time to include it all in this commit. But, it's a start. :)
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 21, 2018
Add DoubleEndedIterator::nth_back

As suggested by rust-lang#54054. This doesn't fix that issue, as this doesn't add enough implementations to optimise that specific use case, but it adds the method and a few (relatively) trivial overrides to work as an initial implementation.

It's probably going to be a lot of work adding `nth_back` implementations everywhere, and I don't have the time to include it all in this commit. But, it's a start. :)
Centril added a commit to Centril/rust that referenced this issue Dec 22, 2018
Add DoubleEndedIterator::nth_back

As suggested by rust-lang#54054. This doesn't fix that issue, as this doesn't add enough implementations to optimise that specific use case, but it adds the method and a few (relatively) trivial overrides to work as an initial implementation.

It's probably going to be a lot of work adding `nth_back` implementations everywhere, and I don't have the time to include it all in this commit. But, it's a start. :)
kennytm added a commit to kennytm/rust that referenced this issue Dec 22, 2018
Add DoubleEndedIterator::nth_back

As suggested by rust-lang#54054. This doesn't fix that issue, as this doesn't add enough implementations to optimise that specific use case, but it adds the method and a few (relatively) trivial overrides to work as an initial implementation.

It's probably going to be a lot of work adding `nth_back` implementations everywhere, and I don't have the time to include it all in this commit. But, it's a start. :)
@scottmcm scottmcm added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Feb 4, 2019
@scottmcm
Copy link
Member

scottmcm commented Feb 4, 2019

With nth_back landed, this is now a fairly-straightforward task to just override nth_back in double-ended iterators that have nth overridden already.

For example, the slice iterator has

rust/src/libcore/slice/mod.rs

Lines 3802 to 3813 in 8ae730a

#[inline]
fn nth(&mut self, n: usize) -> Option<Self::Item> {
let (end, overflow) = self.size.overflowing_add(n);
if end > self.v.len() || overflow {
self.v = &[];
None
} else {
let nth = &self.v[n..end];
self.v = &self.v[n+1..];
Some(nth)
}
}

So it should probably have nth_back in its DEI implementation a few lines later:

impl<'a, T> DoubleEndedIterator for Windows<'a, T> {

The general pattern here will be to just do whatever nth is doing from the other end; the differences between next and next_back should give you a good hint about what differences are needed.

@koalatux
Copy link
Contributor Author

koalatux commented Feb 5, 2019

Thanks, I will look into implementing some of the nth_back specializations soon. This sounds a promising task to get familiar with libstd.

kennytm added a commit to kennytm/rust that referenced this issue Mar 24, 2019
Implement specialized nth_back() for Box and Windows.

Hi there, this is my first pull request to rust :-)

I started implementing some specializations for DoubleEndedIterator::nth_back() and these are the first two. The problem has been discussed in rust-lang#54054 and nth_back() is tracked in rust-lang#56995.

I'm stuck with the next implementation so I though I do a PR for the ones I'm confident with to get some feedback.
Centril added a commit to Centril/rust that referenced this issue Apr 18, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 18, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 18, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 18, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 18, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 18, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 18, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 19, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 19, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 19, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Apr 19, 2019
implement specialized nth_back() for Bytes, Fuse and Enumerate

Hi,

After my first PR has been successfully merged, here is my second pull request :-)

Also this PR contains some specializations for the problem discussed in rust-lang#54054.
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 1, 2019
bors added a commit that referenced this issue May 13, 2019
Implement nth_back for slice::{Iter, IterMut}

Part of #54054.

I implemented `nth_back` as straightforwardly as I could, and then slightly changed `nth` to match `nth_back`. I believe I did so correctly, but please double-check 🙂

I also added the helper methods `zst_shrink`, `next_unchecked`, and `next_back_unchecked` to get rid of some duplicated code. These changes hopefully make this code easier to understand for new contributors like me.

I noticed the `is_empty!` and `len!` macros which sole purpose seems to be inlining, according to the comment right above them, but the `is_empty` and `len` methods are already marked with `#[inline(always)]`. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.
bors added a commit that referenced this issue May 25, 2019
Implement nth_back for slice::{Iter, IterMut}

Part of #54054.

I implemented `nth_back` as straightforwardly as I could, and then slightly changed `nth` to match `nth_back`. I believe I did so correctly, but please double-check 🙂

I also added the helper methods `zst_shrink`, `next_unchecked`, and `next_back_unchecked` to get rid of some duplicated code. These changes hopefully make this code easier to understand for new contributors like me.

I noticed the `is_empty!` and `len!` macros which sole purpose seems to be inlining, according to the comment right above them, but the `is_empty` and `len` methods are already marked with `#[inline(always)]`. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.
Centril added a commit to Centril/rust that referenced this issue May 29, 2019
…tmcm

Add Step::sub_usize

Required for rust-lang#54054.

I'm aware that the `Step` trait needs a rework, but this still seems like a reasonable addition?

This currently doesn't compile because Chalk contains a type that implement this trait, and this is a breaking change. How can that be fixed?
Centril added a commit to Centril/rust that referenced this issue May 29, 2019
…ottmcm

Implement nth_back for RChunks(Exact)(Mut)

Part of rust-lang#54054.

These implementations may not be optimal because of the use of `self.len()`, but it's quite cheap and simplifies the code a lot.

There's quite some duplication going on here, I wouldn't mind cleaning this up later. A good next step would probably be to add private `split_off_up_to`/`split_off_from` helper methods for slices since their behavior is commonly useful throughout the `Chunks` types.

r? @scottmcm
Centril added a commit to Centril/rust that referenced this issue May 29, 2019
…tmcm

Add Step::sub_usize

Required for rust-lang#54054.

I'm aware that the `Step` trait needs a rework, but this still seems like a reasonable addition?

This currently doesn't compile because Chalk contains a type that implement this trait, and this is a breaking change. How can that be fixed?
Centril added a commit to Centril/rust that referenced this issue May 29, 2019
…ottmcm

Implement nth_back for RChunks(Exact)(Mut)

Part of rust-lang#54054.

These implementations may not be optimal because of the use of `self.len()`, but it's quite cheap and simplifies the code a lot.

There's quite some duplication going on here, I wouldn't mind cleaning this up later. A good next step would probably be to add private `split_off_up_to`/`split_off_from` helper methods for slices since their behavior is commonly useful throughout the `Chunks` types.

r? @scottmcm
@jonas-schievink jonas-schievink added the A-iterators Area: Iterators label Jun 9, 2019
Centril added a commit to Centril/rust that referenced this issue Jun 12, 2019
implement nth_back for Range(Inclusive)

This is part of  rust-lang#54054.
Centril added a commit to Centril/rust that referenced this issue Jun 20, 2019
Add custom nth_back to Skip

Implementation of nth_back for Skip.
Part of rust-lang#54054
Centril added a commit to Centril/rust that referenced this issue Jun 20, 2019
…=scottmcm

Implement nth_back for slice::{Iter, IterMut}

Part of rust-lang#54054.

I implemented `nth_back` as straightforwardly as I could, and then slightly changed `nth` to match `nth_back`. I believe I did so correctly, but please double-check 🙂

I also added the helper methods `zst_shrink`, `next_unchecked`, and `next_back_unchecked` to get rid of some duplicated code. These changes hopefully make this code easier to understand for new contributors like me.

I noticed the `is_empty!` and `len!` macros which sole purpose seems to be inlining, according to the comment right above them, but the `is_empty` and `len` methods are already marked with `#[inline(always)]`. Does that mean we could replace these macros with method calls, without affecting anything? I'd love to get rid of them.
Centril added a commit to Centril/rust that referenced this issue Aug 16, 2019
Add custom nth_back for Chain

Implementation of nth_back for Chain.
Part of rust-lang#54054
Centril added a commit to Centril/rust that referenced this issue Aug 20, 2019
…unksexactmut, r=scottmcm

Implement `nth_back` for ChunksExactMut

This is a part of rust-lang#54054.

r? @scottmcm
Centril added a commit to Centril/rust that referenced this issue Aug 20, 2019
…unksexactmut, r=scottmcm

Implement `nth_back` for ChunksExactMut

This is a part of rust-lang#54054.

r? @scottmcm
Centril added a commit to Centril/rust that referenced this issue Aug 20, 2019
…unksexactmut, r=scottmcm

Implement `nth_back` for ChunksExactMut

This is a part of rust-lang#54054.

r? @scottmcm
Centril added a commit to Centril/rust that referenced this issue Aug 20, 2019
…unksexactmut, r=scottmcm

Implement `nth_back` for ChunksExactMut

This is a part of rust-lang#54054.

r? @scottmcm
@lnicola
Copy link
Member

lnicola commented Aug 31, 2019

Looks like ChunksExactMut can be ticked off (#63265).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants