ARROW-11037: [Rust] Optimized creation of string array from iterator.

[jorgecarleitao]

Avoids a memcopy from 2 Vec<T> to Buffer, by building the buffers on the fly.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-11037

[codecov-io]

Codecov Report

Merging #9016 (6e8d2ff) into master (86cf246) will decrease coverage by 0.00%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #9016      +/-   ##
==========================================
- Coverage   82.60%   82.60%   -0.01%     
==========================================
  Files         204      204              
  Lines       50175    50176       +1     
==========================================
- Hits        41447    41446       -1     
- Misses       8728     8730       +2     

Impacted Files	Coverage Δ
rust/arrow/src/array/array_string.rs	88.88% <87.50%> (-1.06%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86cf246...efb7883. Read the comment docs.

[Dandandan]

I think the previous time I tried this in a kernel, I got worse perf results than copying them from the Vecs, but maybe the buffer implementation is faster now? Does this code have a benchmark?

[jorgecarleitao]

That is a good point. I assumed that such a memcopy would always be worse for sufficiently large arrays, but I have not validated this.

[jorgecarleitao]

I am moving this to draft until the benches are in place. @Dandandan is right that this is not necessarily an improvement. I am investigating this.\

[Dandandan]

Might be interesting to test here whether it is cheaper to calculate offsets + total size first for values buffer based on final length_so_far so that it doesn't require extra allocations.

[Dandandan]

So something like

for s in &v {
            length_so_far = length_so_far + OffsetSize::from_usize(s.len()).unwrap();
            offsets.extend_from_slice(length_so_far.to_byte_slice());
        }
        
let mut values = MutableBuffer::new(length_so_far);
for s in &v {
    values.extend_from_slice(s.as_bytes());
}

[Dandandan]

Probably the offset buffer is also faster after converting it to a slice using typed_data_mut::<OffsetSize>() and iterating it with iter_mut, this way it can skip the capacity checks, and also current conversion to a byte slice may add some overhead?

[jorgecarleitao]

Both great ideas. If it is ok for you, I will leave them to future work, as I am focusing my work on the MutableBuffer atm.

[jorgecarleitao]

I.e. the general direction that I am pushing towards is to stop using Vec and replace it by MutableBuffer to avoid extra allocations. Once that is in place, we can replace these by iter_mut.

also current conversion to a byte slice may add some overhead?

I believe so, as to_byte_slice returns a slice of unknown size, both for &T and &[T]. Either the compiler optimizes it, or there is an extra cost. I benched a 10% diff in building buffers when introduced a method that was not doing bound checks on these. IMO we should be using to_le_bytes, and have a method MutableBuffer::push<ToByteSlice>. I think we need the crate byteorder because AFAIK std's to_le_bytes does not have a trait (which we need).

[jorgecarleitao]

I have re-opened this PR as #9032 shows that creating a buffer from a mutable buffer is 2x faster.

[alamb]

The full set of Rust CI tests did not run on this PR :(

Can you please rebase this PR against apache/master to pick up the changes in #9056 so that they do?

I apologize for the inconvenience.

[alamb]

Looks good to me. Thanks @jorgecarleitao -- I didn't test this PR out or run the benchmarks but I think your results are good for me;

I did read the code and I understand the changes. 👍