Optimize implementations of FromIterator and Extend for Vec

[mzabaluev]

Instead of a fast branch with a sized iterator falling back to a potentially poorly optimized iterate-and-push loop, a single efficient loop can serve all cases.

In my benchmark runs, I see some good gains, but also some regressions, possibly due to different inlining choices by the compiler. YMMV.

[rust-highfive]

r? @Gankro

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

[Gankro]

Can you post your experimental results.

[mahkoh]

Does it optimize to a memcpy if the src is contiguous? If not then it will have to be changed anyway.

[pczarn]

Can you use while let here?

[mzabaluev]

I can, good point.

[mzabaluev]

Results of benchmarking on i686: https://gist.github.com/mzabaluev/df41c2f50464416b0a26

tl;dr: Notable losers are (observed consistently over multiple bench runs):

Test	Percentage
bit::bit_vec_bench::bench_bit_vec_big_iter	189%
slice::bench::sort_big_random_small	143%
slice::bench::zero_1kb_from_elem	184%
vec::tests::bench_extend_0000_1000	119%
vec::tests::bench_from_elem_0100	129%
vec::tests::bench_from_iter_0010	121%
vec::tests::bench_from_iter_0100	125%

Winners:

Test	Percentage
string::tests::bench_push_char_two_bytes	95%
string::tests::from_utf8_lossy_100_invalid	38%
vec::tests::bench_extend_0000_0010	87%
vec::tests::bench_extend_0000_0100	89%
vec::tests::bench_from_elem_1000	48%
vec::tests::bench_from_iter_1000	81%

Overall, I don't think too much trust should be put in microbenchmarks repeatedly testing one call-site optimized function.

[huonw]

How much is "winning" and "losing'"?

[mzabaluev]

@mahkoh These changes should not drastically change the performance with slice iterators, in which case it's as close to reserve + type-aligned memcpy as the optimizer can make it. The main purpose is to provide comparable efficiency for size-unaware or pessimistic iterators.

[mzabaluev]

@huonw I've added percentages to the comment above.

[pczarn]

Two of these methods have #[inline], but extend_desugared does not; are benchmarks affected? Is there some benefit to having extend_desugared vs calling vector.extend(iterator)? Does extend_desugared change inlining choices, too?

[mzabaluev]

I actually ran benchmarks once with #[inline] on extend_desugared, but the results did not look more different than some jitter. I don't think #[inline] has any effect on generic methods, as they are always available in metadata, so the compiler can always choose to inline them across crates. Even less so for unit tests.

[mzabaluev]

There should be more benchmarks with extending from sizeless/pessimistic iterators. Any good candidates in libcollections?

[Gankro]

BTreeMap's range iter, VecMap's iter, and bitvset's iter have no idea how many elements they contain, but they're all also doing a non-trivial amount of work to actually yield those elements.

[mzabaluev]

@pczarn Without looking too closely into the results (I get easily frustrated with the long build times of the library crates and their tests), I assume the optimizer has more "reason" to share extend_desugared if it is used in various places, unless the tradeoff between cache/branch predictor efficiency and removing the overhead of calling leans towards the latter.

[steveklabnik]

How do we all feel about this PR today?

[Gankro]

Oh geez, this slipped right through the cracks! I don't have a great gut on this since it seems to just be shuffling perf around. r? @huonw

[huonw]

Hm, does this actually improve performance, over the simpler:

let mut vector = Vec::with_capacity(iterator.size_hint().0);
vector.extend_desugared(iterator);
vector

[mzabaluev]

This avoids a branch that is present in extend_desugared. That branch tends to be not taken, but in the first iteration the vector is always expanded.

[huonw]

Hm, why it does it avoid that branch? It seems that if the iterator has a non-zero size hint the with_capacity will ensure that it isn't taken, and in the case that the iterator has a zero-size hint it seems both with and without this are equally bad off?

Basically, I'm asking if this was noticeable in practice.

[mzabaluev]

You are right, and it was somehow lost on me that Vec::with_capacity(1) allocates exactly one element. I should simplify the code to your suggestion; the performance difference will likely be negligible.

[mzabaluev]

With the suggested change, vec::bench_from_iter_0000 regresses
from 29 ns/iter (+/- 2) to 87 ns/iter (+/- 48) in my testing
(rebased against commit 3dbfa74), the other benchmarks seemingly unaffected. I wonder if it can be considered an edge case.

[Gankro]

@huonw @mzabaluev What's up with this?

[Gankro]

Closing due to inactivity.

[mzabaluev]

@Gankro Ah sorry, I let your request slip by. I believe the branch is good as it is. @huonw's suggestion, while good from code clarity point of view, resulted in some performance degradation as mentioned in the discussion above.

[Gankro]

@huonw Any reason not to merge this?

[Gankro]

huon's busy

[Gankro]

Calling size_hint in a loop seems really bad. This is not necessarily a straight-forward or cheap method. Is hoisting it out not worth it in your testing?

[huonw]

It is called exponentially rarely, so it seems fine?

[Gankro]

Oh derp, sure.

[Gankro]

At worst I like this better than the old code. r+

[huonw]

@bors r+

[bors]

📌 Commit 7b464d3 has been approved by huonw

[bors]

⌛️ Testing commit 7b464d3 with merge 0250ff9...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt