Preallocate the vector containing predicates in decode_predicates

[ljedrz]

decode_predicates is marked as #[inline], so this extra bit of performance could be worthwhile. As an added bonus this makes the returned object more readable.

[rust-highfive]

r? @eddyb

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

[eddyb]

Does this actually change anything? I thought using the range with collect should preallocate all the same, even with the map there.

r? @nikomatsakis

[ljedrz]

@eddyb the original version collects into a Result, so the final capacity is unknown (because it can result in an error). This change expects an Ok Result and allocates for the full vector.

[ljedrz]

The difference in performance can be quite significant: example.

[eddyb]

@ljedrz I frankly would've expected the full capacity to be allocated even for Result.

@gankro @alexcrichton @bluss What's happening here, why are things the way they are?

[ljedrz]

Threre is a related issue (#48994), but the attempts to crack it have been unsuccessful so far.

[eddyb]

I think #52910 shouldn't have been closed, just redone using specialization.

[ljedrz]

@eddyb I'm not too savvy with specializations, but I took a stab at it; would it look something like this?

liballoc/vec.rs

trait SpecResult<I, T, E> where I: Iterator<Item = Result<T, E>> {
    fn from_iter(iter: I) -> Result<Vec<T>, E>;
}

impl<I, T, E> SpecResult<I, T, E> for Vec<Result<T, E>>
    where I: Iterator<Item = Result<T, E>>
{
    default fn from_iter(mut iterator: I) -> Result<Vec<T>, E> {
        let (_low, high) = iterator.size_hint(); // lower bound is 0
        let mut vector: Vec<T> = if let Some(high) = high {
            Vec::with_capacity(high) // optimize for the common case (all elements are Ok)
        } else {
            Vec::new()
        };
        let mut len = 0;

        while let Some(element) = iterator.next() {
            match element {
                Ok(element) => {
                    vector.reserve(1);
                    unsafe {
                        ptr::write(vector.get_unchecked_mut(len), element);
                        // NB can't overflow since we would have had to alloc the address space
                        vector.set_len(len + 1);
                    }
                    len += 1;
                },
                Err(err) => return Err(err)
            }
        }
        Ok(vector)
    }
}

[eddyb]

@ljedrz Maybe, but I think you need the more general implementation, and that would have default fn, but the specialization for Vec wouldn't.
Also, you only really need to specialize the initial capacity, not the rest of the code.
And it should involve / be specialized for the iterator adapter that handles collecting to Result, not any arbitrary Vec. And I don't think you should use the upper bound at all.

I think you need to talk to someone from @rust-lang/libs - which is why discussion should've continued on #52910.

[nikomatsakis]

Thoughts?

[nikomatsakis]

Unless I'm mistaken, the iterator will already pre-allocate a vector of the right size here, so this doesn't really buy us anything.

[ljedrz]

The lower bound of size_hint for the implementation of FromIterator for Result is equal to zero and the implementation of from_iter for Vec uses it to calculate the capacity. I don't think this is a case of TrustedLen (due to non-conforming size_hint), otherwise this PR (where the number of elements iterated over is known) wouldn't benefit from such a change either.

[nikomatsakis]

Ah, I see, ok.

[nikomatsakis]

I also see I missed a lot of discussion on this topic already 😊

[nikomatsakis]

Do we care enough to measure this? I'm sort of inclined to just r+, but I wonder what @eddyb thinks?

Do we have enough information to compute the "max" length? Maybe we could have a variant of collect for use in the compiler that tries to pre-allocate when the "max length" is reasonable?

[ljedrz]

I think @eddyb's idea regarding specialization is a good general approach to solving this (optimizing for the probably most common all-Ok cases), but I don't know how it should be implemented 🤔. In this particular case the maximum length is available.

[eddyb]

@nikomatsakis I think we should try to figure out how to use specialization to pass the right length from "collect into a Result" to "collect into a Vec", if possible.
But we need someone who has been doing a lot of specialization work around iterators.

[nikomatsakis]

In that case, should we close this PR? Land it in the meantime? Do we have any perf measurements?

[eddyb]

@nikomatsakis Close this, reopen #52910 (which had already been FCP-approved before considered to break a "law" of Iterator::size_hint), and continue the discussion about specialization there.