From ec4dc9b963a422b58d145da8517838982f1ee34c Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 9 Sep 2026 17:28:07 +0100 Subject: [PATCH 1/3] Replace MaybeUninit transmutes with write_copy_of_slice Signed-off-by: Robert Kruszewski --- .../src/bitpacking/array/unpack_iter.rs | 12 +- .../benches/take_slices_to_buffer_matrix.rs | 12 +- .../arrays/filter/execute/byte_compress.rs | 7 +- .../src/arrays/filter/execute/slice.rs | 14 +- .../src/arrays/fixed_width/take/slices.rs | 12 +- .../src/arrays/varbin/compute/take.rs | 23 +-- .../src/arrays/varbinview/compute/take.rs | 23 +-- vortex-array/src/builders/primitive.rs | 7 +- vortex-buffer/src/allocation.rs | 4 +- vortex-buffer/src/buffer.rs | 30 ++- vortex-buffer/src/buffer_mut.rs | 180 ++++++++++++++++-- vortex-buffer/src/const.rs | 10 +- 12 files changed, 218 insertions(+), 116 deletions(-) diff --git a/encodings/fastlanes/src/bitpacking/array/unpack_iter.rs b/encodings/fastlanes/src/bitpacking/array/unpack_iter.rs index 518495896e2..4877fa9c57f 100644 --- a/encodings/fastlanes/src/bitpacking/array/unpack_iter.rs +++ b/encodings/fastlanes/src/bitpacking/array/unpack_iter.rs @@ -190,20 +190,14 @@ impl<'a, T: PhysicalPType, S: UnpackStrategy> UnpackedChunks<'a, T, S> { if let Some(initial) = self.initial() { local_idx = initial.len(); - // TODO(connor): use maybe_uninit_write_slice when it gets stabilized. - // SAFETY: &[T] and &[MaybeUninit] have the same layout. - let init_initial: &[MaybeUninit] = unsafe { mem::transmute(initial) }; - output[..local_idx].copy_from_slice(init_initial); + output[..local_idx].write_copy_of_slice(initial); } local_idx = self.decode_full_chunks_into_at(output, local_idx); if let Some(trailer) = self.trailer() { - // TODO(connor): use maybe_uninit_write_slice when it gets stabilized. - // SAFETY: &[T] and &[MaybeUninit] have the same layout. - let init_trailer: &[MaybeUninit] = unsafe { mem::transmute(trailer) }; - output[local_idx..][..init_trailer.len()].copy_from_slice(init_trailer); - local_idx += init_trailer.len(); + output[local_idx..][..trailer.len()].write_copy_of_slice(trailer); + local_idx += trailer.len(); } debug_assert_eq!(local_idx, self.len); diff --git a/vortex-array/benches/take_slices_to_buffer_matrix.rs b/vortex-array/benches/take_slices_to_buffer_matrix.rs index 21cb539ce32..b534f5e8579 100644 --- a/vortex-array/benches/take_slices_to_buffer_matrix.rs +++ b/vortex-array/benches/take_slices_to_buffer_matrix.rs @@ -4,7 +4,7 @@ //! Microbenchmarks for primitive `take_slices_to_buffer` copy-loop variants. //! //! The matrix covers: -//! - append via `BufferMut::extend_from_slice`, indexed cursor copy, and advancing pointer copy +//! - append via `BufferMut::copy_from_slice`, indexed cursor copy, and advancing pointer copy //! into spare output capacity //! - ordinary checked slicing vs a preverification pass followed by unchecked slicing //! - fixed-width short slices at the run counts used by the FSL take benchmarks @@ -157,7 +157,7 @@ fn take_extend_safe( ) -> Buffer { let mut result = BufferMut::::with_capacity(output_len); for (&start, &length) in starts.iter().zip(lengths) { - result.extend_from_slice(&values[start..start + length]); + result.copy_from_slice(&values[start..start + length]); } result.freeze() } @@ -223,7 +223,7 @@ fn take_preverify_extend_unchecked( for (&start, &length) in starts.iter().zip(lengths) { // SAFETY: `preverify` checked every source range. unsafe { - result.extend_from_slice(values.get_unchecked(start..start + length)); + result.copy_from_slice(values.get_unchecked(start..start + length)); } } result.freeze() @@ -290,8 +290,7 @@ fn preverify(source_len: usize, starts: &[usize], lengths: &[usize], output_len: fn copy_to_spare(result: &mut BufferMut, cursor: usize, source: &[u16]) { let dst = &mut result.spare_capacity_mut()[cursor..][..source.len()]; - // SAFETY: `dst` has exactly `source.len()` spare slots and does not overlap with source. - unsafe { copy_to_uninit(dst.as_mut_ptr().cast(), source) }; + dst.write_copy_of_slice(source); } unsafe fn copy_to_spare_unchecked(result: &mut BufferMut, cursor: usize, source: &[u16]) { @@ -301,8 +300,7 @@ unsafe fn copy_to_spare_unchecked(result: &mut BufferMut, cursor: usize, so .spare_capacity_mut() .get_unchecked_mut(cursor..cursor + source.len()) }; - // SAFETY: `dst` has exactly `source.len()` spare slots and does not overlap with source. - unsafe { copy_to_uninit(dst.as_mut_ptr().cast(), source) }; + dst.write_copy_of_slice(source); } unsafe fn copy_to_uninit(dst: *mut u16, source: &[u16]) { diff --git a/vortex-array/src/arrays/filter/execute/byte_compress.rs b/vortex-array/src/arrays/filter/execute/byte_compress.rs index 032b11a56b3..2af3d75cd43 100644 --- a/vortex-array/src/arrays/filter/execute/byte_compress.rs +++ b/vortex-array/src/arrays/filter/execute/byte_compress.rs @@ -121,17 +121,14 @@ fn filter_chunk_into( return; } - let out_ptr = out.spare_capacity_mut().as_mut_ptr(); if chunk.len() == 8 && mask_byte == 0xFF { // All 8 selected, so bulk copy. - // SAFETY: write_pos + 8 <= capacity. - unsafe { - std::ptr::copy_nonoverlapping(chunk.as_ptr(), out_ptr.add(*write_pos).cast::(), 8); - } + out.spare_capacity_mut()[*write_pos..][..8].write_copy_of_slice(chunk); *write_pos += 8; return; } + let out_ptr = out.spare_capacity_mut().as_mut_ptr(); let (perm, count) = &BYTE_COMPRESS_LUT[mask_byte as usize]; let count = *count as usize; debug_assert_eq!(mask_byte & !low_bits_mask(chunk.len()), 0); diff --git a/vortex-array/src/arrays/filter/execute/slice.rs b/vortex-array/src/arrays/filter/execute/slice.rs index 52d1328c92e..68e900d74f7 100644 --- a/vortex-array/src/arrays/filter/execute/slice.rs +++ b/vortex-array/src/arrays/filter/execute/slice.rs @@ -64,18 +64,14 @@ pub(super) fn filter_slice_by_bitmap(slice: &[T], mask: &MaskValues) -> let output_len = mask.true_count(); let mut out = BufferMut::::with_capacity(output_len); let src_ptr = slice.as_ptr(); - let out_ptr = out.spare_capacity_mut().as_mut_ptr().cast::(); + let spare = out.spare_capacity_mut(); let mut write_pos = 0; for_each_mask_word(mask, |word, word_start, word_len| { let all_selected = low_bits_mask(word_len); debug_assert_eq!(word & !all_selected, 0); if word == all_selected { - // SAFETY: a full mask word selects `word_len` in-bounds source values and the output - // was allocated for every selected value. - unsafe { - ptr::copy_nonoverlapping(src_ptr.add(word_start), out_ptr.add(write_pos), word_len); - } + spare[write_pos..][..word_len].write_copy_of_slice(&slice[word_start..][..word_len]); write_pos += word_len; } else { let mut selected = word; @@ -84,7 +80,9 @@ pub(super) fn filter_slice_by_bitmap(slice: &[T], mask: &MaskValues) -> // SAFETY: set bits are limited to `word_len`, and the output was allocated for // exactly `mask.true_count()` values. unsafe { - out_ptr.add(write_pos).write(*src_ptr.add(index)); + spare + .get_unchecked_mut(write_pos) + .write(*src_ptr.add(index)); } write_pos += 1; selected &= selected - 1; @@ -123,7 +121,7 @@ pub(super) fn filter_slice_by_slices( ) -> Buffer { let mut out = BufferMut::::with_capacity(output_len); for (start, end) in slices { - out.extend_from_slice(&slice[*start..*end]); + out.copy_from_slice(&slice[*start..*end]); } out.freeze() diff --git a/vortex-array/src/arrays/fixed_width/take/slices.rs b/vortex-array/src/arrays/fixed_width/take/slices.rs index a4d9ceda84a..28efe82b625 100644 --- a/vortex-array/src/arrays/fixed_width/take/slices.rs +++ b/vortex-array/src/arrays/fixed_width/take/slices.rs @@ -1,8 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -use std::ptr; - use itertools::Itertools as _; use vortex_buffer::BufferMut; use vortex_buffer::ByteBuffer; @@ -87,15 +85,7 @@ fn copy_slices( let byte_start = start * byte_width; let byte_length = length * byte_width; let source = &values[byte_start..][..byte_length]; - // SAFETY: `source` and the checked spare-capacity range have equal lengths and do not - // overlap. - unsafe { - ptr::copy_nonoverlapping( - source.as_ptr(), - spare[cursor..][..source.len()].as_mut_ptr().cast::(), - source.len(), - ); - } + spare[cursor..][..source.len()].write_copy_of_slice(source); cursor += source.len(); } diff --git a/vortex-array/src/arrays/varbin/compute/take.rs b/vortex-array/src/arrays/varbin/compute/take.rs index 59689d9116c..c41fb8c70af 100644 --- a/vortex-array/src/arrays/varbin/compute/take.rs +++ b/vortex-array/src/arrays/varbin/compute/take.rs @@ -2,7 +2,6 @@ // SPDX-FileCopyrightText: Copyright the Vortex contributors use std::iter; -use std::ptr; use std::sync::Arc; use itertools::Itertools as _; @@ -390,7 +389,7 @@ fn take( let stop = offsets[idx + 1] .to_usize() .vortex_expect("Failed to cast max offset to usize"); - new_data.extend_from_slice(&data[start..stop]); + new_data.copy_from_slice(&data[start..stop]); } let array_validity = Validity::from(dtype.nullability()); @@ -642,14 +641,7 @@ where let byte_start = offset_range[0].as_(); let byte_end = offset_range[length].as_(); let src = &data[byte_start..byte_end]; - // SAFETY: `src` and the checked `spare` range have equal lengths and cannot overlap. - unsafe { - ptr::copy_nonoverlapping( - src.as_ptr(), - spare[cursor..][..src.len()].as_mut_ptr().cast::(), - src.len(), - ); - } + spare[cursor..][..src.len()].write_copy_of_slice(src); cursor += src.len(); } // SAFETY: the loop initialized the prefix `0..cursor` of the spare capacity. @@ -735,14 +727,7 @@ where let byte_start = offset_range[0].as_(); let byte_end = offset_range[length].as_(); let src = &data[byte_start..byte_end]; - // SAFETY: `src` and the checked `spare` range have equal lengths and cannot overlap. - unsafe { - ptr::copy_nonoverlapping( - src.as_ptr(), - spare[cursor..][..src.len()].as_mut_ptr().cast::(), - src.len(), - ); - } + spare[cursor..][..src.len()].write_copy_of_slice(src); cursor += src.len(); } // SAFETY: the loop initialized the prefix `0..cursor` of the spare capacity. @@ -813,7 +798,7 @@ fn take_nullable( let stop = offsets[data_idx + 1] .to_usize() .vortex_expect("Failed to cast max offset to usize"); - new_data.extend_from_slice(&data[start..stop]); + new_data.copy_from_slice(&data[start..stop]); } let array_validity = Validity::from(validity_buffer.freeze()); diff --git a/vortex-array/src/arrays/varbinview/compute/take.rs b/vortex-array/src/arrays/varbinview/compute/take.rs index c10b16cd419..5e63f5359f1 100644 --- a/vortex-array/src/arrays/varbinview/compute/take.rs +++ b/vortex-array/src/arrays/varbinview/compute/take.rs @@ -2,7 +2,6 @@ // SPDX-FileCopyrightText: Copyright the Vortex contributors use std::iter; -use std::ptr; use std::sync::Arc; use itertools::Itertools as _; @@ -180,16 +179,7 @@ where for &start in starts { let start = start.as_(); let src = &source[start..][..length]; - // SAFETY: `src` and the checked `spare` range have equal lengths and cannot overlap. - unsafe { - ptr::copy_nonoverlapping( - src.as_ptr(), - spare[cursor..][..src.len()] - .as_mut_ptr() - .cast::(), - src.len(), - ); - } + spare[cursor..][..src.len()].write_copy_of_slice(src); cursor += src.len(); } // SAFETY: the loop initialized the prefix `0..cursor` of the spare capacity. @@ -219,16 +209,7 @@ where let start = start.as_(); let length = length.as_(); let src = &source[start..][..length]; - // SAFETY: `src` and the checked `spare` range have equal lengths and cannot overlap. - unsafe { - ptr::copy_nonoverlapping( - src.as_ptr(), - spare[cursor..][..src.len()] - .as_mut_ptr() - .cast::(), - src.len(), - ); - } + spare[cursor..][..src.len()].write_copy_of_slice(src); cursor += src.len(); } // SAFETY: the loop initialized the prefix `0..cursor` of the spare capacity. diff --git a/vortex-array/src/builders/primitive.rs b/vortex-array/src/builders/primitive.rs index 647854176c5..4f70f1a90b9 100644 --- a/vortex-array/src/builders/primitive.rs +++ b/vortex-array/src/builders/primitive.rs @@ -160,7 +160,7 @@ impl PrimitiveBuilder { "Cannot append primitive array with different ptype" ); - self.values.extend_from_slice(array.as_slice::()); + self.values.copy_from_slice(array.as_slice::()); self.nulls.append_validity_mask( &array .as_ref() @@ -321,14 +321,11 @@ impl UninitRange<'_, T> { "tried to copy a slice into a `UninitRange` past its boundary" ); - // SAFETY: &[T] and &[MaybeUninit] have the same layout. - let uninit_src: &[MaybeUninit] = unsafe { std::mem::transmute(src) }; - // Note: spare_capacity_mut() returns the spare capacity starting from the current length, // so we just use local_offset directly. let dst = &mut self.builder.values.spare_capacity_mut()[local_offset..local_offset + src.len()]; - dst.copy_from_slice(uninit_src); + dst.write_copy_of_slice(src); } /// Get a mutable slice of uninitialized memory at the specified offset within this range. diff --git a/vortex-buffer/src/allocation.rs b/vortex-buffer/src/allocation.rs index 22792408cca..e86ba55438a 100644 --- a/vortex-buffer/src/allocation.rs +++ b/vortex-buffer/src/allocation.rs @@ -83,7 +83,7 @@ impl BufferAllocatorRef { } /// Copy values into a mutable buffer made by this allocator. - pub fn copy_from(&self, values: impl AsRef<[T]>) -> BufferMut { + pub fn copy_from(&self, values: impl AsRef<[T]>) -> BufferMut { BufferMut::copy_from_in(values, self.clone()) } } @@ -182,7 +182,7 @@ impl StaticBufferAllocator { } /// Copy values into a mutable buffer made by the static allocator. - pub fn copy_from(values: impl AsRef<[T]>) -> BufferMut { + pub fn copy_from(values: impl AsRef<[T]>) -> BufferMut { BufferMut::copy_from(values) } } diff --git a/vortex-buffer/src/buffer.rs b/vortex-buffer/src/buffer.rs index 73d15991f0b..4c338c368d7 100644 --- a/vortex-buffer/src/buffer.rs +++ b/vortex-buffer/src/buffer.rs @@ -167,12 +167,18 @@ impl Buffer { /// of the provided `Vec` while maintaining the ability to convert it back into a mutable /// buffer. We could fix this by forking `Bytes`, or in many other complex ways, but for now /// callers should prefer to construct `Buffer` from a `BufferMut`. - pub fn copy_from(values: impl AsRef<[T]>) -> Self { + pub fn copy_from(values: impl AsRef<[T]>) -> Self + where + T: Copy, + { BufferMut::copy_from(values).freeze() } /// Returns a new `Buffer` copied with the provided allocator. - pub fn copy_from_in(values: impl AsRef<[T]>, allocator: BufferAllocatorRef) -> Self { + pub fn copy_from_in(values: impl AsRef<[T]>, allocator: BufferAllocatorRef) -> Self + where + T: Copy, + { BufferMut::copy_from_in(values, allocator).freeze() } @@ -182,7 +188,10 @@ impl Buffer { /// `alignment`. Use [`copy_from_preferred_aligned`] to control the over-alignment. /// /// [`copy_from_preferred_aligned`]: Self::copy_from_preferred_aligned - pub fn copy_from_aligned(values: impl AsRef<[T]>, alignment: Alignment) -> Self { + pub fn copy_from_aligned(values: impl AsRef<[T]>, alignment: Alignment) -> Self + where + T: Copy, + { Self::copy_from_preferred_aligned(values, alignment, Some(Alignment::DEFAULT_ALIGNMENT)) } @@ -194,7 +203,10 @@ impl Buffer { values: impl AsRef<[T]>, alignment: Alignment, preferred_alignment: Option, - ) -> Self { + ) -> Self + where + T: Copy, + { BufferMut::copy_from_preferred_aligned(values, alignment, preferred_alignment).freeze() } @@ -670,7 +682,10 @@ impl Buffer { } /// Convert self into `BufferMut`, cloning the data if there are multiple strong references. - pub fn into_mut(self) -> BufferMut { + pub fn into_mut(self) -> BufferMut + where + T: Copy, + { self.try_into_mut().unwrap_or_else(|buffer| { let allocator = buffer.allocator().clone(); BufferMut::::copy_from_aligned_in(&buffer, buffer.alignment, allocator) @@ -683,7 +698,10 @@ impl Buffer { } /// Return a `Buffer` with the given alignment. Where possible, this will be zero-copy. - pub fn aligned(mut self, alignment: Alignment) -> Self { + pub fn aligned(mut self, alignment: Alignment) -> Self + where + T: Copy, + { if alignment.is_ptr_aligned(self.as_ptr()) { self.alignment = alignment; self diff --git a/vortex-buffer/src/buffer_mut.rs b/vortex-buffer/src/buffer_mut.rs index 71e27c4f8ce..9896ab45d14 100644 --- a/vortex-buffer/src/buffer_mut.rs +++ b/vortex-buffer/src/buffer_mut.rs @@ -308,12 +308,18 @@ impl BufferMut { } /// Create a mutable scalar buffer by copying the contents of the slice. - pub fn copy_from(other: impl AsRef<[T]>) -> Self { + pub fn copy_from(other: impl AsRef<[T]>) -> Self + where + T: Copy, + { Self::copy_from_in(other, BufferAllocatorRef::statically_allocated()) } /// Create a mutable scalar buffer by copying with the given allocator. - pub fn copy_from_in(other: impl AsRef<[T]>, allocator: BufferAllocatorRef) -> Self { + pub fn copy_from_in(other: impl AsRef<[T]>, allocator: BufferAllocatorRef) -> Self + where + T: Copy, + { Self::copy_from_aligned_in(other, Alignment::of::(), allocator) } @@ -327,7 +333,10 @@ impl BufferMut { /// ## Panics /// /// Panics when the requested alignment isn't itself aligned to type T. - pub fn copy_from_aligned(other: impl AsRef<[T]>, alignment: Alignment) -> Self { + pub fn copy_from_aligned(other: impl AsRef<[T]>, alignment: Alignment) -> Self + where + T: Copy, + { Self::copy_from_aligned_in(other, alignment, BufferAllocatorRef::statically_allocated()) } @@ -336,7 +345,10 @@ impl BufferMut { other: impl AsRef<[T]>, alignment: Alignment, allocator: BufferAllocatorRef, - ) -> Self { + ) -> Self + where + T: Copy, + { Self::copy_from_preferred_aligned_in( other, alignment, @@ -357,7 +369,10 @@ impl BufferMut { other: impl AsRef<[T]>, alignment: Alignment, preferred_alignment: Option, - ) -> Self { + ) -> Self + where + T: Copy, + { Self::copy_from_preferred_aligned_in( other, alignment, @@ -372,7 +387,10 @@ impl BufferMut { alignment: Alignment, preferred_alignment: Option, allocator: BufferAllocatorRef, - ) -> Self { + ) -> Self + where + T: Copy, + { if !alignment.is_aligned_to(Alignment::of::()) { vortex_panic!("Given alignment is not aligned to type T") } @@ -383,7 +401,7 @@ impl BufferMut { preferred_alignment, allocator, ); - buffer.extend_from_slice(other); + buffer.copy_from_slice(other); debug_assert_eq!(buffer.alignment(), alignment); buffer } @@ -663,7 +681,13 @@ impl BufferMut { self.length += n; } - /// Appends a slice of type `T`, growing the internal buffer as needed. + /// Appends a slice by cloning its elements, growing the internal buffer as needed. + /// + /// Prefer [`Self::copy_from_slice`] when `T` implements `Copy`. + /// + /// # Panics + /// + /// If cloning an element panics, the buffer's length and existing values are unchanged. /// /// # Example: /// @@ -676,16 +700,52 @@ impl BufferMut { /// assert_eq!(builder.len(), 3); /// ``` #[inline] - pub fn extend_from_slice(&mut self, slice: &[T]) { + pub fn extend_from_slice(&mut self, slice: &[T]) + where + T: Clone, + { self.reserve(slice.len()); - // SAFETY: reserve made the destination valid and non-overlapping for slice.len() values. - unsafe { - std::ptr::copy_nonoverlapping( - slice.as_ptr(), - self.as_mut_ptr().add(self.length), - slice.len(), - ); - } + // SAFETY: reserve guarantees at least slice.len() spare slots. + let dst = unsafe { self.spare_capacity_mut().get_unchecked_mut(..slice.len()) }; + dst.write_clone_of_slice(slice); + self.length += slice.len(); + } + + /// Appends a slice by copying its elements, growing the internal buffer as needed. + /// + /// This does not call [`Clone::clone`]. For types that only implement `Clone`, use + /// [`Self::extend_from_slice`]. + /// + /// # Example + /// + /// ``` + /// use vortex_buffer::BufferMut; + /// + /// let mut buffer = BufferMut::from_iter([1, 2]); + /// buffer.copy_from_slice(&[3, 4]); + /// assert_eq!(buffer.as_slice(), &[1, 2, 3, 4]); + /// ``` + /// + /// Elements must implement `Copy`, even when they implement `Clone`. + /// + /// ```compile_fail,E0277 + /// use vortex_buffer::BufferMut; + /// + /// #[derive(Clone)] + /// struct NotCopy(u32); + /// + /// let mut buffer = BufferMut::with_capacity(1); + /// buffer.copy_from_slice(&[NotCopy(1)]); + /// ``` + #[inline] + pub fn copy_from_slice(&mut self, slice: &[T]) + where + T: Copy, + { + self.reserve(slice.len()); + // SAFETY: reserve guarantees at least slice.len() spare slots. + let dst = unsafe { self.spare_capacity_mut().get_unchecked_mut(..slice.len()) }; + dst.write_copy_of_slice(slice); self.length += slice.len(); } @@ -734,14 +794,17 @@ impl BufferMut { /// If the data is already properly aligned, this is a metadata-only operation. /// /// If the data is not aligned, we copy it into a new allocation. - pub fn aligned(self, alignment: Alignment) -> Self { + pub fn aligned(self, alignment: Alignment) -> Self + where + T: Copy, + { if self.as_ptr().align_offset(alignment.as_usize()) == 0 { Self { alignment, ..self } } else { let capacity = self.capacity(); let allocator = self.allocation.allocator().clone(); let mut aligned = Self::with_capacity_aligned_in(capacity, alignment, allocator); - aligned.extend_from_slice(&self); + aligned.copy_from_slice(&self); aligned.capacity = capacity; aligned } @@ -777,7 +840,7 @@ impl BufferMut { } } -impl Clone for BufferMut { +impl Clone for BufferMut { fn clone(&self) -> Self { let mut buffer = BufferMut::::with_capacity_aligned_in( self.capacity(), @@ -994,11 +1057,86 @@ impl FromIterator for BufferMut { } #[cfg(test)] -mod test { +mod tests { + use std::cell::Cell; + use std::panic::AssertUnwindSafe; + use std::panic::catch_unwind; + use crate::Alignment; use crate::BufferMut; use crate::buffer_mut; + #[derive(Clone, Debug, PartialEq)] + struct CloneOnly(u32); + + #[derive(Copy, Debug, PartialEq)] + struct CopyWithCloneCounter<'a> { + value: u32, + clones: &'a Cell, + panic_at: Option, + } + + #[allow(clippy::non_canonical_clone_impl)] + impl Clone for CopyWithCloneCounter<'_> { + fn clone(&self) -> Self { + self.clones.set(self.clones.get() + 1); + assert_ne!(Some(self.clones.get()), self.panic_at, "clone panicked"); + *self + } + } + + #[test] + fn extend_from_slice_supports_clone_only() { + let mut buffer = BufferMut::empty(); + buffer.push(CloneOnly(1)); + buffer.extend_from_slice(&[CloneOnly(2), CloneOnly(3)]); + + assert_eq!( + buffer.as_slice(), + &[CloneOnly(1), CloneOnly(2), CloneOnly(3)] + ); + assert_eq!(buffer.clone().as_slice(), buffer.as_slice()); + } + + #[test] + fn copy_from_slice_skips_clone() { + let clones = Cell::new(0); + let source = [CopyWithCloneCounter { + value: 42, + clones: &clones, + panic_at: None, + }]; + let mut buffer = BufferMut::empty(); + buffer.extend_from_slice(&source); + assert_eq!(clones.get(), 1); + + buffer.copy_from_slice(&source); + buffer.copy_from_slice(&[]); + assert_eq!(clones.get(), 1); + assert_eq!(buffer.as_slice(), &[source[0], source[0]]); + } + + #[test] + fn extend_from_slice_preserves_buffer_on_clone_panic() { + let clones = Cell::new(0); + let source = [CopyWithCloneCounter { + value: 42, + clones: &clones, + panic_at: Some(2), + }; 2]; + let mut buffer = BufferMut::empty(); + buffer.push(source[0]); + + let result = catch_unwind(AssertUnwindSafe(|| buffer.extend_from_slice(&source))); + assert!(result.is_err()); + assert_eq!(clones.get(), 2); + assert_eq!(buffer.as_slice(), &source[..1]); + + buffer.copy_from_slice(&source); + assert_eq!(clones.get(), 2); + assert_eq!(buffer.as_slice(), &[source[0]; 3]); + } + #[test] fn capacity() { let mut n = 57; diff --git a/vortex-buffer/src/const.rs b/vortex-buffer/src/const.rs index d37631c3eea..e49ba46dfe5 100644 --- a/vortex-buffer/src/const.rs +++ b/vortex-buffer/src/const.rs @@ -20,12 +20,18 @@ impl ConstBuffer { } /// Align the given buffer (possibly with a copy) and return a new `ConstBuffer`. - pub fn align_from>>(buf: B) -> Self { + pub fn align_from>>(buf: B) -> Self + where + T: Copy, + { Self(buf.into().aligned(Self::alignment())) } /// Create a new [`ConstBuffer`] with a copy from the provided slice. - pub fn copy_from>(buf: B) -> Self { + pub fn copy_from>(buf: B) -> Self + where + T: Copy, + { Self(Buffer::::copy_from_aligned(buf, Self::alignment())) } From 29b8c59a678cf3a94628c0bb3e1d8ca491fad996 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 9 Sep 2026 17:45:52 +0100 Subject: [PATCH 2/3] less Signed-off-by: Robert Kruszewski --- .../benches/take_slices_to_buffer_matrix.rs | 6 +- .../src/arrays/filter/execute/slice.rs | 2 +- .../src/arrays/varbin/compute/take.rs | 4 +- vortex-array/src/builders/primitive.rs | 2 +- vortex-buffer/src/buffer_mut.rs | 99 +++---------------- 5 files changed, 20 insertions(+), 93 deletions(-) diff --git a/vortex-array/benches/take_slices_to_buffer_matrix.rs b/vortex-array/benches/take_slices_to_buffer_matrix.rs index b534f5e8579..40813129dd6 100644 --- a/vortex-array/benches/take_slices_to_buffer_matrix.rs +++ b/vortex-array/benches/take_slices_to_buffer_matrix.rs @@ -4,7 +4,7 @@ //! Microbenchmarks for primitive `take_slices_to_buffer` copy-loop variants. //! //! The matrix covers: -//! - append via `BufferMut::copy_from_slice`, indexed cursor copy, and advancing pointer copy +//! - append via `BufferMut::extend_from_slice`, indexed cursor copy, and advancing pointer copy //! into spare output capacity //! - ordinary checked slicing vs a preverification pass followed by unchecked slicing //! - fixed-width short slices at the run counts used by the FSL take benchmarks @@ -157,7 +157,7 @@ fn take_extend_safe( ) -> Buffer { let mut result = BufferMut::::with_capacity(output_len); for (&start, &length) in starts.iter().zip(lengths) { - result.copy_from_slice(&values[start..start + length]); + result.extend_from_slice(&values[start..start + length]); } result.freeze() } @@ -223,7 +223,7 @@ fn take_preverify_extend_unchecked( for (&start, &length) in starts.iter().zip(lengths) { // SAFETY: `preverify` checked every source range. unsafe { - result.copy_from_slice(values.get_unchecked(start..start + length)); + result.extend_from_slice(values.get_unchecked(start..start + length)); } } result.freeze() diff --git a/vortex-array/src/arrays/filter/execute/slice.rs b/vortex-array/src/arrays/filter/execute/slice.rs index 68e900d74f7..91556c7b541 100644 --- a/vortex-array/src/arrays/filter/execute/slice.rs +++ b/vortex-array/src/arrays/filter/execute/slice.rs @@ -121,7 +121,7 @@ pub(super) fn filter_slice_by_slices( ) -> Buffer { let mut out = BufferMut::::with_capacity(output_len); for (start, end) in slices { - out.copy_from_slice(&slice[*start..*end]); + out.extend_from_slice(&slice[*start..*end]); } out.freeze() diff --git a/vortex-array/src/arrays/varbin/compute/take.rs b/vortex-array/src/arrays/varbin/compute/take.rs index c41fb8c70af..46bc1ec5b33 100644 --- a/vortex-array/src/arrays/varbin/compute/take.rs +++ b/vortex-array/src/arrays/varbin/compute/take.rs @@ -389,7 +389,7 @@ fn take( let stop = offsets[idx + 1] .to_usize() .vortex_expect("Failed to cast max offset to usize"); - new_data.copy_from_slice(&data[start..stop]); + new_data.extend_from_slice(&data[start..stop]); } let array_validity = Validity::from(dtype.nullability()); @@ -798,7 +798,7 @@ fn take_nullable( let stop = offsets[data_idx + 1] .to_usize() .vortex_expect("Failed to cast max offset to usize"); - new_data.copy_from_slice(&data[start..stop]); + new_data.extend_from_slice(&data[start..stop]); } let array_validity = Validity::from(validity_buffer.freeze()); diff --git a/vortex-array/src/builders/primitive.rs b/vortex-array/src/builders/primitive.rs index 4f70f1a90b9..45198a1e7c5 100644 --- a/vortex-array/src/builders/primitive.rs +++ b/vortex-array/src/builders/primitive.rs @@ -160,7 +160,7 @@ impl PrimitiveBuilder { "Cannot append primitive array with different ptype" ); - self.values.copy_from_slice(array.as_slice::()); + self.values.extend_from_slice(array.as_slice::()); self.nulls.append_validity_mask( &array .as_ref() diff --git a/vortex-buffer/src/buffer_mut.rs b/vortex-buffer/src/buffer_mut.rs index 9896ab45d14..75a57992da4 100644 --- a/vortex-buffer/src/buffer_mut.rs +++ b/vortex-buffer/src/buffer_mut.rs @@ -401,7 +401,7 @@ impl BufferMut { preferred_alignment, allocator, ); - buffer.copy_from_slice(other); + buffer.extend_from_slice(other); debug_assert_eq!(buffer.alignment(), alignment); buffer } @@ -681,40 +681,9 @@ impl BufferMut { self.length += n; } - /// Appends a slice by cloning its elements, growing the internal buffer as needed. - /// - /// Prefer [`Self::copy_from_slice`] when `T` implements `Copy`. - /// - /// # Panics - /// - /// If cloning an element panics, the buffer's length and existing values are unchanged. - /// - /// # Example: - /// - /// ``` - /// # use vortex_buffer::BufferMut; - /// - /// let mut builder = BufferMut::::with_capacity(10); - /// builder.extend_from_slice(&[42, 44, 46]); - /// - /// assert_eq!(builder.len(), 3); - /// ``` - #[inline] - pub fn extend_from_slice(&mut self, slice: &[T]) - where - T: Clone, - { - self.reserve(slice.len()); - // SAFETY: reserve guarantees at least slice.len() spare slots. - let dst = unsafe { self.spare_capacity_mut().get_unchecked_mut(..slice.len()) }; - dst.write_clone_of_slice(slice); - self.length += slice.len(); - } - /// Appends a slice by copying its elements, growing the internal buffer as needed. /// - /// This does not call [`Clone::clone`]. For types that only implement `Clone`, use - /// [`Self::extend_from_slice`]. + /// This does not call [`Clone::clone`]. /// /// # Example /// @@ -722,7 +691,7 @@ impl BufferMut { /// use vortex_buffer::BufferMut; /// /// let mut buffer = BufferMut::from_iter([1, 2]); - /// buffer.copy_from_slice(&[3, 4]); + /// buffer.extend_from_slice(&[3, 4]); /// assert_eq!(buffer.as_slice(), &[1, 2, 3, 4]); /// ``` /// @@ -735,10 +704,10 @@ impl BufferMut { /// struct NotCopy(u32); /// /// let mut buffer = BufferMut::with_capacity(1); - /// buffer.copy_from_slice(&[NotCopy(1)]); + /// buffer.extend_from_slice(&[NotCopy(1)]); /// ``` #[inline] - pub fn copy_from_slice(&mut self, slice: &[T]) + pub fn extend_from_slice(&mut self, slice: &[T]) where T: Copy, { @@ -804,7 +773,7 @@ impl BufferMut { let capacity = self.capacity(); let allocator = self.allocation.allocator().clone(); let mut aligned = Self::with_capacity_aligned_in(capacity, alignment, allocator); - aligned.copy_from_slice(&self); + aligned.extend_from_slice(&self); aligned.capacity = capacity; aligned } @@ -840,7 +809,7 @@ impl BufferMut { } } -impl Clone for BufferMut { +impl Clone for BufferMut { fn clone(&self) -> Self { let mut buffer = BufferMut::::with_capacity_aligned_in( self.capacity(), @@ -1059,82 +1028,40 @@ impl FromIterator for BufferMut { #[cfg(test)] mod tests { use std::cell::Cell; - use std::panic::AssertUnwindSafe; - use std::panic::catch_unwind; use crate::Alignment; use crate::BufferMut; use crate::buffer_mut; - #[derive(Clone, Debug, PartialEq)] - struct CloneOnly(u32); - #[derive(Copy, Debug, PartialEq)] struct CopyWithCloneCounter<'a> { value: u32, clones: &'a Cell, - panic_at: Option, } #[allow(clippy::non_canonical_clone_impl)] impl Clone for CopyWithCloneCounter<'_> { fn clone(&self) -> Self { self.clones.set(self.clones.get() + 1); - assert_ne!(Some(self.clones.get()), self.panic_at, "clone panicked"); *self } } #[test] - fn extend_from_slice_supports_clone_only() { - let mut buffer = BufferMut::empty(); - buffer.push(CloneOnly(1)); - buffer.extend_from_slice(&[CloneOnly(2), CloneOnly(3)]); - - assert_eq!( - buffer.as_slice(), - &[CloneOnly(1), CloneOnly(2), CloneOnly(3)] - ); - assert_eq!(buffer.clone().as_slice(), buffer.as_slice()); - } - - #[test] - fn copy_from_slice_skips_clone() { + fn extend_from_slice_skips_clone() { let clones = Cell::new(0); let source = [CopyWithCloneCounter { value: 42, clones: &clones, - panic_at: None, }]; let mut buffer = BufferMut::empty(); buffer.extend_from_slice(&source); - assert_eq!(clones.get(), 1); - - buffer.copy_from_slice(&source); - buffer.copy_from_slice(&[]); - assert_eq!(clones.get(), 1); + buffer.extend_from_slice(&source); + buffer.extend_from_slice(&[]); + assert_eq!(clones.get(), 0); assert_eq!(buffer.as_slice(), &[source[0], source[0]]); - } - - #[test] - fn extend_from_slice_preserves_buffer_on_clone_panic() { - let clones = Cell::new(0); - let source = [CopyWithCloneCounter { - value: 42, - clones: &clones, - panic_at: Some(2), - }; 2]; - let mut buffer = BufferMut::empty(); - buffer.push(source[0]); - - let result = catch_unwind(AssertUnwindSafe(|| buffer.extend_from_slice(&source))); - assert!(result.is_err()); - assert_eq!(clones.get(), 2); - assert_eq!(buffer.as_slice(), &source[..1]); - - buffer.copy_from_slice(&source); - assert_eq!(clones.get(), 2); - assert_eq!(buffer.as_slice(), &[source[0]; 3]); + assert_eq!(buffer.clone().as_slice(), buffer.as_slice()); + assert_eq!(clones.get(), 0); } #[test] From 2720cefb41d817abc922b0e28a76b6bd3ce0d11c Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 9 Sep 2026 17:49:01 +0100 Subject: [PATCH 3/3] less Signed-off-by: Robert Kruszewski --- vortex-buffer/src/buffer_mut.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/vortex-buffer/src/buffer_mut.rs b/vortex-buffer/src/buffer_mut.rs index 75a57992da4..fd2ff059704 100644 --- a/vortex-buffer/src/buffer_mut.rs +++ b/vortex-buffer/src/buffer_mut.rs @@ -712,8 +712,10 @@ impl BufferMut { T: Copy, { self.reserve(slice.len()); - // SAFETY: reserve guarantees at least slice.len() spare slots. - let dst = unsafe { self.spare_capacity_mut().get_unchecked_mut(..slice.len()) }; + let dst = self + .spare_capacity_mut() + .get_mut(..slice.len()) + .vortex_expect("reserve guarantees sufficient spare capacity"); dst.write_copy_of_slice(slice); self.length += slice.len(); }