Skip to content

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Mar 1, 2018

Fixes the performance difference observed in https://users.rust-lang.org/t/performance-difference-between-iterator-zip-and-skip-order/15743

Before:

test iter::bench_skip_then_zip                     ... bench:         154 ns/iter (+/- 4)
test iter::bench_zip_then_skip                     ... bench:       4,725 ns/iter (+/- 225)

After:

test iter::bench_skip_then_zip                     ... bench:         154 ns/iter (+/- 2)
test iter::bench_zip_then_skip                     ... bench:          81 ns/iter (+/- 19)

Makes the bench asked about on URLO 58x faster :)
@rust-highfive
Copy link
Contributor

r? @kennytm

(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 Mar 1, 2018
@kennytm
Copy link
Member

kennytm commented Mar 1, 2018

Could you add a test to ensure side effects are executed?

fn main() {
    let mut a = Vec::new();
    let mut b = Vec::new();
    let value = (1..7)
        .map(|n| {
            a.push(n);
            n * 10
        })
        .zip((2..9).map(|n| {
            b.push(n * 100);
            n * 1000
        }))
        .skip(1)
        .nth(3);
    assert_eq!(value, Some((50, 6000)));
    assert_eq!(a, vec![1, 2, 3, 4, 5]);
    assert_eq!(b, vec![200, 300, 400, 500, 600]);
}

@scottmcm
Copy link
Member Author

scottmcm commented Mar 1, 2018

@kennytm Good call.

It looks like Range: !TrustedRandomAccess, though, so I swapped it out for cloning slice iterators.

@@ -1094,6 +1107,12 @@ impl<A, B> ZipImpl<A, B> for Zip<A, B>
})
}

#[inline]
default fn nth(&mut self, n: usize) -> Option<Self::Item>
{
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: why this { is on a new line 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that is an excellent question 😅

@@ -1174,6 +1193,25 @@ impl<A, B> ZipImpl<A, B> for Zip<A, B>
(len, Some(len))
}

#[inline]
fn nth(&mut self, n: usize) -> Option<Self::Item>
{
Copy link
Member

Choose a reason for hiding this comment

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

(This also)

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 1, 2018
@kennytm
Copy link
Member

kennytm commented Mar 1, 2018

Travis passed, so

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 1, 2018

📌 Commit 5105fc1 has been approved by kennytm

@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 Mar 1, 2018
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 3, 2018
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 3, 2018
Specialize Zip::nth for TrustedRandomAccess

Fixes the performance difference observed in https://users.rust-lang.org/t/performance-difference-between-iterator-zip-and-skip-order/15743

Before:
```
test iter::bench_skip_then_zip                     ... bench:         154 ns/iter (+/- 4)
test iter::bench_zip_then_skip                     ... bench:       4,725 ns/iter (+/- 225)
```

After:
```
test iter::bench_skip_then_zip                     ... bench:         154 ns/iter (+/- 2)
test iter::bench_zip_then_skip                     ... bench:          81 ns/iter (+/- 19)
```
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 3, 2018
@bors bors merged commit 5105fc1 into rust-lang:master Mar 3, 2018
@scottmcm scottmcm deleted the faster-zip-nth branch March 4, 2018 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

4 participants