Skip to content
129 changes: 123 additions & 6 deletions arrow-buffer/src/buffer/mutable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,10 @@ pub(crate) fn dangling_ptr() -> NonNull<u8> {
}

impl<A: ArrowNativeType> Extend<A> for MutableBuffer {
/// Appends the items of `iter`.
///
/// If `iter`'s [`size_hint`](Iterator::size_hint) reports an exact length,
/// at most that many items are appended.
#[inline]
fn extend<T: IntoIterator<Item = A>>(&mut self, iter: T) {
let iterator = iter.into_iter();
Expand Down Expand Up @@ -995,11 +999,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) {
Comment thread
neilconway marked this conversation as resolved.
// SAFETY: `reserve` just made room for `lower` items
return unsafe { self.extend_from_exact_size_iter(iterator, lower) };
}
}

// this is necessary because of https://github.com/rust-lang/rust/issues/32155
// Keep the length in a local and write it back when `len` is dropped.
// If the loop updated `self.len` directly, the compiler couldn't rule
// out that the writes through `dst` change it, so it would load and
// store the length for every item. `Vec` works around the same problem
// (https://github.com/rust-lang/rust/issues/32155).
let mut len = SetLenOnDrop::new(&mut self.len);
let mut dst = unsafe { self.data.as_ptr().add(len.local_len) };
let capacity = self.layout.size();
Expand All @@ -1021,8 +1034,48 @@ impl MutableBuffer {
iterator.for_each(|item| self.push(item));
}

/// Appends the items of `iterator`, whose size hint says it yields exactly
/// `count` items.
///
/// The loop doesn't check the capacity before each item. If `iterator`
/// yields more than `count` items, the extra items are not appended.
///
/// # Safety
///
/// The buffer must have room for `count` more items.
#[inline]
unsafe fn extend_from_exact_size_iter<T: ArrowNativeType, I: Iterator<Item = T>>(
&mut self,
iterator: I,
count: usize,
) {
debug_assert!(
count
.checked_mul(std::mem::size_of::<T>())
.is_some_and(|bytes| bytes <= self.capacity() - self.len),
"buffer has no room for {count} more items"
);

// Keep the length in a local and write it back when `len` is dropped.
// If the loop updated `self.len` directly, the compiler couldn't rule
// out that the writes through `dst` change it, so it would load and
// store the length for every item. `Vec` works around the same problem
// (https://github.com/rust-lang/rust/issues/32155).
let mut len = SetLenOnDrop::new(&mut self.len);
let start = len.local_len;
// SAFETY: `start` is at most the capacity. It need not be a multiple of
// `T`'s alignment, so `dst` is only written unaligned.
let dst = unsafe { self.data.as_ptr().add(start) }.cast::<T>();
for (i, item) in (0..count).zip(iterator) {
// SAFETY: `i < count`, and the caller guarantees room for `count`
// items after the buffer's length.
unsafe { dst.add(i).write_unaligned(item) };
len.local_len = start + (i + 1) * std::mem::size_of::<T>();
}
}

/// Creates a [`MutableBuffer`] from an [`Iterator`] with a trusted (upper) length.
/// Prefer this to `collect` whenever possible, as it is faster ~60% faster.
/// Prefer this to `collect` whenever possible, as it is ~60% faster.
/// # Example
/// ```
/// # use arrow_buffer::buffer::MutableBuffer;
Expand Down Expand Up @@ -1105,7 +1158,7 @@ impl MutableBuffer {

/// Creates a [`MutableBuffer`] from an [`Iterator`] with a trusted (upper) length or errors
/// if any of the items of the iterator is an error.
/// Prefer this to `collect` whenever possible, as it is faster ~60% faster.
/// Prefer this to `collect` whenever possible, as it is ~60% faster.
///
/// # Errors
///
Expand Down Expand Up @@ -1292,6 +1345,10 @@ impl std::iter::FromIterator<bool> for MutableBuffer {
}

impl<T: ArrowNativeType> std::iter::FromIterator<T> for MutableBuffer {
/// Collects the items of `iter`.
///
/// If `iter`'s [`size_hint`](Iterator::size_hint) reports an exact length,
/// at most that many items are collected.
fn from_iter<I: IntoIterator<Item = T>>(iter: I) -> Self {
let mut buffer = Self::default();
buffer.extend_from_iter(iter.into_iter());
Expand Down Expand Up @@ -1366,6 +1423,66 @@ mod tests {
assert_eq!(&[1u8, 1u8, 0, 0, 0, 0, 0, 0, 0], buf.as_slice());
}

/// Yields `items`, but its size hint always claims exactly `claimed` items.
struct WrongSizeHint<'a> {
items: std::slice::Iter<'a, u32>,
claimed: usize,
}

impl Iterator for WrongSizeHint<'_> {
type Item = u32;

fn next(&mut self) -> Option<u32> {
self.items.next().copied()
}

fn size_hint(&self) -> (usize, Option<usize>) {
(self.claimed, Some(self.claimed))
}
}

/// `extend` is safe, so an exact size hint that is wrong must not make it
/// write past the reserved space or claim bytes it never wrote. Miri
/// catches both when it runs this test.
#[test]
fn mutable_extend_from_iter_wrong_exact_size_hint() {
// Fewer items than claimed: the length covers only the items written
let mut buf = MutableBuffer::new(0);
buf.extend(WrongSizeHint {
items: [1, 2].iter(),
claimed: 3,
});
assert_eq!(buf.typed_data::<u32>(), &[1, 2]);

// More items than claimed: nothing is written past the reserved space
let items: Vec<u32> = (0..32).collect();
let mut buf = MutableBuffer::new(0);
buf.extend(WrongSizeHint {
items: items.iter(),
claimed: 2,
});
assert_eq!(buf.typed_data::<u32>(), &[0, 1]);

// A claimed size whose byte count overflows `usize`: the hint is ignored
let mut buf = MutableBuffer::new(0);
buf.extend(WrongSizeHint {
items: [1, 2].iter(),
claimed: usize::MAX / 4 + 1,
});
assert_eq!(buf.typed_data::<u32>(), &[1, 2]);
}

#[test]
fn mutable_extend_from_iter_inexact_size_hint() {
// `filter` reports a lower bound of 0, so nothing is reserved up
// front: the first 16 items fill the existing capacity and the rest
// are pushed
let mut buf = MutableBuffer::new(64);
buf.extend((0..100_u32).filter(|x| x % 3 == 0));
let expected: Vec<u32> = (0..100).filter(|x| x % 3 == 0).collect();
assert_eq!(buf.typed_data::<u32>(), &expected[..]);
}

#[test]
fn mutable_extend_from_slice_unaligned_u64() {
let mut buf = MutableBuffer::new(16);
Expand Down
8 changes: 8 additions & 0 deletions arrow-buffer/src/builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,10 @@ impl<T: ArrowNativeType> Default for BufferBuilder<T> {
}

impl<T: ArrowNativeType> Extend<T> for BufferBuilder<T> {
/// Appends the items of `iter`.
///
/// If `iter`'s [`size_hint`](Iterator::size_hint) reports an exact length,
/// at most that many items are appended.
fn extend<I: IntoIterator<Item = T>>(&mut self, iter: I) {
self.buffer.extend(iter)
}
Expand All @@ -391,6 +395,10 @@ impl<T: ArrowNativeType> From<Vec<T>> for BufferBuilder<T> {
}

impl<T: ArrowNativeType> FromIterator<T> for BufferBuilder<T> {
/// Collects the items of `iter`.
///
/// If `iter`'s [`size_hint`](Iterator::size_hint) reports an exact length,
/// at most that many items are collected.
fn from_iter<I: IntoIterator<Item = T>>(iter: I) -> Self {
let mut builder = Self::default();
builder.extend(iter);
Expand Down
22 changes: 14 additions & 8 deletions arrow-data/src/transform/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
//! Provides utilities for creating, manipulating, and converting Arrow arrays
//! made of primitive types, strings, and nested types.

use super::{ArrayData, ArrayDataBuilder, ByteView, data::new_buffers};
use super::{ArrayData, ArrayDataBuilder, MAX_INLINE_VIEW_LEN, data::new_buffers};
use crate::bit_mask::set_bits;
use arrow_buffer::buffer::{BooleanBuffer, NullBuffer};
use arrow_buffer::{ArrowNativeType, Buffer, IntervalMonthDayNano, MutableBuffer, bit_util, i256};
Expand Down Expand Up @@ -226,13 +226,19 @@ fn build_extend_view(array: &ArrayData, buffer_offset: u32) -> Extend<'_> {
mutable
.buffer1
.extend(views[start..start + len].iter().map(|v| {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a good call out; the branchless view copy is indeed the culprit here. I had Claude explore a bunch of alternatives and it seems that rather than unpacking and repacking the view, we can use this trick instead:

--- a/arrow-data/src/transform/mod.rs
+++ b/arrow-data/src/transform/mod.rs
@@ -20,7 +20,7 @@
 //! Provides utilities for creating, manipulating, and converting Arrow arrays
 //! made of primitive types, strings, and nested types.

-use super::{ArrayData, ArrayDataBuilder, ByteView, data::new_buffers};
+use super::{ArrayData, ArrayDataBuilder, data::new_buffers};
 use crate::bit_mask::set_bits;
 use arrow_buffer::buffer::{BooleanBuffer, NullBuffer};
 use arrow_buffer::{ArrowNativeType, Buffer, IntervalMonthDayNano, MutableBuffer, bit_util, i256};
@@ -229,9 +229,10 @@ fn build_extend_view(array: &ArrayData, buffer_offset: u32) -> Extend<'_> {
                     // 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 };
-                    let mut view = ByteView::from(*v);
-                    view.buffer_index += offset;
-                    u128::from(view)
+                    // The buffer index is bits 64 to 95 of a view. The new index
+                    // is less than the number of data buffers, so the sum never
+                    // carries into the bits above it.
+                    v + ((offset as u128) << 64)

That results in turning the x86 regression into a nice win (~20%) on both x86 and ARM.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the branchless view copy is indeed the culprit here. I had Claude explore a bunch of alternatives and it seems that rather than unpacking and repacking the view, we can use this trick instead

The codegen agrees with that. I built arrow-data with the release profile for x86_64-unknown-linux-gnu (rustc 1.99.0). At e2eecf1, the repack sends the high half of each view through an SSE register to clear the old buffer index:

movq    xmm1, qword ptr [r8 + 8]       # high 64 bits of the view
...
add     r11d, dword ptr [r8 + 8]       # new buffer index
pand    xmm1, xmm0                     # clear the old buffer index
movq    rbx, xmm1
or      rbx, r11                       # merge in the new one

At 2edf0bb, the offset is added straight into the high word from memory. There is no SSE round trip, and LLVM unrolls the loop by two:

cmp     r15d, 13
mov     r12d, 0
cmovae  r12d, r11d                     # buffer_offset, or 0 for an inline view
add     r12, qword ptr [r8 + r14 - 8]  # high 64 bits + offset

On aarch64 the and / add w / orr repack becomes a single add x13, x15, x13. That takes the loop from 11 to 9 instructions per view.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking this!

let len = *v as u32;
if len <= 12 {
return *v; // Stored inline
}
let mut view = ByteView::from(*v);
view.buffer_index += buffer_offset;
view.into()
// A view of a string 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 > MAX_INLINE_VIEW_LEN {
buffer_offset
} else {
0
};
// The buffer index is bits 64 to 95 of a view. Adding
// `buffer_offset` gives the string's buffer index in the
// output, which fits in 32 bits, so the sum never carries
// into bit 96.
v + ((offset as u128) << 64)
}));
Ok(())
},
Expand Down