From e852ffb331e66ae673061824f640f8c5ba84d93c Mon Sep 17 00:00:00 2001 From: Dillard Robertson Date: Sun, 3 Aug 2025 12:07:47 -0400 Subject: [PATCH 1/4] Revert "perf: use `NonNull` for null pointer optimization (#45)" This reverts commit d55eca7561caf3361692f4908ec47a18c214cbf6. --- src/smallbox.rs | 117 ++++++++++++++++-------------------------------- 1 file changed, 38 insertions(+), 79 deletions(-) diff --git a/src/smallbox.rs b/src/smallbox.rs index 2c1cb8a..2213e85 100644 --- a/src/smallbox.rs +++ b/src/smallbox.rs @@ -5,7 +5,6 @@ use core::fmt; use core::future::Future; use core::hash::Hash; use core::hash::{self}; -use core::hint::unreachable_unchecked; use core::marker::PhantomData; #[cfg(feature = "coerce")] use core::marker::Unsize; @@ -17,7 +16,6 @@ use core::ops; use core::ops::CoerceUnsized; use core::pin::Pin; use core::ptr; -use core::ptr::NonNull; use ::alloc::alloc; use ::alloc::alloc::Layout; @@ -26,17 +24,6 @@ use ::alloc::boxed::Box; use crate::sptr; -/// A sentinel pointer that signals that the value is stored on the stack -/// -/// It is never supposed to be dereferenced -const INLINE_SENTINEL: *mut u8 = sptr::without_provenance_mut(0x1); - -/// Minimum alignment for allocations -/// -/// Forcing a minimum alignment prevents the allocator -/// from returning a pointer with the same address as `INLINE_SENTINEL` -const MIN_ALIGNMENT: usize = 2; - #[cfg(feature = "coerce")] impl, U: ?Sized, Space> CoerceUnsized> for SmallBox @@ -87,7 +74,7 @@ macro_rules! smallbox { /// An optimized box that store value on stack or on heap depending on its size pub struct SmallBox { space: MaybeUninit>, - ptr: NonNull, + ptr: *const T, _phantom: PhantomData, } @@ -177,49 +164,39 @@ impl SmallBox { /// ``` #[inline] pub fn is_heap(&self) -> bool { - self.ptr.as_ptr().cast::() != INLINE_SENTINEL + !self.ptr.is_null() } unsafe fn new_copy(val: &U, metadata_ptr: *const T) -> SmallBox where U: ?Sized { - let layout = Layout::for_value::(val); - let space_layout = Layout::new::(); + let size = mem::size_of_val(val); + let align = mem::align_of_val(val); let mut space = MaybeUninit::>::uninit(); - let (ptr_this, val_dst): (*mut u8, *mut u8) = - if layout.size() <= space_layout.size() && layout.align() <= space_layout.align() { - // Stack. - (INLINE_SENTINEL, space.as_mut_ptr().cast()) - } else if layout.size() == 0 { - // ZST with alignment greater than Space, which will behave like being stored on - // heap but will not actually allocate. - ( - sptr::without_provenance_mut(layout.align()), - sptr::without_provenance_mut(layout.align()), - ) - } else { - // Heap. - let layout = layout - // Safety: MIN_ALIGNMENT is 2, which is a valid power-of-two alignment. - .align_to(MIN_ALIGNMENT) - .unwrap_or_else(|_| unreachable_unchecked()); - let heap_ptr = alloc::alloc(layout); - - if heap_ptr.is_null() { - handle_alloc_error(layout) - } + let (ptr_this, val_dst): (*mut u8, *mut u8) = if size == 0 { + ( + sptr::without_provenance_mut(align), + sptr::without_provenance_mut(align), + ) + } else if size > mem::size_of::() || align > mem::align_of::() { + // Heap + let layout = Layout::for_value::(val); + let heap_ptr = alloc::alloc(layout); + + if heap_ptr.is_null() { + handle_alloc_error(layout) + } - (heap_ptr, heap_ptr) - }; + (heap_ptr, heap_ptr) + } else { + (ptr::null_mut(), space.as_mut_ptr().cast()) + }; // `self.ptr` always holds the metadata, even if stack allocated. let ptr = sptr::with_metadata_of_mut(ptr_this, metadata_ptr); - // Safety: ptr is either INLINE_SENTINEL or returned from the allocator and checked for - // null. - let ptr = NonNull::new_unchecked(ptr); - ptr::copy_nonoverlapping(sptr::from_ref(val).cast(), val_dst, layout.size()); + ptr::copy_nonoverlapping(sptr::from_ref(val).cast(), val_dst, size); SmallBox { space, @@ -254,18 +231,18 @@ impl SmallBox { #[inline] unsafe fn as_ptr(&self) -> *const T { if self.is_heap() { - self.ptr.as_ptr() + self.ptr } else { - sptr::with_metadata_of(self.space.as_ptr(), self.ptr.as_ptr()) + sptr::with_metadata_of(self.space.as_ptr(), self.ptr) } } #[inline] unsafe fn as_mut_ptr(&mut self) -> *mut T { if self.is_heap() { - self.ptr.as_ptr() + self.ptr.cast_mut() } else { - sptr::with_metadata_of_mut(self.space.as_mut_ptr(), self.ptr.as_ptr()) + sptr::with_metadata_of_mut(self.space.as_mut_ptr(), self.ptr.cast_mut()) } } @@ -292,14 +269,9 @@ impl SmallBox { // Just deallocates the heap memory without dropping the boxed value if this.is_heap() && mem::size_of::() != 0 { - // Safety: MIN_ALIGNMENT is 2, aligning to 2 should not create an invalid layout - let layout = unsafe { - Layout::new::() - .align_to(MIN_ALIGNMENT) - .unwrap_or_else(|_| unreachable_unchecked()) - }; + let layout = Layout::new::(); unsafe { - alloc::dealloc(this.ptr.as_ptr().cast::(), layout); + alloc::dealloc(this.ptr.cast_mut().cast::(), layout); } } @@ -328,14 +300,12 @@ impl SmallBox { /// assert_eq!(*small_box, [1, 2, 3, 4]); /// ``` pub fn from_box(boxed: ::alloc::boxed::Box) -> Self { - unsafe { - let ptr = NonNull::new_unchecked(Box::into_raw(boxed)); - let space = MaybeUninit::>::uninit(); - SmallBox { - space, - ptr, - _phantom: PhantomData, - } + let ptr = Box::into_raw(boxed); + let space = MaybeUninit::>::uninit(); + SmallBox { + space, + ptr, + _phantom: PhantomData, } } @@ -461,13 +431,11 @@ impl ops::DerefMut for SmallBox { impl ops::Drop for SmallBox { fn drop(&mut self) { unsafe { - let layout = Layout::for_value::(&*self) - .align_to(MIN_ALIGNMENT) - .unwrap_or_else(|_| unreachable_unchecked()); + let layout = Layout::for_value::(&*self); ptr::drop_in_place::(&mut **self); if self.is_heap() && layout.size() != 0 { - alloc::dealloc(self.ptr.as_ptr().cast::(), layout); + alloc::dealloc(self.ptr.cast_mut().cast::(), layout); } } } @@ -566,7 +534,6 @@ unsafe impl Sync for SmallBox {} #[cfg(test)] mod tests { use core::any::Any; - use core::mem; use core::ptr::addr_of; use ::alloc::boxed::Box; @@ -813,7 +780,7 @@ mod tests { let zst: SmallBox = smallbox!(OveralignedZst); #[allow(clippy::as_conversions)] - let zst_addr = addr_of!(*zst) as usize; + let zst_addr = addr_of!(*zst).addr(); assert_eq!(*zst, OveralignedZst); assert_eq!(zst_addr % 512, 0); } @@ -830,18 +797,10 @@ mod tests { let zst: SmallBox = smallbox!(OveralignedZst); #[allow(clippy::as_conversions)] - let zst_addr = addr_of!(*zst) as *const () as usize; + let zst_addr = addr_of!(*zst).addr(); assert_eq!(zst_addr % 512, 0); } - #[test] - fn test_null_ptr_optimization() { - assert_eq!( - mem::size_of::>(), - mem::size_of::>>() - ); - } - #[test] fn test_box_roundtrip() { // Box -> SmallBox -> Box From cd42e55b1c8f3ffeb3365163992b2e2a993dd374 Mon Sep 17 00:00:00 2001 From: Dillard Robertson Date: Sun, 3 Aug 2025 13:10:04 -0400 Subject: [PATCH 2/4] `ptr.addr()` not stable in rust 1.80, switch back to as cast. --- src/smallbox.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/smallbox.rs b/src/smallbox.rs index 2213e85..f295ad8 100644 --- a/src/smallbox.rs +++ b/src/smallbox.rs @@ -780,7 +780,7 @@ mod tests { let zst: SmallBox = smallbox!(OveralignedZst); #[allow(clippy::as_conversions)] - let zst_addr = addr_of!(*zst).addr(); + let zst_addr = addr_of!(*zst) as usize; assert_eq!(*zst, OveralignedZst); assert_eq!(zst_addr % 512, 0); } @@ -797,7 +797,7 @@ mod tests { let zst: SmallBox = smallbox!(OveralignedZst); #[allow(clippy::as_conversions)] - let zst_addr = addr_of!(*zst).addr(); + let zst_addr = addr_of!(*zst) as *const u8 as usize; assert_eq!(zst_addr % 512, 0); } From 6838f73cce8d620c1f9d54a634a1004ca12d792a Mon Sep 17 00:00:00 2001 From: Dillard Robertson <32615829+dilr@users.noreply.github.com> Date: Wed, 6 Aug 2025 12:09:27 -0400 Subject: [PATCH 3/4] Update src/smallbox.rs Co-authored-by: Andy Lok --- src/smallbox.rs | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/smallbox.rs b/src/smallbox.rs index f295ad8..ca0f56f 100644 --- a/src/smallbox.rs +++ b/src/smallbox.rs @@ -174,12 +174,27 @@ impl SmallBox { let mut space = MaybeUninit::>::uninit(); - let (ptr_this, val_dst): (*mut u8, *mut u8) = if size == 0 { - ( - sptr::without_provenance_mut(align), - sptr::without_provenance_mut(align), - ) - } else if size > mem::size_of::() || align > mem::align_of::() { + let (ptr_this, val_dst): (*mut u8, *mut u8) = + if layout.size() <= space_layout.size() && layout.align() <= space_layout.align() { + // Layout requirement satisfied; store on stack. + (ptr::null_mut(), space.as_mut_ptr().cast()) + } else if layout.size() == 0 { + // ZST with unsatisfied alignment; pretend to store on the heap. + ( + sptr::without_provenance_mut(layout.align()), + sptr::without_provenance_mut(layout.align()), + ) + } else { + // Otherwise, allocate on the heap. + let layout = Layout::for_value::(val); + let heap_ptr = alloc::alloc(layout); + + if heap_ptr.is_null() { + handle_alloc_error(layout) + } + + (heap_ptr, heap_ptr) + }; // Heap let layout = Layout::for_value::(val); let heap_ptr = alloc::alloc(layout); From 84d32cc4b52c3137292618b863950946a236e3b5 Mon Sep 17 00:00:00 2001 From: andylokandy Date: Thu, 7 Aug 2025 20:16:41 +0800 Subject: [PATCH 4/4] fix --- src/smallbox.rs | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/smallbox.rs b/src/smallbox.rs index ca0f56f..2a5be8d 100644 --- a/src/smallbox.rs +++ b/src/smallbox.rs @@ -135,7 +135,7 @@ impl SmallBox { let this = ManuallyDrop::new(self); if this.is_heap() { - // don't change anything if data is already on heap + // Do not change anything if data is already on heap. let space = MaybeUninit::>::uninit(); SmallBox { space, @@ -169,8 +169,8 @@ impl SmallBox { unsafe fn new_copy(val: &U, metadata_ptr: *const T) -> SmallBox where U: ?Sized { - let size = mem::size_of_val(val); - let align = mem::align_of_val(val); + let layout = Layout::for_value(val); + let space_layout = Layout::new::(); let mut space = MaybeUninit::>::uninit(); @@ -195,23 +195,11 @@ impl SmallBox { (heap_ptr, heap_ptr) }; - // Heap - let layout = Layout::for_value::(val); - let heap_ptr = alloc::alloc(layout); - - if heap_ptr.is_null() { - handle_alloc_error(layout) - } - - (heap_ptr, heap_ptr) - } else { - (ptr::null_mut(), space.as_mut_ptr().cast()) - }; // `self.ptr` always holds the metadata, even if stack allocated. let ptr = sptr::with_metadata_of_mut(ptr_this, metadata_ptr); - ptr::copy_nonoverlapping(sptr::from_ref(val).cast(), val_dst, size); + ptr::copy_nonoverlapping(sptr::from_ref(val).cast(), val_dst, layout.size()); SmallBox { space, @@ -293,7 +281,7 @@ impl SmallBox { ret_val } - /// Creates a [`SmallBox`] from a standard [`Box`]. + /// Creates a [`SmallBox`] from a [`Box`]. /// /// The data will always be stored on the heap since it's already allocated there. /// This method transfers ownership from the [`Box`] to the [`SmallBox`] without copying @@ -324,7 +312,7 @@ impl SmallBox { } } - /// Converts a [`SmallBox`] into a standard [`Box`]. + /// Converts a [`SmallBox`] into a [`Box`]. /// /// If the data is stored on the stack, it will be moved to the heap. /// If the data is already on the heap, ownership is transferred without