libcore::iter Refactor Iterator::next() for Skip

[syml]

So I simplified the code, taking into account the points raised in the discussion.

I added some benchmarks, and verified that the performance improved. It's about twice as fast on the benchmarks.

Close reason (last comment)

Hmm now that I understand more the code I think I need more benchmarks before changing that part. I need to test with big iterators and big skip ranges. I will close that request for now and think more. I will come back with more detailed benchmarks at least, even if I am not changing the code.

[alexcrichton]

Iterators are generally pretty performance sensitive, could you verify that a benchmark for skip is the same both before and after?

[syml]

I wrote some benchmarks
The results are similar before and after my change

➜ rust_test ./skip_bench_before --bench
running 3 tests
test bench_skip ... bench: 116 ns/iter (+/- 16)
test bench_skip_reach_end ... bench: 210 ns/iter (+/- 20)
test bench_skip_zero ... bench: 58 ns/iter (+/- 2)
test result: ok. 0 passed; 0 failed; 0 ignored; 3 measured

➜ rust_test ./skip_bench_after --bench
running 3 tests
test bench_skip ... bench: 115 ns/iter (+/- 4)
test bench_skip_reach_end ... bench: 210 ns/iter (+/- 17)
test bench_skip_zero ... bench: 58 ns/iter (+/- 2)
test result: ok. 0 passed; 0 failed; 0 ignored; 3 measured

Here is the code

extern crate test;
use test::Bencher;

#[bench]
fn bench_skip(b: &mut Bencher) {
    let xs = [0u, 1, 2, 3, 5, 13, 15, 16, 17, 19, 20, 30];
    b.iter(|| xs.iter().skip(5).next());
}

#[bench]
fn bench_skip_zero(b: &mut Bencher) {
    let xs = [0u, 1, 2, 3, 5, 13, 15, 16, 17, 19, 20, 30];
    b.iter(|| xs.iter().skip(0).next());
}

#[bench]
fn bench_skip_reach_end(b: &mut Bencher) {
    let xs = [0u, 1, 2, 3, 5, 13, 15, 16, 17, 19, 20, 30];
    b.iter(|| xs.iter().skip(20).next());
}

[pczarn]

how about mem::replace(&mut self.n, 0)?

[pczarn]

You may add these benchmarks to libcoretest/iter.rs.

[syml]

Just adding that benchmark code at the end of the file or is there some way of presenting it ?

[pczarn]

Just at the end, like it is in mem.rs.

[syml]

It's like 10ns per iter slower with your changes

test bench_skip ... bench: 127 ns/iter (+/- 6)
test bench_skip_reach_end ... bench: 202 ns/iter (+/- 11)
test bench_skip_zero ... bench: 68 ns/iter (+/- 3)

I don't know if these are worth the more complex code. What is weird is that with the -O flag the new code is 10ns faster
Before

test bench_skip ... bench: 9 ns/iter (+/- 0)
test bench_skip_reach_end ... bench: 21 ns/iter (+/- 2)
test bench_skip_zero ... bench: 0 ns/iter (+/- 0)

After

test bench_skip ... bench: 3 ns/iter (+/- 2)
test bench_skip_reach_end ... bench: 11 ns/iter (+/- 0)
test bench_skip_zero ... bench: 0 ns/iter (+/- 0)

I am not expert on what these benchmarks actually mean, with or without optimizations etc.
I would be in favor of the simpler code though. And I can still try without the mem::replace since I think it's the part that adds steps in the benchmark without optimizations.

[pczarn]

Yes, the path that eventually returns Some is slower by 10ns.

-O is the most common option when the run-time performance matters. The code that runs the fastest with -O is the best choice, in my opinion. With mem::replace or not, of course.

I think self.n = 0; can go before return Some so that mem::replace is not necessary.

[syml]

I am not sure that mem::replace is an issue, since the path that returns None is actually faster than before. And as you said with the optimization flag the whole code is faster than before. I will still try without mem::replace to see if it improves.

[pczarn]

Perhaps returning Some is slower in unoptimized code because the for loop first moves the value out of Option<T> and then wraps it back.

[syml]

Ok it's even faster without mem::replace, I replaced it as you suggested.

test bench_skip ... bench: 2 ns/iter (+/- 0)
test bench_skip_reach_end ... bench: 8 ns/iter (+/- 0)
test bench_skip_zero ... bench: 0 ns/iter (+/- 0)

Looks like we can have shorter and faster code at the end : )

[pczarn]

The assignment you removed either doesn't matter or it should set self.n = n instead, I suppose. I'm not sure how the underlying iterator will behave once it's exhausted. Not all iterators are Fused.

[thestinger]

An iterator adaptor doesn't have to worry about the behaviour after it's exhausted once, as long as it's not memory unsafe.

[syml]

If you look at the original code it sets self.n = 0 in case of exhaustion. Actually all pathes set self.n = 0.
Now regarding whether it's correct/useful or not to do that I am not sure. I didn't look at all the use cases of the Skip object.

[thestinger]

It doesn't have to do that, any code depending on it is buggy.

[syml]

@thestinger If I understand well what you say self.n should be set to n in case the iterator is exhausted ? Or do you mean that we shouldn't even care of updating self.n ?

[bluss]

Fyi, only optimized benchmark runs matter at all. Comparing unoptimized results is next to meaningless in Rust.

[syml]

Hmm now that I understand more the code I think I need more benchmarks before changing that part. I need to test with big iterators and big skip ranges. I will close that request for now and think more. I will come back with more detailed benchmarks at least, even if I am not changing the code.

[pczarn]

We don't care at all of self.n in case the iterator is exhausted. The code seems entirely correct to me.

The benchmarks should cover chained iterators: .skip(x).skip(y).skip(z), .skip(n).cycle().take(n).advance(|| true) etc.