mbutrovich commented on code in PR #11656:
URL: https://github.com/apache/arrow-rs/pull/11656#discussion_r4303920811
##########
arrow-data/src/transform/mod.rs:
##########
@@ -226,13 +226,12 @@ fn build_extend_view(array: &ArrayData, buffer_offset:
u32) -> Extend<'_> {
mutable
.buffer1
.extend(views[start..start + len].iter().map(|v| {
- let len = *v as u32;
- if len <= 12 {
- return *v; // Stored inline
- }
+ // A view of 12 bytes or fewer stores the string inline and
+ // has no buffer index, so adding 0 leaves it unchanged.
+ let offset = if *v as u32 > 12 { buffer_offset } else { 0
};
Review Comment:
Since this line is being rewritten anyway, what do you think about using
[`MAX_INLINE_VIEW_LEN`](https://github.com/apache/arrow-rs/blob/e2eecf12101913f94f1ec573951f4b5bdc645c47/arrow-data/src/byte_view.rs#L27)
instead of the literal `12`? That's what [`ByteView`'s own inline
check](https://github.com/apache/arrow-rs/blob/e2eecf12101913f94f1ec573951f4b5bdc645c47/arrow-data/src/byte_view.rs#L170)
uses. It also needs `MAX_INLINE_VIEW_LEN` added to the `use super::{...}`
import at the top of the file.
```suggestion
let offset = if *v as u32 > MAX_INLINE_VIEW_LEN {
buffer_offset
} else {
0
};
```
##########
arrow-data/src/transform/mod.rs:
##########
@@ -226,13 +226,12 @@ fn build_extend_view(array: &ArrayData, buffer_offset:
u32) -> Extend<'_> {
mutable
.buffer1
.extend(views[start..start + len].iter().map(|v| {
Review Comment:
Do you know where the extra time goes in `mutable str_view 1024` on x86? The
description shows 318.2 us to 356.8 us (+12.1%) there, while the other x86
`mutable_array` cases improve. On an M5 Max I see the same benchmark improve by
3.6% (168.2 us to 162.1 us). This is the long-slice case with strings of 0 to
399 bytes. Few of those strings are inline, so the old branch was well
predicted. If the cost comes from the branchless select, it would help to know
before trading it for the gains in the short-slice cases.
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -995,11 +995,20 @@ impl MutableBuffer {
mut iterator: I,
) {
let item_size = std::mem::size_of::<T>();
- let (lower, _) = iterator.size_hint();
- let additional = lower * item_size;
- self.reserve(additional);
+ let (lower, upper) = iterator.size_hint();
+ if let Some(byte_count) = lower.checked_mul(item_size) {
+ self.reserve(byte_count);
+ if upper == Some(lower) {
Review Comment:
Should `extend` keep appending when an iterator yields more items than its
exact size hint claims? As I read the new path, those items are dropped without
an error, and `mutable_extend_from_iter_wrong_exact_size_hint` checks for that
(32 items in, `[0, 1]` out). Before this PR, every item was appended.
The `Iterator::size_hint`
[docs](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint)
allow this: "A buggy iterator may yield less than the lower bound or more than
the upper bound of elements" and "An incorrect implementation of `size_hint()`
should not lead to memory safety violations." So it's sound. But it is a change
users can observe through the public
[`Extend`](https://github.com/apache/arrow-rs/blob/e2eecf12101913f94f1ec573951f4b5bdc645c47/arrow-buffer/src/buffer/mutable.rs#L962-L968)
and
[`FromIterator`](https://github.com/apache/arrow-rs/blob/e2eecf12101913f94f1ec573951f4b5bdc645c47/arrow-buffer/src/buffer/mutable.rs#L1343-L1349)
impls on `MutableBuffer`. It also makes them differ from
[`Buffer::from_iter`](https://github.com/apache/arrow-rs/blob/e2eecf12101913f94f1ec573951f4b5bdc645c47/arrow-buffer/src/buffer/immutable.rs#L641-L645),
which collects through a `Vec` and keeps every item.
I tried the obvious alternative: zip over `iterator.by_ref()` in
`extend_from_exact_size_iter`, then `drop(len)` and `iterator.for_each(|item|
self.push(item))`. It's correct for any hint, but on my machine it gives back
about half of the gain on short chunks:
| case (avg 16 items per `extend`) | base | PR | append remainder |
|---|---|---|---|
| map i32 | 12.44 us | 7.36 us | 9.66 us |
| copied u8 | 12.66 us | 7.39 us | 10.71 us |
| view offset closure | 15.11 us | 10.78 us | 13.76 us |
The cost seems to come from losing the `Zip` fast path for `&mut I`: the
vector loop stays, but it gains a runtime alias check and a scalar tail with
two exits. So I think keeping truncation is reasonable. If we do keep it, what
do you think about documenting it on the `Extend` and `FromIterator` impls, and
mentioning it under user-facing changes in the description?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
