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..40813129dd6 100644 --- a/vortex-array/benches/take_slices_to_buffer_matrix.rs +++ b/vortex-array/benches/take_slices_to_buffer_matrix.rs @@ -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..91556c7b541 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; 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..46bc1ec5b33 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 _; @@ -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. 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..45198a1e7c5 100644 --- a/vortex-array/src/builders/primitive.rs +++ b/vortex-array/src/builders/primitive.rs @@ -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..fd2ff059704 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") } @@ -663,29 +681,42 @@ impl BufferMut { self.length += n; } - /// Appends a slice of type `T`, growing the internal buffer as needed. + /// Appends a slice by copying its elements, growing the internal buffer as needed. + /// + /// This does not call [`Clone::clone`]. + /// + /// # Example /// - /// # Example: + /// ``` + /// use vortex_buffer::BufferMut; /// + /// let mut buffer = BufferMut::from_iter([1, 2]); + /// buffer.extend_from_slice(&[3, 4]); + /// assert_eq!(buffer.as_slice(), &[1, 2, 3, 4]); /// ``` - /// # use vortex_buffer::BufferMut; /// - /// let mut builder = BufferMut::::with_capacity(10); - /// builder.extend_from_slice(&[42, 44, 46]); + /// Elements must implement `Copy`, even when they implement `Clone`. + /// + /// ```compile_fail,E0277 + /// use vortex_buffer::BufferMut; + /// + /// #[derive(Clone)] + /// struct NotCopy(u32); /// - /// assert_eq!(builder.len(), 3); + /// let mut buffer = BufferMut::with_capacity(1); + /// buffer.extend_from_slice(&[NotCopy(1)]); /// ``` #[inline] - pub fn extend_from_slice(&mut self, slice: &[T]) { + pub fn extend_from_slice(&mut self, slice: &[T]) + where + T: Copy, + { 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(), - ); - } + 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(); } @@ -734,7 +765,10 @@ 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 { @@ -777,7 +811,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 +1028,44 @@ impl FromIterator for BufferMut { } #[cfg(test)] -mod test { +mod tests { + use std::cell::Cell; + use crate::Alignment; use crate::BufferMut; use crate::buffer_mut; + #[derive(Copy, Debug, PartialEq)] + struct CopyWithCloneCounter<'a> { + value: u32, + clones: &'a Cell, + } + + #[allow(clippy::non_canonical_clone_impl)] + impl Clone for CopyWithCloneCounter<'_> { + fn clone(&self) -> Self { + self.clones.set(self.clones.get() + 1); + *self + } + } + + #[test] + fn extend_from_slice_skips_clone() { + let clones = Cell::new(0); + let source = [CopyWithCloneCounter { + value: 42, + clones: &clones, + }]; + let mut buffer = BufferMut::empty(); + buffer.extend_from_slice(&source); + buffer.extend_from_slice(&source); + buffer.extend_from_slice(&[]); + assert_eq!(clones.get(), 0); + assert_eq!(buffer.as_slice(), &[source[0], source[0]]); + assert_eq!(buffer.clone().as_slice(), buffer.as_slice()); + assert_eq!(clones.get(), 0); + } + #[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())) }