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

step() slowness #133

Closed
palacaze opened this issue Jul 27, 2016 · 6 comments
Closed

step() slowness #133

palacaze opened this issue Jul 27, 2016 · 6 comments

Comments

@palacaze
Copy link

I encountered a case where the step() method leads to incredibly slow code:

#![feature(step_by)]
extern crate time;
use time::PreciseTime;
extern crate itertools;
use itertools::Itertools;

fn calc2(n: usize) -> Vec<usize> {
    let mut t = vec![0; n] ;
    for i in 2..n {
        if t[i] == 0 {
            t[i] = i - 1;
            for j in (2*i..n).step_by(i) {
                if t[j] == 0 {
                    t[j] = j;
                }
                t[j] = t[j] * (i - 1) / i;
            }
        }
    }
    t
}

fn calc1(n: usize) -> Vec<usize> {
    let mut t = vec![0; n] ;
    for i in 2..n {
        if t[i] == 0 {
            t[i] = i - 1;
            for j in (2*i..n).step(i) {
                if t[j] == 0 {
                    t[j] = j;
                }
                t[j] = t[j] * (i - 1) / i;
            }
        }
    }
    t
}

fn main() {
    let nb = 100_001;
    let start = PreciseTime::now();
    let s = calc2(nb).into_iter().fold(0, |a, c| a + c);
    println!("sum (step_by): {} {:?}", s, start.to(PreciseTime::now()));

    let start = PreciseTime::now();
    let s = calc1(nb).into_iter().fold(0, |a, c| a + c);
    println!("sum (step): {} {:?}", s, start.to(PreciseTime::now()));
}

Replacing step() with the unstable step_by() gives a 100 fold improvement in execution time, as can be observed on the playground here.

@bluss
Copy link
Member

bluss commented Aug 1, 2016

It's not really surprising, .step() is written for general iterators and uses .fuse() among other things (whose performance impediment is well known).

@palacaze
Copy link
Author

palacaze commented Aug 2, 2016

Thanks for the explanation. I see now that step_by() is only implemented for ranges.
Should the performance implications be mentioned in the documentation, as a few iterators, such as interleave(), make use of Fuse internally?

@bluss
Copy link
Member

bluss commented Aug 2, 2016

It's not just fuse, it's a lot of stuff that needs to be covered. Every adaptor could have its own performance story explained.

Step could possibly be improved with specialization (also fuse-related specialization).

@bluss
Copy link
Member

bluss commented Sep 23, 2016

One prerequisite for making the best step iterator for slices (like Stride/StrideMut) is to have arith_offset as a stable function. Then specialization to let us do interesting specializations in itertools itself.

@bluss
Copy link
Member

bluss commented Sep 25, 2017

std has Iterator::step_by now. We can deprecate step() when it's stable.

@bluss
Copy link
Member

bluss commented Nov 24, 2018

Step has been deprecated in itertools 0.8

@bluss bluss closed this as completed Nov 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants