Insurance policy in case iter.size_hint() lies.

[Centril]

Follow up to https://github.com/rust-lang/rust/pull/64949/files#r334235076.
(If the perf impact is bad we can use debug_assert! instead.)

The good news is that the UI tests pass locally so iter.size_hint() seems to be honest thus far.
On the other hand, with the status quo we do not have an insurance policy should that change in some case. This is problematic because a) this could possibly make some program be accepted which shouldn't, b) the compiler itself could have memory unsafety if the correctness of the iterator is assumed in unsafe { ... } code (even though the blame lies with the unsafe { ... } block in question.)

r? @RalfJung
cc @nnethercote

[Centril]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 256335b with merge bc4dafa90d4ba113c682edfe266ea94a0e2fbc2f...

[bors]

☀️ Try build successful - checks-azure
Build commit: bc4dafa90d4ba113c682edfe266ea94a0e2fbc2f (bc4dafa90d4ba113c682edfe266ea94a0e2fbc2f)

[rust-timer]

Queued bc4dafa90d4ba113c682edfe266ea94a0e2fbc2f with parent 4a8c5b2, future comparison URL.

[RalfJung]

r=me if perf looks good.

[Marwes]

If it matters this could be written without unwrap's or asserts.

let a = match iter.next() {
    None => return Ok(f(&[])),
    Some(a) => a?,
};
let b = match iter.next() {
    None => return Ok(f(&[a])),
    Some(b) => b?,
};
let c = match iter.next() {
    None => return Ok(f(&[a, b])),
    Some(c) => c?,
};
let mut vec: SmallVec<[_; 8]> = SmallVec::with_capacity(iter.size_hint().0 + 3);
vec.push(a);
vec.push(b);
vec.push(c);
for result in iter {
    vec.push(result?);
}
Ok(f(&vec))

[RalfJung]

Nice! OTOH, the original order of the match was chosen by @nnethercote to reflect the frequency of the cases.

[Centril]

@RalfJung Should we perf test this variant?

[RalfJung]

No opinion. @nnethercote what do you think?

[nnethercote]

The vec construction could be more concise, e.g. using extend. And I would use t0, t1, t2 instead of a, b, c. And the comment needs updating. Otherwise, fine by me to use this form if the perf isn't hurt.

[Marwes]

extend would be better but there isn't a way in std to extend from a Result iterator so you need a workaround like https://github.com/mitsuhiko/redis-rs/blob/418512bf9171589b814da4c5ecb500e8747781e0/src/parser.rs#L21-L54

[rust-timer]

Finished benchmarking try commit bc4dafa90d4ba113c682edfe266ea94a0e2fbc2f, comparison URL.

[Centril]

Perf looks like noise / possibly an improvement.

[Centril]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit c85bfc5 with merge 2beb6a3...

[bors]

☀️ Try build successful - checks-azure
Build commit: 2beb6a3 (2beb6a387a95fb690924f39416f3cee585149506)

[rust-timer]

Queued 2beb6a3 with parent 10a52c2, future comparison URL.

[rust-timer]

Finished benchmarking try commit 2beb6a3, comparison URL.

[Centril]

@nnethercote Thoughts on ^---? It looks like a slight regression to me.

[nnethercote]

Let's go with the first one! :)

[mati865]

I think Clippy even has lint against:

for result in iter {
    vec.push(result?);
}

IIRC this performs poorly and could be main reason of this regression.

[Centril]

Dropped the second commits in favor of the first solution which had clean perf; @bors r=RalfJung

[bors]

📌 Commit dfcfca2 has been approved by RalfJung