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

Specialize methods on iter::Cloned<I> where I::Item: Copy. #36791

Closed
wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Contributor

A common idiom when iterating over a collection of, e.g., &strs is to write my_collection.iter().cloned().something_else(). Unfortunately, because the call to clone might do something interesting, we can't specialize count, last, and nth as discussed in #28125. However, now that rust-lang/rfcs#1521 has been merged and we have specialization, we can specialize count, last, and nth for iter::Cloned<I> where I::Item: Copy (this also optimizes skip). Benchmarks below:

extern crate test;
use test::Bencher;

pub struct Cloned<I> {
    it: I,
}

impl<'a, I, T: 'a> Iterator for Cloned<I>
    where I: Iterator<Item=&'a T>, T: Clone
{
    type Item = T;

    fn next(&mut self) -> Option<T> {
        self.it.next().cloned()
    }

    fn size_hint(&self) -> (usize, Option<usize>) {
        self.it.size_hint()
    }
}

impl<'a, I, T: 'a> Iterator for Cloned<I>
    where I: Iterator<Item=&'a T>, T: Copy
{
    fn nth(&mut self, n: usize) -> Option<T> {
        self.it.nth(n).cloned()
    }

    fn last(self) -> Option<T> {
        self.it.last().cloned()
    }

    fn count(self) -> usize {
        self.it.count()
    }
}

fn bench_cloned(b: &mut Bencher) {
    let numbers: Vec<_> = vec!["asfd"; 100];
    b.iter(|| {
        test::black_box(test::black_box(numbers.iter()).cloned().last())
    })
}

fn bench_my_cloned(b: &mut Bencher) {
    let numbers: Vec<_> = vec!["asfd"; 100];
    b.iter(|| {
        test::black_box(Cloned { it: test::black_box(numbers.iter()) }.last())
    })
}


fn bench_normal(b: &mut Bencher) {
    let numbers: Vec<_> = vec!["asfd"; 100];
    b.iter(|| {
        test::black_box(test::black_box(numbers.iter()).last())
    })
}


fn bench_cloned_long(b: &mut Bencher) {
    let numbers: Vec<_> = vec!["asfd"; 1000];
    b.iter(|| {
        test::black_box(test::black_box(numbers.iter()).cloned().last())
    })
}

fn bench_my_cloned_long(b: &mut Bencher) {
    let numbers: Vec<_> = vec!["asfd"; 1000];
    b.iter(|| {
        test::black_box(Cloned { it: test::black_box(numbers.iter()) }.last())
    })
}


fn bench_normal_long(b: &mut Bencher) {
    let numbers: Vec<_> = vec!["asfd"; 1000];
    b.iter(|| {
        test::black_box(test::black_box(numbers.iter()).last())
    })
}

Results:

test bench_cloned         ... bench:          85 ns/iter (+/- 2)
test bench_cloned_long    ... bench:         774 ns/iter (+/- 20)
test bench_my_cloned      ... bench:           2 ns/iter (+/- 0)
test bench_my_cloned_long ... bench:           2 ns/iter (+/- 0)
test bench_normal         ... bench:           1 ns/iter (+/- 0)
test bench_normal_long    ... bench:           1 ns/iter (+/- 0)

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@Stebalien
Copy link
Contributor Author

Please wait for travis to pass before bringing in bors. I'm at a coffee shop with a slow internet connection and an out-of-date rust snapshot so I can't so much as run make tidy at the moment.

@Stebalien
Copy link
Contributor Author

In the future, it might be nice to introduce an auto-derived ClonePure marker trait for side-effect free clones and specialize using that instead.

@Stebalien
Copy link
Contributor Author

Tripping over #36053. We may have to wait a cycle (or backport to beta).

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 28, 2016
@alexcrichton
Copy link
Member

cc @rust-lang/libs, @bluss

@bluss
Copy link
Member

bluss commented Sep 28, 2016

Then it can implement TrustedRandomAccess too, which is great (.zip() specialization).

@Stebalien
Copy link
Contributor Author

Stebalien commented Sep 28, 2016

Also, the cloned adapter (well, adapters in general) appear to be inhibiting some optimizations. Benchmarks:

#![feature(test)]
extern crate test;
use test::Bencher;

#[bench]
fn bench_cloned(b: &mut Bencher) {
    let strings: Vec<_> = vec!["asfd"; 1000];
    b.iter(|| {
        for i in test::black_box(strings.iter()).cloned() {
            test::black_box(i);
        }
    })
}


#[bench]
fn bench_map(b: &mut Bencher) {
    let strings: Vec<_> = vec!["asfd"; 1000];
    b.iter(|| {
        for i in test::black_box(strings.iter()).map(|&x|x) {
            test::black_box(i);
        }
    })
}

#[bench]
fn bench_pat(b: &mut Bencher) {
    let strings: Vec<_> = vec!["asfd"; 1000];
    b.iter(|| {
        for &i in test::black_box(strings.iter()) {
            test::black_box(i);
        }
    })
}
test bench_cloned ... bench:         858 ns/iter (+/- 14)
test bench_map    ... bench:         856 ns/iter (+/- 8)
test bench_pat    ... bench:         347 ns/iter (+/- 17)

Edit: Actually, I think the last case is just optimizing the deref somehow. Never mind.

#[bench]
fn bench_deref(b: &mut Bencher) {
    let strings: Vec<_> = vec!["asfd"; 1000];
    b.iter(|| {
        for i in test::black_box(strings.iter()) {
            test::black_box(*test::black_box(i));
        }
    })
}
test bench_cloned ... bench:         810 ns/iter (+/- 10)
test bench_deref  ... bench:         816 ns/iter (+/- 18)
test bench_map    ... bench:         806 ns/iter (+/- 22)
test bench_pat    ... bench:         354 ns/iter (+/- 35)

@bluss
Copy link
Member

bluss commented Sep 28, 2016

@Stebalien It's pretty hard to say if those results are realistic, or artifacts of how black box interacts with the optimization of the loop.

I'll put in my own straw men: integer sum/autovectorization and linear search and here all the three variants are the same. https://gist.github.com/bluss/3f5ded1cdd7304e1abc7dc586ed1c1fa

Instead of cloning a bunch of copyable types only to drop them (in `nth`,
`last`, and `count`), take advantage of rust-lang#1521 (Copy clone semantics) and don't
bother cloning them in the first place (directly call `nth`, `last`, and `count`
on the wrapped iterator). If the wrapped iterator optimizes these methods,
`Cloned` now inherits this optimization.
@alexcrichton
Copy link
Member

@Stebalien if this is rebased, perhaps it's got a green travis now?

@Stebalien
Copy link
Contributor Author

Not unless #36848 has been fixed.

@alexcrichton
Copy link
Member

Ah ok, in that case do we want to close this?

@Stebalien
Copy link
Contributor Author

Might as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants