Skip to content

Commit b0aa3bc

Browse files
committed
fix(buffer): avoid shrinking allocations through grow
Signed-off-by: "Connor Tsui" <[email protected]>
1 parent 2d9414a commit b0aa3bc

1 file changed

Lines changed: 96 additions & 37 deletions

File tree

‎vortex-buffer/src/buffer_mut.rs‎

Lines changed: 96 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -502,73 +502,109 @@ impl<T> BufferMut<T> {
502502
self.reserve_allocate(additional);
503503
}
504504

505-
/// A separate function so we can inline the reserve call's fast path.
505+
/// Keeps allocation work out of the inline fast path in [`Self::reserve`].
506+
///
507+
/// Buffer capacity measures the space available from the data pointer. The allocation also
508+
/// includes any bytes before that pointer, which can come from alignment padding or slicing.
509+
/// The new layout follows the buffer growth policy without retaining that old prefix.
510+
///
511+
/// For a byte buffer:
512+
///
513+
/// ```text
514+
/// Before reserve(100): allocation = 1,000 bytes, capacity = 100 bytes.
515+
/// | 900 bytes before slice | 100 data |
516+
/// ^ data pointer
517+
///
518+
/// After reserve(100): allocation = 257 bytes, capacity = 256 bytes.
519+
/// | 100 data | 156 spare | 1 unused |
520+
/// ^ data pointer
521+
/// ```
522+
///
523+
/// [`Allocator::grow`] requires the new allocation to be at least as large as the entire old
524+
/// allocation. If the new layout is smaller, allocate a separate block and copy the initialized
525+
/// data instead of violating that safety contract.
526+
///
527+
/// [`Allocator::grow`]: allocator_api2::alloc::Allocator::grow
506528
fn reserve_allocate(&mut self, additional: usize) {
507-
let required = self
529+
let required_capacity = self
508530
.length
509531
.checked_add(additional)
510532
.vortex_expect("buffer capacity overflow");
511-
let required_size = required
533+
let required_bytes = required_capacity
512534
.checked_mul(size_of::<T>())
513535
.vortex_expect("buffer capacity overflow");
514-
let alignment = self.alignment;
515-
let current_size = self
536+
let capacity_bytes = self
516537
.capacity
517538
.checked_mul(size_of::<T>())
518539
.vortex_expect("buffer capacity overflow");
519-
let logical_size = required_size
520-
.max(current_size.saturating_mul(2))
540+
let new_capacity_bytes = required_bytes
541+
.max(capacity_bytes.saturating_mul(2))
521542
.max(Alignment::DEFAULT_ALIGNMENT.as_usize());
522-
let allocation_size = logical_size
523-
.checked_add(alignment.as_usize())
543+
544+
// Reserve space before the data so its pointer can satisfy the buffer's alignment.
545+
let data_alignment = self.alignment.as_usize();
546+
let new_allocation_bytes = new_capacity_bytes
547+
.checked_add(data_alignment)
524548
.vortex_expect("buffer capacity overflow");
525549
let allocation_alignment = if self.allocation.size() == 0 {
526550
1
527551
} else {
528552
self.allocation.alignment()
529553
};
530-
let layout = Layout::from_size_align(allocation_size, allocation_alignment)
554+
let new_layout = Layout::from_size_align(new_allocation_bytes, allocation_alignment)
531555
.unwrap_or_else(|_| vortex_panic!("buffer capacity exceeds maximum allocation size"));
556+
let initialized_bytes = self.length * size_of::<T>();
557+
558+
// The default global allocator (`is_statically_allocated`) uses allocate-and-copy. Custom
559+
// allocators must also allocate and copy when the new layout is smaller, because
560+
// `Allocator::grow` forbids shrinking.
561+
let needs_new_allocation = self.allocation.allocator().is_statically_allocated()
562+
|| new_layout.size() < self.allocation.size();
532563

533-
let old_offset = self.ptr.cast::<u8>().addr().get() - self.allocation.ptr().addr().get();
534-
let new_offset = if self.allocation.allocator().is_statically_allocated() {
535-
let allocation =
536-
Allocation::allocate(layout, BufferAllocatorRef::statically_allocated());
537-
let new_offset = allocation.ptr().as_ptr().align_offset(alignment.as_usize());
538-
// SAFETY: both allocations have room for the initialized elements and do not overlap.
564+
let data_offset = if needs_new_allocation {
565+
let new_allocation =
566+
Allocation::allocate(new_layout, self.allocation.allocator().clone());
567+
let new_data_offset = new_allocation.ptr().as_ptr().align_offset(data_alignment);
568+
569+
// SAFETY: self.ptr addresses initialized_bytes of live data. The new allocation has
570+
// room for those bytes after alignment padding and is disjoint from the old allocation.
539571
unsafe {
540572
std::ptr::copy_nonoverlapping(
541573
self.ptr.cast::<u8>().as_ptr(),
542-
allocation.ptr().as_ptr().add(new_offset),
543-
self.length * size_of::<T>(),
574+
new_allocation.ptr().as_ptr().add(new_data_offset),
575+
initialized_bytes,
544576
);
545577
}
546-
self.allocation = allocation;
547-
new_offset
578+
579+
self.allocation = new_allocation;
580+
581+
new_data_offset
548582
} else {
549-
self.allocation.grow(layout);
550-
let new_offset = self
551-
.allocation
552-
.ptr()
553-
.as_ptr()
554-
.align_offset(alignment.as_usize());
555-
if new_offset != old_offset {
556-
// SAFETY: grow preserved the initialized elements at old_offset. The new allocation
557-
// has room for the requested elements plus alignment padding, and copy permits
558-
// overlap.
583+
let old_data_offset =
584+
self.ptr.cast::<u8>().addr().get() - self.allocation.ptr().addr().get();
585+
self.allocation.grow(new_layout);
586+
587+
let new_data_offset = self.allocation.ptr().as_ptr().align_offset(data_alignment);
588+
589+
if new_data_offset != old_data_offset {
590+
// SAFETY: grow preserved initialized_bytes at old_data_offset. The reserved padding
591+
// leaves room for the data at new_data_offset, and copy permits overlap.
559592
unsafe {
560593
std::ptr::copy(
561-
self.allocation.ptr().as_ptr().add(old_offset),
562-
self.allocation.ptr().as_ptr().add(new_offset),
563-
self.length * size_of::<T>(),
594+
self.allocation.ptr().as_ptr().add(old_data_offset),
595+
self.allocation.ptr().as_ptr().add(new_data_offset),
596+
initialized_bytes,
564597
);
565598
}
566599
}
567-
new_offset
600+
601+
new_data_offset
568602
};
569-
// SAFETY: new_offset was computed within the allocation for alignment.
570-
self.ptr = unsafe { self.allocation.ptr().add(new_offset).cast() };
571-
self.capacity = logical_size / size_of::<T>();
603+
604+
// SAFETY: data_offset satisfies the required alignment and leaves new_capacity_bytes
605+
// available within the allocation.
606+
self.ptr = unsafe { self.allocation.ptr().add(data_offset).cast() };
607+
self.capacity = new_capacity_bytes / size_of::<T>();
572608
}
573609

574610
/// Returns the spare capacity of the buffer as a slice of `MaybeUninit<T>`.
@@ -1149,6 +1185,29 @@ mod tests {
11491185
assert_eq!(buffer[capacity], u32::MAX);
11501186
}
11511187

1188+
#[test]
1189+
fn growth_of_sliced_buffer_preserves_allocator_and_values() {
1190+
let allocator = BufferAllocatorRef::new(Global);
1191+
let mut buffer = BufferMut::<u32>::with_capacity_preferred_aligned_in(
1192+
1024,
1193+
Alignment::of::<u32>(),
1194+
None,
1195+
allocator.clone(),
1196+
);
1197+
buffer.extend_from_slice(&[7; 1024]);
1198+
let sliced_ptr = buffer.as_ptr().wrapping_add(1016);
1199+
let sliced = buffer.freeze().slice(1016..);
1200+
let mut buffer = sliced.try_into_mut().unwrap();
1201+
assert_eq!(buffer.as_ptr(), sliced_ptr);
1202+
1203+
// The slice retains a large backing allocation, but needs only a small capacity increase.
1204+
buffer.extend_from_slice(&[8; 16]);
1205+
1206+
assert!(buffer.allocator().ptr_eq(&allocator));
1207+
assert_eq!(&buffer[..8], &[7; 8]);
1208+
assert_eq!(&buffer[8..], &[8; 16]);
1209+
}
1210+
11521211
#[test]
11531212
fn raising_logical_alignment_preserves_capacity() {
11541213
let buffer =

0 commit comments

Comments
 (0)