From 9c63bac045e2369891d08b657a6f4065f51574c7 Mon Sep 17 00:00:00 2001 From: Anders Musikka Date: Wed, 2 Oct 2024 17:35:35 +0200 Subject: [PATCH] Was missing ?Sized-bound on Sync-impl (#7) * Was missing ?Sized-bound on Sync-impl * Add ?Sized-bounds and make ArcShift actually work for unsized payloads --- README.md | 2 +- arcshift/Cargo.toml | 2 +- arcshift/src/lib.rs | 230 +++++++++++++++++++++++++------------------- 3 files changed, 131 insertions(+), 103 deletions(-) diff --git a/README.md b/README.md index e6600a4..dad6bb2 100644 --- a/README.md +++ b/README.md @@ -425,7 +425,7 @@ this line to the cargo mutants-config. Another challenge is how to handle overflow of ArcShift-instances. The problem is that cargo mutants identifies that removing the code for handling ArcShift instance count overflow doesn't -fail the test suite. However, triggering such an overflow would require creating 2*45 instances +fail the test suite. However, triggering such an overflow would require creating 2^45 instances of ArcShift. Unfortunately, this requires at least 280 TB of RAM. In the end, I just added an exception for this logic. Sanity checks which are not expected diff --git a/arcshift/Cargo.toml b/arcshift/Cargo.toml index 8f76ac6..8c06976 100644 --- a/arcshift/Cargo.toml +++ b/arcshift/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "arcshift" -version = "0.1.9" +version = "0.1.10" documentation = "https://docs.rs/arcshift/" homepage = "https://github.com/avl/arcshift/" repository = "https://github.com/avl/arcshift/" diff --git a/arcshift/src/lib.rs b/arcshift/src/lib.rs index 1fba4cc..d26c747 100644 --- a/arcshift/src/lib.rs +++ b/arcshift/src/lib.rs @@ -315,6 +315,31 @@ pub struct ArcShift { item: NonNull>, } +impl Debug for ArcShift +where + T: Debug, +{ + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "ArcShift({:?})", self.shared_get()) + } +} +impl Debug for ArcShiftLight +where + T: Debug, +{ + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "ArcShiftLight(..)") + } +} +impl Debug for ArcShiftCell +where + T: Debug, +{ + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "ArcShiftCell({:?})", &*self.borrow()) + } +} + impl UnwindSafe for ArcShift {} /// ArcShiftLight is like ArcShift, except it does not provide overhead-free access. @@ -344,12 +369,12 @@ pub struct ArcShiftLight { /// SAFETY: /// ArcShiftLight can be Send as long as T is Send. /// ArcShiftLight's mechanisms are compatible with both Send and Sync -unsafe impl Send for ArcShiftLight where T: Send {} +unsafe impl Send for ArcShiftLight where T: Send {} /// SAFETY: /// ArcShiftLight can be Sync as long as T is Sync. /// ArcShiftLight's mechanisms are compatible with both Send and Sync -unsafe impl Sync for ArcShiftLight where T: Sync {} +unsafe impl Sync for ArcShiftLight where T: Sync {} /// ArcShiftCell is like an ArcShift, except that it can be reloaded /// without requiring 'mut'-access. @@ -361,7 +386,7 @@ unsafe impl Sync for ArcShiftLight where T: Sync {} /// but if it is leaked, the effect is that whatever value the ArcShiftCell-instance /// pointed to at that time, will forever leak also. All the linked-list nodes from /// that entry and onward will also leak. So make sure to not leak the handle! -pub struct ArcShiftCell { +pub struct ArcShiftCell { inner: UnsafeCell>, recursion: Cell, } @@ -372,11 +397,11 @@ pub struct ArcShiftCell { /// that entry and onward will also leak. So make sure to not leak the handle! /// /// Leaking the handle does not cause unsoundness and is not UB. -pub struct ArcShiftCellHandle<'a, T: 'static> { +pub struct ArcShiftCellHandle<'a, T: 'static + ?Sized> { cell: &'a ArcShiftCell, } -impl<'a, T: 'static> Drop for ArcShiftCellHandle<'a, T> { +impl<'a, T: 'static + ?Sized> Drop for ArcShiftCellHandle<'a, T> { fn drop(&mut self) { let mut rec = self.cell.recursion.get(); rec -= 1; @@ -390,7 +415,7 @@ impl<'a, T: 'static> Drop for ArcShiftCellHandle<'a, T> { } } -impl<'a, T: 'static> Deref for ArcShiftCellHandle<'a, T> { +impl<'a, T: 'static + ?Sized> Deref for ArcShiftCellHandle<'a, T> { type Target = T; fn deref(&self) -> &Self::Target { @@ -422,7 +447,7 @@ impl<'a, T: 'static> Deref for ArcShiftCellHandle<'a, T> { /// could call 'get' simultaneously, corrupting the (non-atomic) refcount. unsafe impl Send for ArcShiftCell where T: Send {} -impl Clone for ArcShiftCell { +impl Clone for ArcShiftCell { fn clone(&self) -> Self { // SAFETY: // Accessing the inner value is safe, since we know no other thread @@ -459,7 +484,8 @@ impl ArcShiftCell { pub fn new(value: T) -> ArcShiftCell { ArcShiftCell::from_arcshift(ArcShift::new(value)) } - +} +impl ArcShiftCell { /// Creates an ArcShiftCell from an ArcShift-instance. /// The payload is not cloned, the two pointers keep pointing to the same object. pub fn from_arcshift(input: ArcShift) -> ArcShiftCell { @@ -550,7 +576,7 @@ const fn is_sized() -> bool { size_of::<&T>() == size_of::<&()>() } -impl Clone for ArcShiftLight { +impl Clone for ArcShiftLight { fn clone(&self) -> Self { let mut curitem = self.item.as_ptr() as *const _; loop { @@ -956,6 +982,91 @@ impl ArcShiftLight { Some(next) } } + + /// Create an ArcShift instance from this ArcShiftLight. + pub fn upgrade(&self) -> ArcShift { + debug_println!("ArcShiftLight Promoting to ArcShift {:?}", self.item); + let mut curitem = self.item.as_ptr(); + loop { + let Some(next) = Self::load_nontentative_next(curitem) else { + atomic::spin_loop(); + continue; + }; + + debug_println!( + "ArcShiftLight upgrade {:?}, next: {:?} = {:?}", + curitem, + next, + get_state(next) + ); + + if !next.is_null() { + debug_println!( + "ArcShiftLight traversing chain to {:?} -> {:?}", + curitem, + next + ); + curitem = undecorate(next) as *mut _; + atomic::spin_loop(); + continue; + } + + let precount = get_refcount(curitem).fetch_add(MAX_ROOTS, Ordering::SeqCst); + if precount >= MAX_ARCSHIFT { + let _precount = get_refcount(curitem).fetch_sub(MAX_ROOTS, Ordering::SeqCst); + panic!( + "Maximum supported ArcShift instance count reached: {}", + MAX_ARCSHIFT + ); + } + atomic::fence(Ordering::SeqCst); //Just to make loom work + debug_println!( + "Promote {:?}, prev count: {}, new count {}", + curitem, + precount, + precount + MAX_ROOTS + ); + assert!(precount >= 1); + let Some(next) = Self::load_nontentative_next(curitem) else { + let _precount = get_refcount(curitem).fetch_sub(MAX_ROOTS, Ordering::SeqCst); + assert!(_precount > MAX_ROOTS && _precount < 1_000_000_000_000); + atomic::spin_loop(); + continue; + }; + if !undecorate(next).is_null() { + debug_println!( + "ArcShiftLight About to reduce count {:?} by {}, and traversing to {:?}", + curitem, + MAX_ROOTS, + next + ); + + let _precount = get_refcount(curitem).fetch_sub(MAX_ROOTS, Ordering::SeqCst); + assert!(_precount > MAX_ROOTS && _precount < 1_000_000_000_000); + curitem = undecorate(next) as *mut _; + atomic::spin_loop(); + continue; + } + + let mut temp = ArcShift { item: + // SAFETY: + // curitem cannot be null, since it is either the value of 'self.item', which + // cannot be null, or assigned by code above that first checks for null + unsafe { NonNull::new_unchecked(curitem) } + }; + temp.reload(); + return temp; + } + } + #[cfg_attr(test, mutants::skip)] + fn verify_count(count: usize) { + if (count & (MAX_ROOTS - 1)) >= MAX_ROOTS - 1 { + panic!( + "Max limit of ArcShiftLight clones ({}) was reached", + MAX_ROOTS + ); + } + } } impl ArcShiftLight { @@ -1048,16 +1159,6 @@ impl ArcShiftLight { } } - #[cfg_attr(test, mutants::skip)] - fn verify_count(count: usize) { - if (count & (MAX_ROOTS - 1)) >= MAX_ROOTS - 1 { - panic!( - "Max limit of ArcShiftLight clones ({}) was reached", - MAX_ROOTS - ); - } - } - /// Update the contents of this ArcShift, and all other instances cloned from this /// instance. The next time such an instance of ArcShift is dereferenced, this /// new value will be returned. @@ -1186,82 +1287,6 @@ impl ArcShiftLight { let new_ptr = ArcShift::from_box_impl(new_payload); ArcShift::update_shared_impl(self.item.as_ptr(), new_ptr as *mut _, true, null()); } - - /// Create an ArcShift instance from this ArcShiftLight. - pub fn upgrade(&self) -> ArcShift { - debug_println!("ArcShiftLight Promoting to ArcShift {:?}", self.item); - let mut curitem = self.item.as_ptr(); - loop { - let Some(next) = Self::load_nontentative_next(curitem) else { - atomic::spin_loop(); - continue; - }; - - debug_println!( - "ArcShiftLight upgrade {:?}, next: {:?} = {:?}", - curitem, - next, - get_state(next) - ); - - if !next.is_null() { - debug_println!( - "ArcShiftLight traversing chain to {:?} -> {:?}", - curitem, - next - ); - curitem = undecorate(next) as *mut _; - atomic::spin_loop(); - continue; - } - - let precount = get_refcount(curitem).fetch_add(MAX_ROOTS, Ordering::SeqCst); - if precount >= MAX_ARCSHIFT { - let _precount = get_refcount(curitem).fetch_sub(MAX_ROOTS, Ordering::SeqCst); - panic!( - "Maximum supported ArcShift instance count reached: {}", - MAX_ARCSHIFT - ); - } - atomic::fence(Ordering::SeqCst); //Just to make loom work - debug_println!( - "Promote {:?}, prev count: {}, new count {}", - curitem, - precount, - precount + MAX_ROOTS - ); - assert!(precount >= 1); - let Some(next) = Self::load_nontentative_next(curitem) else { - let _precount = get_refcount(curitem).fetch_sub(MAX_ROOTS, Ordering::SeqCst); - assert!(_precount > MAX_ROOTS && _precount < 1_000_000_000_000); - atomic::spin_loop(); - continue; - }; - if !undecorate(next).is_null() { - debug_println!( - "ArcShiftLight About to reduce count {:?} by {}, and traversing to {:?}", - curitem, - MAX_ROOTS, - next - ); - - let _precount = get_refcount(curitem).fetch_sub(MAX_ROOTS, Ordering::SeqCst); - assert!(_precount > MAX_ROOTS && _precount < 1_000_000_000_000); - curitem = undecorate(next) as *mut _; - atomic::spin_loop(); - continue; - } - - let mut temp = ArcShift { item: - // SAFETY: - // curitem cannot be null, since it is either the value of 'self.item', which - // cannot be null, or assigned by code above that first checks for null - unsafe { NonNull::new_unchecked(curitem) } - }; - temp.reload(); - return temp; - } - } } impl Drop for ArcShiftLight { @@ -1273,17 +1298,20 @@ impl Drop for ArcShiftLight { /// SAFETY: /// If `T` is `Sync`, `ArcShift` can also be `Sync` -unsafe impl Sync for ArcShift {} +unsafe impl Sync for ArcShift {} /// SAFETY: /// If `T` is `Send`, `ArcShift` can also be `Send` -unsafe impl Send for ArcShift {} +unsafe impl Send for ArcShift {} impl Drop for ArcShift { fn drop(&mut self) { verify_item(get_ptr(self.item)); - self.reload(); + if !undecorate(get_next_and_state(self.item.as_ptr()).load(Ordering::Relaxed)).is_null() { + //Fast-path, when 'next' is null, don't do the expensive and complicated 'reload' + self.reload(); + } debug_println!("ArcShift::drop({:?}) - reloaded", self.item); drop_item(get_ptr(self.item)); debug_println!("ArcShift::drop({:?}) DONE", self.item); @@ -1539,7 +1567,7 @@ fn is_superseded_by_tentative(state: Option) -> bool { matches!(state, Some(ItemStateEnum::SupersededByTentative)) } -impl Clone for ArcShift { +impl Clone for ArcShift { fn clone(&self) -> Self { debug_println!("ArcShift::clone({:?})", self.item); let rescount = @@ -1557,7 +1585,7 @@ impl Clone for ArcShift { ArcShift { item: self.item } } } -impl Deref for ArcShift { +impl Deref for ArcShift { type Target = T; fn deref(&self) -> &Self::Target { @@ -1874,7 +1902,7 @@ impl ArcShift { } debug_println!("drop_impl to reduce count {:?} by {}", self_item, strength); - let count = get_refcount(self_item).fetch_sub(strength, atomic::Ordering::SeqCst); + let count = get_refcount(self_item).fetch_sub(strength, atomic::Ordering::AcqRel); debug_println!( "drop_impl on {:?} - count: {} -> {}", self_item,