From e1de5ab10db70b834be39f51eb27a11ba19b6e96 Mon Sep 17 00:00:00 2001 From: fren_gor Date: Tue, 30 Jul 2024 20:11:40 +0200 Subject: [PATCH 01/13] Remove useless `?Sized` bound --- src/cc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cc.rs b/src/cc.rs index 3461350..1f2d75c 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -814,7 +814,7 @@ impl InternalTrace for CcBox { // # Cc Trait impls # // #################################### -impl Default for Cc { +impl Default for Cc { /// Creates a new [`Cc`][`Cc`], with the [`Default`] value for `T`. /// /// # Collection From 43093ea2fbfe5e4eab3dbd3576ade42e223fd254 Mon Sep 17 00:00:00 2001 From: fren_gor Date: Tue, 30 Jul 2024 01:00:58 +0200 Subject: [PATCH 02/13] Rename list.rs to lists.rs --- src/cc.rs | 2 +- src/config.rs | 2 +- src/lib.rs | 4 ++-- src/{list.rs => lists.rs} | 0 src/tests/list.rs | 2 +- src/tests/mod.rs | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) rename src/{list.rs => lists.rs} (100%) diff --git a/src/cc.rs b/src/cc.rs index 1f2d75c..797899e 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -21,7 +21,7 @@ use core::{ use crate::counter_marker::{CounterMarker, Mark}; use crate::state::{replace_state_field, state, State, try_state}; use crate::trace::{Context, ContextInner, Finalize, Trace}; -use crate::list::ListMethods; +use crate::lists::ListMethods; use crate::utils::*; use crate::POSSIBLE_CYCLES; #[cfg(feature = "weak-ptrs")] diff --git a/src/config.rs b/src/config.rs index 8ff058f..043bdab 100644 --- a/src/config.rs +++ b/src/config.rs @@ -26,7 +26,7 @@ use core::num::NonZeroUsize; use core::marker::PhantomData; use thiserror::Error; -use crate::list::CountedList; +use crate::lists::CountedList; use crate::state::State; use crate::utils; diff --git a/src/lib.rs b/src/lib.rs index 67221c6..47f9b5d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -152,7 +152,7 @@ use core::ops::{Deref, DerefMut}; use crate::cc::CcBox; use crate::counter_marker::Mark; -use crate::list::*; +use crate::lists::*; use crate::state::{replace_state_field, State, try_state}; use crate::trace::ContextInner; use crate::utils::*; @@ -162,7 +162,7 @@ mod tests; mod cc; mod counter_marker; -mod list; +mod lists; pub mod state; mod trace; mod utils; diff --git a/src/list.rs b/src/lists.rs similarity index 100% rename from src/list.rs rename to src/lists.rs diff --git a/src/tests/list.rs b/src/tests/list.rs index e605474..6ef1d30 100644 --- a/src/tests/list.rs +++ b/src/tests/list.rs @@ -4,7 +4,7 @@ use std::panic::{AssertUnwindSafe, catch_unwind}; use std::ptr::NonNull; use crate::{CcBox, Mark}; -use crate::list::*; +use crate::lists::*; use crate::counter_marker::CounterMarker; use test_case::{test_case, test_matrix}; diff --git a/src/tests/mod.rs b/src/tests/mod.rs index f7d4f4c..ebc8da6 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -5,7 +5,7 @@ use std::cell::Cell; use std::ops::{Deref, DerefMut}; use crate::trace::Trace; -use crate::list::*; +use crate::lists::*; use crate::{state, Cc, Context, Finalize, POSSIBLE_CYCLES}; use crate::state::state; From 3a7b5dcabe67bbfad1242a7dc12051aed0da59e1 Mon Sep 17 00:00:00 2001 From: fren_gor Date: Tue, 30 Jul 2024 19:16:08 +0200 Subject: [PATCH 03/13] Rework lists Now POSSIBLE_CYCLES doesn't need a RefCell anymore --- src/cc.rs | 19 +- src/config.rs | 9 +- src/lib.rs | 63 +++---- src/lists.rs | 309 +++++++++++++++++--------------- src/state.rs | 7 +- src/tests/cc.rs | 4 +- src/tests/{list.rs => lists.rs} | 218 +++++++++++++++++----- src/tests/mod.rs | 12 +- src/tests/weak/mod.rs | 3 +- src/trace.rs | 10 +- 10 files changed, 391 insertions(+), 263 deletions(-) rename src/tests/{list.rs => lists.rs} (59%) diff --git a/src/cc.rs b/src/cc.rs index 797899e..be239f1 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -21,7 +21,6 @@ use core::{ use crate::counter_marker::{CounterMarker, Mark}; use crate::state::{replace_state_field, state, State, try_state}; use crate::trace::{Context, ContextInner, Finalize, Trace}; -use crate::lists::ListMethods; use crate::utils::*; use crate::POSSIBLE_CYCLES; #[cfg(feature = "weak-ptrs")] @@ -507,13 +506,12 @@ pub(crate) fn remove_from_list(ptr: NonNull>) { if counter_marker.is_in_possible_cycles() { // ptr is in the list, remove it let _ = POSSIBLE_CYCLES.try_with(|pc| { - let mut list = pc.borrow_mut(); // Confirm is_in_possible_cycles() in debug builds #[cfg(feature = "pedantic-debug-assertions")] - debug_assert!(list.contains(ptr)); + debug_assert!(pc.contains(ptr)); counter_marker.mark(Mark::NonMarked); - list.remove(ptr); + pc.remove(ptr); }); } else { // ptr is not in the list @@ -523,7 +521,7 @@ pub(crate) fn remove_from_list(ptr: NonNull>) { #[cfg(feature = "pedantic-debug-assertions")] debug_assert! { POSSIBLE_CYCLES.try_with(|pc| { - !pc.borrow().contains(ptr) + !pc.contains(ptr) }).unwrap_or(true) }; } @@ -534,20 +532,19 @@ pub(crate) fn add_to_list(ptr: NonNull>) { let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); let _ = POSSIBLE_CYCLES.try_with(|pc| { - let mut list = pc.borrow_mut(); - // Check if ptr is in possible_cycles list since we have to move it at its start if counter_marker.is_in_possible_cycles() { // Confirm is_in_possible_cycles() in debug builds #[cfg(feature = "pedantic-debug-assertions")] - debug_assert!(list.contains(ptr)); + debug_assert!(pc.contains(ptr)); - list.remove(ptr); + pc.remove(ptr); // In this case we don't need to update the mark since we put it back into the list } else { // Confirm !is_in_possible_cycles() in debug builds #[cfg(feature = "pedantic-debug-assertions")] - debug_assert!(!list.contains(ptr)); + debug_assert!(!pc.contains(ptr)); + debug_assert!(counter_marker.is_not_marked()); // Mark it @@ -557,7 +554,7 @@ pub(crate) fn add_to_list(ptr: NonNull>) { // // Make sure this operation is the first after the if-else, since the CcBox is in // an invalid state now (it's marked Mark::PossibleCycles, but it isn't into the list) - list.add(ptr); + pc.add(ptr); }); } diff --git a/src/config.rs b/src/config.rs index 043bdab..0b1b104 100644 --- a/src/config.rs +++ b/src/config.rs @@ -26,8 +26,7 @@ use core::num::NonZeroUsize; use core::marker::PhantomData; use thiserror::Error; -use crate::lists::CountedList; - +use crate::lists::PossibleCycles; use crate::state::State; use crate::utils; @@ -159,7 +158,7 @@ impl Config { } #[inline(always)] - pub(super) fn should_collect(&mut self, state: &State, possible_cycles: &RefCell) -> bool { + pub(super) fn should_collect(&mut self, state: &State, possible_cycles: &PossibleCycles) -> bool { if !self.auto_collect { return false; } @@ -168,8 +167,8 @@ impl Config { return true; } - return if let Some(buffered_threshold) = self.buffered_threshold { - possible_cycles.try_borrow().map_or(false, |pc| pc.size() > buffered_threshold.get()) + if let Some(buffered_threshold) = self.buffered_threshold { + possible_cycles.size() > buffered_threshold.get() } else { false } diff --git a/src/lib.rs b/src/lib.rs index 47f9b5d..9da9c9d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -144,7 +144,6 @@ compile_error!("Feature \"std\" cannot be disabled without enabling feature \"ni extern crate alloc; -use core::cell::RefCell; use core::mem; use core::mem::ManuallyDrop; use core::ptr::NonNull; @@ -186,7 +185,7 @@ pub use cc::Cc; pub use trace::{Context, Finalize, Trace}; rust_cc_thread_local! { - pub(crate) static POSSIBLE_CYCLES: RefCell = RefCell::new(CountedList::new()); + pub(crate) static POSSIBLE_CYCLES: PossibleCycles = PossibleCycles::new(); } /// Immediately executes the cycle collection algorithm and collects garbage cycles. @@ -228,7 +227,7 @@ fn adjust_trigger_point(state: &State) { let _ = config::config(|config| config.adjust(state)); } -fn collect(state: &State, possible_cycles: &RefCell) { +fn collect(state: &State, possible_cycles: &PossibleCycles) { state.set_collecting(true); state.increment_executions_count(); @@ -264,26 +263,26 @@ fn collect(state: &State, possible_cycles: &RefCell) { // Thus, it is fine to just leave the remaining objects into POSSIBLE_CYCLES for the // next collection execution. The program has already been stopped for too much time. - if is_empty(possible_cycles) { + if possible_cycles.is_empty() { break; } __collect(state, possible_cycles); } #[cfg(not(feature = "finalization"))] - if !is_empty(possible_cycles) { + if !possible_cycles.is_empty() { __collect(state, possible_cycles); } // _drop_guard is dropped here, setting state.collecting to false } -fn __collect(state: &State, possible_cycles: &RefCell) { - let mut non_root_list = List::new(); +fn __collect(state: &State, possible_cycles: &PossibleCycles) { + let mut non_root_list = LinkedList::new(); { - let mut root_list = List::new(); + let mut root_list = LinkedList::new(); - while let Some(ptr) = get_and_remove_first(possible_cycles) { + while let Some(ptr) = possible_cycles.remove_first() { // remove_first already marks ptr as NonMarked trace_counting(ptr, &mut root_list, &mut non_root_list); } @@ -322,29 +321,25 @@ fn __collect(state: &State, possible_cycles: &RefCell) { deallocate_list(non_root_list, state); } else { // Put CcBoxes back into the possible cycles list. They will be re-processed in the - // next iteration of the loop, which will automatically check for resurrected objects - // using the same algorithm of the initial tracing. This makes it more difficult to - // create memory leaks accidentally using finalizers than in the previous implementation. - let mut pc = possible_cycles.borrow_mut(); + // next iteration of the loop, which will automatically check for resurrected objects. - // pc is already marked PossibleCycles, while non_root_list is not. - // non_root_list have to be added to pc after having been marked. - // It's good here to instead swap the two, mark the pc list (was non_root_list before) and then - // append the other to it in O(1), since we already know the last element of pc from the marking. - // This avoids iterating unnecessarily both lists and the need to update many pointers. + let old_size = possible_cycles.size(); - let old_size = pc.size(); + // possible_cycles is already marked PossibleCycles, while non_root_list is not. + // non_root_list have to be added to possible_cycles after having been marked. + // It's good here to instead swap the two, mark the possible_cycles list (was non_root_list before) + // and then append the other to it in O(1), since we already know the last element of pc from the + // marking. This avoids iterating unnecessarily both lists and the need to update many pointers. - // SAFETY: non_root_list_size is calculated before and it's the size of non_root_list + // SAFETY: non_root_list_size was calculated before and it's the size of non_root_list unsafe { - pc.swap_list(&mut non_root_list, non_root_list_size); + possible_cycles.swap_list(&mut non_root_list, non_root_list_size); } // SAFETY: swap_list swapped pc and non_root_list, so every element inside non_root_list is already // marked PossibleCycles (because it was pc) and now old_size is the size of non_root_list unsafe { - pc.mark_self_and_append(Mark::PossibleCycles, non_root_list, old_size); + possible_cycles.mark_self_and_append(Mark::PossibleCycles, non_root_list, old_size); } - drop(pc); // Useless, but better be explicit here in case more code is added below this line } } @@ -356,25 +351,15 @@ fn __collect(state: &State, possible_cycles: &RefCell) { } #[inline] -fn is_empty(list: &RefCell) -> bool { - list.borrow().is_empty() -} - -#[inline] -fn get_and_remove_first(list: &RefCell) -> Option>> { - list.borrow_mut().remove_first() -} - -#[inline] -fn deallocate_list(to_deallocate_list: List, state: &State) { +fn deallocate_list(to_deallocate_list: LinkedList, state: &State) { /// Just a wrapper used to handle the dropping of to_deallocate_list. /// When dropped, the objects inside are set as dropped struct ToDropList { - list: ManuallyDrop, + list: ManuallyDrop, } impl Deref for ToDropList { - type Target = List; + type Target = LinkedList; #[inline(always)] fn deref(&self) -> &Self::Target { @@ -455,8 +440,8 @@ fn deallocate_list(to_deallocate_list: List, state: &State) { fn trace_counting( ptr: NonNull>, - root_list: &mut List, - non_root_list: &mut List, + root_list: &mut LinkedList, + non_root_list: &mut LinkedList, ) { let mut ctx = Context::new(ContextInner::Counting { root_list, @@ -466,7 +451,7 @@ fn trace_counting( CcBox::start_tracing(ptr, &mut ctx); } -fn trace_roots(mut root_list: List, non_root_list: &mut List) { +fn trace_roots(mut root_list: LinkedList, non_root_list: &mut LinkedList) { while let Some(ptr) = root_list.remove_first() { let mut ctx = Context::new(ContextInner::RootTracing { non_root_list, root_list: &mut root_list }); CcBox::start_tracing(ptr, &mut ctx); diff --git a/src/lists.rs b/src/lists.rs index 0891a2f..60ff97d 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -1,71 +1,40 @@ use core::marker::PhantomData; use core::ptr::NonNull; +use core::cell::Cell; use crate::{CcBox, Mark}; -/// Methods shared by lists -pub(crate) trait ListMethods: Sized { - #[cfg(all(test, feature = "std"))] // Only used in unit tests - fn first(&self) -> Option>>; - - fn add(&mut self, ptr: NonNull>); - - fn remove(&mut self, ptr: NonNull>); - - fn remove_first(&mut self) -> Option>>; - - fn is_empty(&self) -> bool; - - #[inline] - #[cfg(any(feature = "pedantic-debug-assertions", all(test, feature = "std")))] // Only used in pedantic-debug-assertions or unit tests - fn contains(&self, ptr: NonNull>) -> bool { - self.iter().any(|elem| elem == ptr) - } - - fn iter(&self) -> Iter; - - #[cfg(all(test, feature = "std"))] // Only used in unit tests - fn into_iter(self) -> ListIter; -} - -pub(crate) struct List { +pub(crate) struct LinkedList { first: Option>>, } -impl List { +impl LinkedList { #[inline] - pub(crate) const fn new() -> List { - List { first: None } + pub(crate) const fn new() -> Self { + Self { first: None } } -} -impl ListMethods for List { #[inline] - #[cfg(all(test, feature = "std"))] // Only used in unit tests - fn first(&self) -> Option>> { + pub(crate) fn first(&self) -> Option>> { self.first } #[inline] - fn add(&mut self, ptr: NonNull>) { - if let Some(first) = &mut self.first { + pub(crate) fn add(&mut self, ptr: NonNull>) { + debug_assert_nones(ptr); + + if let Some(first) = self.first { unsafe { *first.as_ref().get_prev() = Some(ptr); - *ptr.as_ref().get_next() = Some(*first); - debug_assert!((*ptr.as_ref().get_prev()).is_none()); - } - *first = ptr; - } else { - self.first = Some(ptr); - unsafe { - debug_assert!((*ptr.as_ref().get_next()).is_none()); - debug_assert!((*ptr.as_ref().get_prev()).is_none()); + *ptr.as_ref().get_next() = Some(first); } } + + self.first = Some(ptr); } #[inline] - fn remove(&mut self, ptr: NonNull>) { + pub(crate) fn remove(&mut self, ptr: NonNull>) { unsafe { match (*ptr.as_ref().get_next(), *ptr.as_ref().get_prev()) { (Some(next), Some(prev)) => { @@ -97,13 +66,12 @@ impl ListMethods for List { self.first = None; }, } - debug_assert!((*ptr.as_ref().get_next()).is_none()); - debug_assert!((*ptr.as_ref().get_prev()).is_none()); + debug_assert_nones(ptr); } } #[inline] - fn remove_first(&mut self) -> Option>> { + pub(crate) fn remove_first(&mut self) -> Option>> { match self.first { Some(first) => unsafe { self.first = *first.as_ref().get_next(); @@ -125,33 +93,27 @@ impl ListMethods for List { } #[inline] - fn is_empty(&self) -> bool { - self.first.is_none() + pub(crate) fn is_empty(&self) -> bool { + self.first().is_none() } #[inline] - fn iter(&self) -> Iter { + pub(crate) fn iter(&self) -> Iter { self.into_iter() } - - #[inline] - #[cfg(all(test, feature = "std"))] // Only used in unit tests - fn into_iter(self) -> ListIter { - ::into_iter(self) - } } -impl Drop for List { +impl Drop for LinkedList { #[inline] fn drop(&mut self) { // Remove the remaining elements from the list while self.remove_first().is_some() { - // remove_first already mark every removed element NonMarked + // remove_first already marks every removed element NonMarked } } } -impl<'a> IntoIterator for &'a List { +impl<'a> IntoIterator for &'a LinkedList { type Item = NonNull>; type IntoIter = Iter<'a>; @@ -164,9 +126,9 @@ impl<'a> IntoIterator for &'a List { } } -impl IntoIterator for List { +impl IntoIterator for LinkedList { type Item = NonNull>; - type IntoIter = ListIter; + type IntoIter = ListIter; #[inline] fn into_iter(self) -> Self::IntoIter { @@ -200,11 +162,11 @@ impl<'a> Iterator for Iter<'a> { } } -pub(crate) struct ListIter { - list: T, +pub(crate) struct ListIter { + list: LinkedList, } -impl Iterator for ListIter { +impl Iterator for ListIter { type Item = NonNull>; #[inline] @@ -213,26 +175,125 @@ impl Iterator for ListIter { } } -/// A [`List`] which keeps track of its size. Used in [`POSSIBLE_CYCLES`]. -/// -/// [`POSSIBLE_CYCLES`]: crate::POSSIBLE_CYCLES -pub(crate) struct CountedList { - list: List, - size: usize, +pub(crate) struct PossibleCycles { + first: Cell>>>, + size: Cell, } -impl CountedList { +impl PossibleCycles { #[inline] - pub(crate) const fn new() -> CountedList { - CountedList { - list: List::new(), - size: 0, + pub(crate) const fn new() -> Self { + Self { + first: Cell::new(None), + size: Cell::new(0), } } + #[inline] + #[cfg(all(test, feature = "std"))] // Only used in unit tests + pub(crate) fn reset(&self) { + self.first.set(None); + self.size.set(0); + } + #[inline] pub(crate) fn size(&self) -> usize { - self.size + self.size.get() + } + + #[inline] + pub(crate) fn first(&self) -> Option>> { + self.first.get() + } + + #[inline] + pub(crate) fn add(&self, ptr: NonNull>) { + debug_assert_nones(ptr); + + self.size.set(self.size.get() + 1); + + if let Some(first) = self.first.get() { + unsafe { + *first.as_ref().get_prev() = Some(ptr); + *ptr.as_ref().get_next() = Some(first); + } + } + + self.first.set(Some(ptr)); + } + + #[inline] + pub(crate) fn remove(&self, ptr: NonNull>) { + self.size.set(self.size.get() - 1); + + unsafe { + match (*ptr.as_ref().get_next(), *ptr.as_ref().get_prev()) { + (Some(next), Some(prev)) => { + // ptr is in between two elements + *next.as_ref().get_prev() = Some(prev); + *prev.as_ref().get_next() = Some(next); + + // Both next and prev are != None + *ptr.as_ref().get_next() = None; + *ptr.as_ref().get_prev() = None; + }, + (Some(next), None) => { + // ptr is the first element + *next.as_ref().get_prev() = None; + self.first.set(Some(next)); + + // Only next is != None + *ptr.as_ref().get_next() = None; + }, + (None, Some(prev)) => { + // ptr is the last element + *prev.as_ref().get_next() = None; + + // Only prev is != None + *ptr.as_ref().get_prev() = None; + }, + (None, None) => { + // ptr is the only one in the list + self.first.set(None); + }, + } + debug_assert_nones(ptr); + } + } + + #[inline] + pub(crate) fn remove_first(&self) -> Option>> { + match self.first.get() { + Some(first) => unsafe { + self.size.set(self.size.get() - 1); + let new_first = *first.as_ref().get_next(); + self.first.set(new_first); + if let Some(next) = new_first { + *next.as_ref().get_prev() = None; + } + *first.as_ref().get_next() = None; + // prev is already None since it's the first element + + // Make sure the mark is correct + first.as_ref().counter_marker().mark(Mark::NonMarked); + + Some(first) + }, + None => { + None + }, + } + } + + #[inline] + pub(crate) fn is_empty(&self) -> bool { + self.first().is_none() + } + + #[inline] + #[cfg(any(feature = "pedantic-debug-assertions", all(test, feature = "std")))] // Only used in pedantic-debug-assertions or unit tests + pub(crate) fn contains(&self, ptr: NonNull>) -> bool { + self.iter().any(|elem| elem == ptr) } /// # Safety @@ -240,9 +301,9 @@ impl CountedList { /// * `to_append_size` must be the size of `to_append` #[inline] #[cfg(feature = "finalization")] - pub(crate) unsafe fn mark_self_and_append(&mut self, mark: Mark, to_append: List, to_append_size: usize) { - if let Some(mut prev) = self.list.first { - for elem in self.list.iter() { + pub(crate) unsafe fn mark_self_and_append(&self, mark: Mark, to_append: LinkedList, to_append_size: usize) { + if let Some(mut prev) = self.first.get() { + for elem in self.iter() { unsafe { elem.as_ref().counter_marker().mark(mark); } @@ -255,10 +316,10 @@ impl CountedList { } } } else { - self.list.first = to_append.first; + self.first.set(to_append.first); // to_append.first.prev is already None } - self.size += to_append_size; + self.size.set(self.size.get() + to_append_size); core::mem::forget(to_append); // Don't run to_append destructor } @@ -266,81 +327,49 @@ impl CountedList { /// `to_swap_size` must be the size of `to_swap`. #[inline] #[cfg(feature = "finalization")] - pub(crate) unsafe fn swap_list(&mut self, to_swap: &mut List, to_swap_size: usize) { - self.size = to_swap_size; - core::mem::swap(&mut self.list, to_swap); - } -} - -impl ListMethods for CountedList { - #[inline] - #[cfg(all(test, feature = "std"))] // Only used in unit tests - fn first(&self) -> Option>> { - self.list.first() + pub(crate) unsafe fn swap_list(&self, to_swap: &mut LinkedList, to_swap_size: usize) { + self.size.set(to_swap_size); + to_swap.first = self.first.replace(to_swap.first); } #[inline] - fn add(&mut self, ptr: NonNull>) { - self.size += 1; - self.list.add(ptr) - } - - #[inline] - fn remove(&mut self, ptr: NonNull>) { - self.size -= 1; - self.list.remove(ptr) + #[cfg(any( + feature = "pedantic-debug-assertions", + feature = "finalization", + all(test, feature = "std") // Unit tests + ))] + pub(crate) fn iter(&self) -> Iter { + self.into_iter() } +} +impl Drop for PossibleCycles { #[inline] - fn remove_first(&mut self) -> Option>> { - let ptr = self.list.remove_first(); - if ptr.is_some() { - self.size -= 1; + fn drop(&mut self) { + // Remove the remaining elements from the list + while self.remove_first().is_some() { + // remove_first already marks every removed element NonMarked } - ptr - } - - #[inline] - fn is_empty(&self) -> bool { - self.size == 0 - } - - #[inline] - #[cfg(any(feature = "pedantic-debug-assertions", all(test, feature = "std")))] // Only used in pedantic-debug-assertions or unit tests - fn contains(&self, ptr: NonNull>) -> bool { - self.list.contains(ptr) - } - - #[inline] - fn iter(&self) -> Iter { - self.list.iter() - } - - #[inline] - #[cfg(all(test, feature = "std"))] // Only used in unit tests - fn into_iter(self) -> ListIter { - ::into_iter(self) } } -impl<'a> IntoIterator for &'a CountedList { +impl<'a> IntoIterator for &'a PossibleCycles { type Item = NonNull>; type IntoIter = Iter<'a>; #[inline] fn into_iter(self) -> Self::IntoIter { - self.list.iter() + Iter { + next: self.first.get(), + _phantom: PhantomData, + } } } -impl IntoIterator for CountedList { - type Item = NonNull>; - type IntoIter = ListIter; - - #[inline] - fn into_iter(self) -> Self::IntoIter { - ListIter { - list: self, - } +#[inline(always)] // The fn is always empty in release mode +fn debug_assert_nones(ptr: NonNull>) { + unsafe { + debug_assert!((*ptr.as_ref().get_next()).is_none()); + debug_assert!((*ptr.as_ref().get_prev()).is_none()); } } diff --git a/src/state.rs b/src/state.rs index 4ff2add..858d2f3 100644 --- a/src/state.rs +++ b/src/state.rs @@ -161,12 +161,7 @@ impl Default for State { pub fn buffered_objects_count() -> Result { // Expose this in state module even though the count is kept inside POSSIBLE_CYCLES // The error returned in case of failed access is a generic StateAccessError::AccessError - crate::POSSIBLE_CYCLES.try_with(|pc| { - match pc.try_borrow() { - Ok(pc) => Ok(pc.size()), - Err(_) => Err(StateAccessError::AccessError), - } - }).unwrap_or(Err(StateAccessError::AccessError)) + crate::POSSIBLE_CYCLES.try_with(|pc| Ok(pc.size())).unwrap_or(Err(StateAccessError::AccessError)) } /// Returns the number of allocated bytes managed by the garbage collector. diff --git a/src/tests/cc.rs b/src/tests/cc.rs index 02ef597..0a63f07 100644 --- a/src/tests/cc.rs +++ b/src/tests/cc.rs @@ -191,8 +191,8 @@ fn test_trait_object() { let inner = cc.deref(); inner.hello(); - let mut l1 = List::new(); - let mut l2 = List::new(); + let mut l1 = LinkedList::new(); + let mut l2 = LinkedList::new(); cc.trace(&mut Context::new(ContextInner::Counting { root_list: &mut l1, diff --git a/src/tests/list.rs b/src/tests/lists.rs similarity index 59% rename from src/tests/list.rs rename to src/tests/lists.rs index 6ef1d30..2914ff3 100644 --- a/src/tests/list.rs +++ b/src/tests/lists.rs @@ -1,15 +1,16 @@ -use std::alloc::{dealloc, Layout}; -use std::any::Any; +use std::alloc::Layout; use std::panic::{AssertUnwindSafe, catch_unwind}; use std::ptr::NonNull; +use test_case::{test_case, test_matrix}; + use crate::{CcBox, Mark}; -use crate::lists::*; use crate::counter_marker::CounterMarker; +use crate::lists::*; +use crate::state::state; +use crate::utils::cc_dealloc; -use test_case::{test_case, test_matrix}; - -fn assert_contains(list: &impl ListMethodsExt, mut elements: Vec) { +fn assert_contains(list: &impl ListMethods, mut elements: Vec) { list.iter().for_each(|ptr| { // Test contains assert!(list.contains(ptr)); @@ -27,7 +28,7 @@ fn assert_contains(list: &impl ListMethodsExt, mut elements: Vec) { ); } -fn new_list(elements: &[i32], list: &mut impl ListMethodsExt) -> Vec>> { +fn new_list(elements: &[i32], list: &mut impl ListMethods) -> Vec>> { elements .iter() .map(|&i| CcBox::new_for_tests(i)) @@ -47,13 +48,14 @@ fn deallocate(elements: Vec>>) { "{} has a prev", *ptr.as_ref().get_elem() ); - dealloc(ptr.cast().as_ptr(), Layout::new::>()); + state(|state| cc_dealloc(ptr, Layout::new::>(), state)); }); } -fn check_list(list: &impl ListMethodsExt) { +fn check_list(list: &impl ListMethods) { let mut iter = list.iter(); let Some(first) = iter.next() else { + assert!(list.is_empty()); list.assert_size(0); return; }; @@ -70,19 +72,21 @@ fn check_list(list: &impl ListMethodsExt) { list.assert_size(real_size); } -#[test_case(List::new())] -#[test_case(CountedList::new())] -fn test_new(list: impl ListMethodsExt) { - assert!(list.first().is_none()); +#[test_case(LinkedList::new())] +#[test_case(PossibleCycles::new())] +fn test_new(mut list: impl ListMethods) { + assert!(list.is_empty()); list.assert_size(0); + assert!(list.first().is_none()); + assert!(list.remove_first().is_none()); } -#[test_case(List::new())] -#[test_case(CountedList::new())] -fn test_add(mut list: impl ListMethodsExt) { +#[test_case(LinkedList::new())] +#[test_case(PossibleCycles::new())] +fn test_add(mut list: impl ListMethods) { let vec: Vec = vec![0, 1, 2]; - assert!(list.first().is_none()); + assert!(list.is_empty()); let elements = new_list(&vec, &mut list); assert!(list.first().is_some()); @@ -95,10 +99,10 @@ fn test_add(mut list: impl ListMethodsExt) { } #[test_matrix( - [List::new(), CountedList::new()], + [LinkedList::new(), PossibleCycles::new()], [0, 1, 2, 3] )] -fn test_remove(mut list: impl ListMethodsExt, index: usize) { +fn test_remove(mut list: impl ListMethods, index: usize) { let mut elements = vec![0, 1, 2, 3]; let vec = new_list(&elements, &mut list); @@ -131,9 +135,58 @@ fn test_remove(mut list: impl ListMethodsExt, index: usize) { deallocate(vec); } -#[test_case(List::new())] -#[test_case(CountedList::new())] -fn test_for_each_clearing_panic(mut list: impl ListMethodsExt) { +#[test_case(LinkedList::new())] +#[test_case(PossibleCycles::new())] +fn test_remove_first(mut list: impl ListMethods) { + let mut elements = vec![0, 1, 2, 3]; + let vec = new_list(&elements, &mut list); + + // Mark to test the removal of the mark + list.iter().for_each(|ptr| unsafe { + ptr.as_ref().counter_marker().mark(Mark::Traced) + }); + + // Iterate over the list to get the elements in the correct order as in the list + elements = list.iter().map(|ptr| unsafe { + *ptr.cast::>().as_ref().get_elem() + }).collect(); + + for element in elements { + let removed = list.remove_first().expect("List has smaller size then expected"); + + unsafe { + assert!( + (*removed.as_ref().get_next()).is_none(), + "Removed element has still a next." + ); + assert!( + (*removed.as_ref().get_prev()).is_none(), + "Removed element has still a prev." + ); + assert_eq!( + *removed.cast::>().as_ref().get_elem(), + element, + "Removed wrong element" + ); + let cm = removed.as_ref().counter_marker(); + assert!( + cm.is_not_marked() && !cm.is_in_possible_cycles(), + "Removed element is still marked" + ); + } + + check_list(&list); + } + + list.assert_size(0); + assert!(list.is_empty()); + drop(list); + deallocate(vec); +} + +#[test] +fn test_for_each_clearing_panic() { + let mut list = LinkedList::new(); let mut vec = new_list(&[0, 1, 2, 3], &mut list); for it in &mut vec { @@ -165,28 +218,28 @@ fn test_for_each_clearing_panic(mut list: impl ListMethodsExt) { deallocate(vec); } -#[test_case(List::new())] -#[test_case(CountedList::new())] -fn test_list_moving(mut list: impl ListMethodsExt) { +#[test_case(LinkedList::new())] +#[test_case(PossibleCycles::new())] +fn test_list_moving(mut list: impl ListMethods) { let cc = CcBox::new_for_tests(5i32); list.add(cc.cast()); let list_moved = list; - list_moved.into_iter().for_each(|elem| unsafe { + list_moved.iter().for_each(|elem| unsafe { assert_eq!(*elem.cast::>().as_ref().get_elem(), 5i32); }); - unsafe { - dealloc(cc.cast().as_ptr(), Layout::new::>()); - } + drop(list_moved); + + deallocate(vec![cc]); } #[cfg(feature = "finalization")] #[test] fn test_mark_self_and_append() { - let mut list = CountedList::new(); - let mut to_append = List::new(); + let mut list = PossibleCycles::new(); + let mut to_append = LinkedList::new(); let elements: Vec = vec![0, 1, 2]; let elements_to_append: Vec = vec![3, 4, 5]; let elements_final: Vec = vec![0, 1, 2, 3, 4, 5]; @@ -221,8 +274,8 @@ fn test_mark_self_and_append() { #[cfg(feature = "finalization")] #[test] fn test_mark_self_and_append_empty_list() { - let mut list = CountedList::new(); - let to_append = List::new(); + let mut list = PossibleCycles::new(); + let to_append = LinkedList::new(); let elements: Vec = vec![0, 1, 2]; let vec = new_list(&elements, &mut list); @@ -250,8 +303,8 @@ fn test_mark_self_and_append_empty_list() { #[cfg(feature = "finalization")] #[test] fn test_mark_empty_self_and_append() { - let mut list = CountedList::new(); - let mut to_append = List::new(); + let list = PossibleCycles::new(); + let mut to_append = LinkedList::new(); let elements: Vec = vec![0, 1, 2]; let vec = new_list(&elements, &mut to_append); @@ -279,23 +332,22 @@ fn test_mark_empty_self_and_append() { #[cfg(feature = "finalization")] #[test] fn test_mark_empty_self_and_append_empty_list() { - let mut list = CountedList::new(); - let to_append = List::new(); + let list = PossibleCycles::new(); + let to_append = LinkedList::new(); unsafe { list.mark_self_and_append(Mark::PossibleCycles, to_append, 0); } list.assert_size(0); - assert!(list.is_empty()); } #[cfg(feature = "finalization")] #[test] fn test_swap_list() { - let mut list = CountedList::new(); - let mut to_swap = List::new(); + let mut list = PossibleCycles::new(); + let mut to_swap = LinkedList::new(); let elements: Vec = vec![0, 1, 2]; let elements_to_swap: Vec = vec![3, 4, 5]; @@ -319,14 +371,90 @@ fn test_swap_list() { deallocate(vec_to_swap); } -trait ListMethodsExt: ListMethods + Any { +// Common methods to DRY in list's tests +// Also, mutating methods always take a &mut reference, even for PossibleCycles +trait ListMethods { + fn first(&self) -> Option>>; + + fn add(&mut self, ptr: NonNull>); + + fn remove(&mut self, ptr: NonNull>); + + fn remove_first(&mut self) -> Option>>; + + fn is_empty(&self) -> bool; + + fn iter(&self) -> Iter; + + fn contains(&self, ptr: NonNull>) -> bool; + fn assert_size(&self, expected_size: usize); } -impl ListMethodsExt for T { +impl ListMethods for LinkedList { + fn first(&self) -> Option>> { + self.first() + } + + fn add(&mut self, ptr: NonNull>) { + self.add(ptr) + } + + fn remove(&mut self, ptr: NonNull>) { + self.remove(ptr) + } + + fn remove_first(&mut self) -> Option>> { + self.remove_first() + } + + fn is_empty(&self) -> bool { + self.is_empty() + } + + fn iter(&self) -> Iter { + self.iter() + } + + fn contains(&self, ptr: NonNull>) -> bool { + self.iter().any(|elem| elem == ptr) + } + fn assert_size(&self, expected_size: usize) { - if let Some(counted_list) = (self as &dyn Any).downcast_ref::() { - assert_eq!(expected_size, counted_list.size()); - } + assert_eq!(expected_size, self.iter().count()); + } +} + +impl ListMethods for PossibleCycles { + fn first(&self) -> Option>> { + self.first() + } + + fn add(&mut self, ptr: NonNull>) { + Self::add(self, ptr) + } + + fn remove(&mut self, ptr: NonNull>) { + Self::remove(self, ptr) + } + + fn remove_first(&mut self) -> Option>> { + Self::remove_first(self) + } + + fn is_empty(&self) -> bool { + self.is_empty() + } + + fn iter(&self) -> Iter { + self.iter() + } + + fn contains(&self, ptr: NonNull>) -> bool { + self.contains(ptr) + } + + fn assert_size(&self, expected_size: usize) { + assert_eq!(expected_size, self.size()); } } diff --git a/src/tests/mod.rs b/src/tests/mod.rs index ebc8da6..7bffdf5 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,17 +1,16 @@ #![cfg(test)] use std::rc::Rc; -use std::cell::Cell; +use std::cell::{Cell, RefCell}; use std::ops::{Deref, DerefMut}; use crate::trace::Trace; -use crate::lists::*; use crate::{state, Cc, Context, Finalize, POSSIBLE_CYCLES}; use crate::state::state; mod bench_code; mod cc; -mod list; +mod lists; mod panicking; mod counter_marker; @@ -22,9 +21,7 @@ mod weak; mod cleaners; pub(crate) fn reset_state() { - POSSIBLE_CYCLES.with(|pc| { - pc.replace(CountedList::new()); - }); + POSSIBLE_CYCLES.with(|pc| pc.reset()); state::reset_state(); #[cfg(feature = "auto-collect")] @@ -126,8 +123,7 @@ impl DropChecker { } pub(crate) fn assert_empty() { - let list = POSSIBLE_CYCLES.with(|pc| pc.borrow().first()); - assert!(list.is_none()); + assert!(POSSIBLE_CYCLES.with(|pc| pc.is_empty())); } pub(crate) fn assert_collecting() { diff --git a/src/tests/weak/mod.rs b/src/tests/weak/mod.rs index 14ac1be..ef2013d 100644 --- a/src/tests/weak/mod.rs +++ b/src/tests/weak/mod.rs @@ -1,7 +1,6 @@ -use std::cell::Cell; use std::panic::catch_unwind; use crate::*; -use super::reset_state; +use super::*; use crate::weak::Weak; #[test] diff --git a/src/trace.rs b/src/trace.rs index 34a55dd..acb0b90 100644 --- a/src/trace.rs +++ b/src/trace.rs @@ -21,7 +21,7 @@ use std::{ ffi::{OsStr, OsString} }; -use crate::List; +use crate::lists::LinkedList; /// Trait to finalize objects before freeing them. /// @@ -138,12 +138,12 @@ pub struct Context<'a> { pub(crate) enum ContextInner<'a> { Counting { - root_list: &'a mut List, - non_root_list: &'a mut List, + root_list: &'a mut LinkedList, + non_root_list: &'a mut LinkedList, }, RootTracing { - root_list: &'a mut List, - non_root_list: &'a mut List, + root_list: &'a mut LinkedList, + non_root_list: &'a mut LinkedList, }, } From 9493d246997c05f02fdd17726bc3441c7782bbce Mon Sep 17 00:00:00 2001 From: fren_gor Date: Wed, 31 Jul 2024 03:24:26 +0200 Subject: [PATCH 04/13] Add LinkedQueue --- src/lists.rs | 87 +++++++++++++++++++ src/tests/lists.rs | 208 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 271 insertions(+), 24 deletions(-) diff --git a/src/lists.rs b/src/lists.rs index 60ff97d..1240867 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -366,6 +366,93 @@ impl<'a> IntoIterator for &'a PossibleCycles { } } +#[allow(dead_code)] // TODO Remove when actually used +pub(crate) struct LinkedQueue { + first: Option>>, + last: Option>>, +} + +#[allow(dead_code)] // TODO Remove when actually used +impl LinkedQueue { + #[inline] + pub(crate) const fn new() -> Self { + Self { + first: None, + last: None, + } + } + + #[inline] + pub(crate) fn add(&mut self, ptr: NonNull>) { + debug_assert_nones(ptr); + + if let Some(last) = self.last { + unsafe { + *last.as_ref().get_next() = Some(ptr); + } + } else { + self.first = Some(ptr); + } + + self.last = Some(ptr); + } + + #[inline] + pub(crate) fn peek(&self) -> Option>> { + self.first + } + + #[inline] + pub(crate) fn poll(&mut self) -> Option>> { + match self.first { + Some(first) => unsafe { + self.first = *first.as_ref().get_next(); + if self.first.is_none() { + // The last element is being removed + self.last = None; + } + *first.as_ref().get_next() = None; + + // Make sure the mark is correct + first.as_ref().counter_marker().mark(Mark::NonMarked); + + Some(first) + }, + None => { + None + }, + } + } + + #[inline] + pub(crate) fn is_empty(&self) -> bool { + self.peek().is_none() + } +} + +impl Drop for LinkedQueue { + #[inline] + fn drop(&mut self) { + // Remove the remaining elements from the queue + while self.poll().is_some() { + // poll() already marks every removed element NonMarked + } + } +} + +impl<'a> IntoIterator for &'a LinkedQueue { + type Item = NonNull>; + type IntoIter = Iter<'a>; + + #[inline] + fn into_iter(self) -> Self::IntoIter { + Iter { + next: self.first, + _phantom: PhantomData, + } + } +} + #[inline(always)] // The fn is always empty in release mode fn debug_assert_nones(ptr: NonNull>) { unsafe { diff --git a/src/tests/lists.rs b/src/tests/lists.rs index 2914ff3..571c323 100644 --- a/src/tests/lists.rs +++ b/src/tests/lists.rs @@ -28,11 +28,11 @@ fn assert_contains(list: &impl ListMethods, mut elements: Vec) { ); } -fn new_list(elements: &[i32], list: &mut impl ListMethods) -> Vec>> { +fn new_collection(elements: &[i32], coll: &mut impl CommonMethods) -> Vec>> { elements .iter() .map(|&i| CcBox::new_for_tests(i)) - .inspect(|&ptr| list.add(ptr.cast())) + .inspect(|&ptr| coll.add(ptr.cast())) .collect() } @@ -87,7 +87,7 @@ fn test_add(mut list: impl ListMethods) { let vec: Vec = vec![0, 1, 2]; assert!(list.is_empty()); - let elements = new_list(&vec, &mut list); + let elements = new_collection(&vec, &mut list); assert!(list.first().is_some()); list.assert_size(vec.len()); @@ -104,7 +104,7 @@ fn test_add(mut list: impl ListMethods) { )] fn test_remove(mut list: impl ListMethods, index: usize) { let mut elements = vec![0, 1, 2, 3]; - let vec = new_list(&elements, &mut list); + let vec = new_collection(&elements, &mut list); list.assert_size(4); @@ -139,7 +139,7 @@ fn test_remove(mut list: impl ListMethods, index: usize) { #[test_case(PossibleCycles::new())] fn test_remove_first(mut list: impl ListMethods) { let mut elements = vec![0, 1, 2, 3]; - let vec = new_list(&elements, &mut list); + let vec = new_collection(&elements, &mut list); // Mark to test the removal of the mark list.iter().for_each(|ptr| unsafe { @@ -187,7 +187,7 @@ fn test_remove_first(mut list: impl ListMethods) { #[test] fn test_for_each_clearing_panic() { let mut list = LinkedList::new(); - let mut vec = new_list(&[0, 1, 2, 3], &mut list); + let mut vec = new_collection(&[0, 1, 2, 3], &mut list); for it in &mut vec { unsafe { @@ -244,8 +244,8 @@ fn test_mark_self_and_append() { let elements_to_append: Vec = vec![3, 4, 5]; let elements_final: Vec = vec![0, 1, 2, 3, 4, 5]; - let vec = new_list(&elements, &mut list); - let vec_to_append = new_list(&elements_to_append, &mut to_append); + let vec = new_collection(&elements, &mut list); + let vec_to_append = new_collection(&elements_to_append, &mut to_append); let list_size = list.iter().inspect(|elem| unsafe { elem.as_ref().counter_marker().mark(Mark::Traced); @@ -278,7 +278,7 @@ fn test_mark_self_and_append_empty_list() { let to_append = LinkedList::new(); let elements: Vec = vec![0, 1, 2]; - let vec = new_list(&elements, &mut list); + let vec = new_collection(&elements, &mut list); list.iter().for_each(|elem| unsafe { elem.as_ref().counter_marker().mark(Mark::Traced); @@ -307,7 +307,7 @@ fn test_mark_empty_self_and_append() { let mut to_append = LinkedList::new(); let elements: Vec = vec![0, 1, 2]; - let vec = new_list(&elements, &mut to_append); + let vec = new_collection(&elements, &mut to_append); to_append.iter().for_each(|elem| unsafe { elem.as_ref().counter_marker().mark(Mark::PossibleCycles); @@ -351,8 +351,8 @@ fn test_swap_list() { let elements: Vec = vec![0, 1, 2]; let elements_to_swap: Vec = vec![3, 4, 5]; - let vec = new_list(&elements, &mut list); - let vec_to_swap = new_list(&elements_to_swap, &mut to_swap); + let vec = new_collection(&elements, &mut list); + let vec_to_swap = new_collection(&elements_to_swap, &mut to_swap); unsafe { list.swap_list(&mut to_swap, elements_to_swap.len()); @@ -371,13 +371,16 @@ fn test_swap_list() { deallocate(vec_to_swap); } -// Common methods to DRY in list's tests +// Lists and queue common methods +trait CommonMethods { + fn add(&mut self, ptr: NonNull>); +} + +// Lists common methods to DRY in list's tests // Also, mutating methods always take a &mut reference, even for PossibleCycles -trait ListMethods { +trait ListMethods: CommonMethods { fn first(&self) -> Option>>; - fn add(&mut self, ptr: NonNull>); - fn remove(&mut self, ptr: NonNull>); fn remove_first(&mut self) -> Option>>; @@ -391,15 +394,17 @@ trait ListMethods { fn assert_size(&self, expected_size: usize); } +impl CommonMethods for LinkedList { + fn add(&mut self, ptr: NonNull>) { + self.add(ptr) + } +} + impl ListMethods for LinkedList { fn first(&self) -> Option>> { self.first() } - fn add(&mut self, ptr: NonNull>) { - self.add(ptr) - } - fn remove(&mut self, ptr: NonNull>) { self.remove(ptr) } @@ -425,15 +430,17 @@ impl ListMethods for LinkedList { } } +impl CommonMethods for PossibleCycles { + fn add(&mut self, ptr: NonNull>) { + Self::add(self, ptr) + } +} + impl ListMethods for PossibleCycles { fn first(&self) -> Option>> { self.first() } - fn add(&mut self, ptr: NonNull>) { - Self::add(self, ptr) - } - fn remove(&mut self, ptr: NonNull>) { Self::remove(self, ptr) } @@ -458,3 +465,156 @@ impl ListMethods for PossibleCycles { assert_eq!(expected_size, self.size()); } } + +mod queue { + use super::*; + + fn check_queue(queue: &LinkedQueue) { + let mut iter = queue.into_iter(); + let Some(first) = iter.next() else { + assert!(queue.is_empty()); + return; + }; + unsafe { + assert_eq!(*first.as_ref().get_prev(), None); + let mut last = first; + for elem in iter { + assert_eq!(*elem.as_ref().get_prev(), None); + last = elem; + } + assert_eq!(*last.as_ref().get_next(), None); + assert_eq!(*last.as_ref().get_prev(), None); + } + } + + fn assert_queue_contains(queue: &LinkedQueue, mut elements: Vec) { + queue.into_iter().for_each(|ptr| { + // Test contains + assert!(queue.into_iter().any(|elem| elem == ptr)); + + let elem = unsafe { *ptr.cast::>().as_ref().get_elem() }; + let index = elements.iter().position(|&i| i == elem); + assert!(index.is_some(), "Couldn't find element {} in queue.", elem); + elements.swap_remove(index.unwrap()); + }); + + assert!( + elements.is_empty(), + "Queue does not contains: {:?}", + elements + ); + } + + impl CommonMethods for LinkedQueue { + fn add(&mut self, ptr: NonNull>) { + self.add(ptr) + } + } + + #[test] + fn test_new() { + let mut queue = LinkedQueue::new(); + assert!(queue.is_empty()); + assert!(queue.peek().is_none()); + assert!(queue.poll().is_none()); + } + + #[test] + fn test_add() { + let mut queue = LinkedQueue::new(); + let vec: Vec = vec![0, 1, 2]; + + assert!(queue.is_empty()); + let elements = new_collection(&vec, &mut queue); + assert!(queue.peek().is_some()); + + check_queue(&queue); + assert_queue_contains(&queue, vec); + + drop(queue); + deallocate(elements); + } + + #[test] + fn test_peek() { + let mut queue = LinkedQueue::new(); + let elements = new_collection(&[5], &mut queue); + let peek = queue.peek(); + + assert!(peek.is_some()); + assert_eq!(peek.unwrap().cast(), elements[0]); + + // Test that peek doesn't remove the first element + assert_eq!(peek, queue.peek()); + + drop(queue); + deallocate(elements); + } + + #[test] + fn test_poll() { + let mut queue = LinkedQueue::new(); + let vec: Vec = vec![0, 1, 2]; + let elements = new_collection(&vec, &mut queue); + + // Mark to test the removal of the mark + queue.into_iter().for_each(|ptr| unsafe { + ptr.as_ref().counter_marker().mark(Mark::Traced) + }); + + for elem in vec { + let removed = queue.poll().expect("Queue has smaller size then expected"); + + unsafe { + assert!( + (*removed.as_ref().get_next()).is_none(), + "Removed element has still a next." + ); + assert!( + (*removed.as_ref().get_prev()).is_none(), + "Removed element has a prev." + ); + assert_eq!( + *removed.cast::>().as_ref().get_elem(), + elem, + "Removed wrong element" + ); + let cm = removed.as_ref().counter_marker(); + assert!( + cm.is_not_marked() && !cm.is_in_possible_cycles(), + "Removed element is still marked" + ); + } + + check_queue(&queue); + } + + assert!(queue.is_empty()); + assert!(queue.peek().is_none()); + + drop(queue); + deallocate(elements); + } + + #[test] + fn test_clearing_mark() { + let mut queue = LinkedQueue::new(); + let vec = new_collection(&[0, 1, 2, 3], &mut queue); + + // Mark to test the removal of the mark + queue.into_iter().for_each(|ptr| unsafe { + ptr.as_ref().counter_marker().mark(Mark::Traced) + }); + + drop(queue); + + for elem in &vec { + let counter_marker = unsafe { elem.as_ref().counter_marker() }; + + assert!(counter_marker.is_not_marked()); + assert!(!counter_marker.is_in_possible_cycles()); + } + + deallocate(vec); + } +} From e0057d4a5d718f604d412678e767198c7f32d5f7 Mon Sep 17 00:00:00 2001 From: fren_gor Date: Thu, 1 Aug 2024 20:20:21 +0200 Subject: [PATCH 05/13] Add `IN_QUEUE` mark Also, `TRACED` is renamed to `IN_LIST` and the `DROPPED` marking is moved to a special value of the tracing counter --- src/cc.rs | 19 ++-- src/counter_marker.rs | 101 ++++++++++++------- src/lib.rs | 6 +- src/tests/counter_marker.rs | 193 ++++++++++++++++++++++++------------ src/tests/lists.rs | 10 +- src/weak/mod.rs | 8 +- 6 files changed, 214 insertions(+), 123 deletions(-) diff --git a/src/cc.rs b/src/cc.rs index be239f1..99ebe55 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -86,7 +86,7 @@ impl Cc { assert!(self.is_unique(), "Cc<_> is not unique"); assert!( - !self.counter_marker().is_traced(), + !self.counter_marker().is_in_list_or_queue(), "Cc<_> is being used by the collector and inner value cannot be taken out (this might have happen inside Trace, Finalize or Drop implementations)." ); @@ -259,10 +259,9 @@ impl Drop for Cc { add_to_list(cc.inner.cast()); } - // A CcBox can be marked traced only during collections while being into a list different than POSSIBLE_CYCLES. + // A CcBox can be in list or queue only during collections while being into a list different than POSSIBLE_CYCLES. // In this case, no further action has to be taken, except decrementing the reference counter. - // Skip the rest of the code also when the value has already been dropped - if self.counter_marker().is_traced_or_dropped() { + if self.counter_marker().is_in_list_or_queue() { decrement_counter(self); return; } @@ -298,7 +297,7 @@ impl Drop for Cc { { // Set the object as dropped before dropping and deallocating it // This feature is used only in weak pointers, so do this only if they're enabled - self.counter_marker().mark(Mark::Dropped); + self.counter_marker().set_dropped(true); self.inner().drop_metadata(); } @@ -590,7 +589,7 @@ impl CcBox<()> { { // Set the object as dropped before dropping it // This feature is used only in weak pointers, so do this only if they're enabled - ptr.as_ref().counter_marker().mark(Mark::Dropped); + ptr.as_ref().counter_marker().set_dropped(true); } CcBox::get_traceable(ptr).as_mut().drop_elem(); @@ -623,7 +622,7 @@ impl CcBox<()> { counter_marker.reset_tracing_counter(); // Element is surely not already marked, marking - counter_marker.mark(Mark::Traced); + counter_marker.mark(Mark::InList); }, ContextInner::RootTracing { .. } => { // ptr is a root @@ -659,7 +658,7 @@ impl CcBox<()> { root_list, non_root_list, } => { - if !counter_marker.is_traced() { + if !counter_marker.is_in_list() { // Not already marked // Make sure ptr is not in POSSIBLE_CYCLES list @@ -680,7 +679,7 @@ impl CcBox<()> { // Marking here since the previous debug_asserts might panic // before ptr is actually added to root_list or non_root_list - counter_marker.mark(Mark::Traced); + counter_marker.mark(Mark::InList); // Continue tracing true @@ -703,7 +702,7 @@ impl CcBox<()> { } }, ContextInner::RootTracing { non_root_list, root_list } => { - if counter_marker.is_traced() { + if counter_marker.is_in_list() { // Marking NonMarked since ptr will be removed from any list it's into. Also, marking // NonMarked will avoid tracing this CcBox again (thanks to the if condition) counter_marker.mark(Mark::NonMarked); diff --git a/src/counter_marker.rs b/src/counter_marker.rs index cfe90ac..66d74b3 100644 --- a/src/counter_marker.rs +++ b/src/counter_marker.rs @@ -4,9 +4,9 @@ use crate::utils; const NON_MARKED: u32 = 0u32; const IN_POSSIBLE_CYCLES: u32 = 1u32 << (u32::BITS - 2); -const TRACED: u32 = 2u32 << (u32::BITS - 2); -#[allow(dead_code)] // Used only in weak ptrs, silence warnings -const DROPPED: u32 = 3u32 << (u32::BITS - 2); +const IN_LIST: u32 = 2u32 << (u32::BITS - 2); +#[allow(dead_code)] // TODO Remove when actually used +const IN_QUEUE: u32 = 3u32 << (u32::BITS - 2); const COUNTER_MASK: u32 = 0b11111111111111u32; // First 14 bits set to 1 const TRACING_COUNTER_MASK: u32 = COUNTER_MASK << 14; // 14 bits set to 1 followed by 14 bits set to 0 @@ -19,7 +19,7 @@ const INITIAL_VALUE: u32 = COUNTER_MASK + 2; // +2 means that tracing counter an const INITIAL_VALUE_FINALIZED: u32 = INITIAL_VALUE | FINALIZED_MASK; // pub(crate) to make it available in tests -pub(crate) const MAX: u32 = COUNTER_MASK; +pub(crate) const MAX: u32 = COUNTER_MASK - 1; /// Internal representation: /// ```text @@ -31,12 +31,14 @@ pub(crate) const MAX: u32 = COUNTER_MASK; /// * `A` has 4 possible states: /// * `NON_MARKED` /// * `IN_POSSIBLE_CYCLES`: in `possible_cycles` list (implies `NON_MARKED`) -/// * `TRACED`: in `root_list` or `non_root_list` -/// * `DROPPED`: allocated value has already been dropped (but not yet deallocated) +/// * `IN_LIST`: in `root_list` or `non_root_list` +/// * `IN_QUEUE`: in queue to be traced /// * `B` is `1` when metadata has been allocated, `0` otherwise /// * `C` is `1` when the element inside `CcBox` has already been finalized, `0` otherwise -/// * `D` is the tracing counter -/// * `E` is the counter (last one for sum/subtraction efficiency) +/// * `D` is the tracing counter. The max value (the one with every bit set to 1) is reserved +/// and indicates that the allocated value has already been dropped (but not yet deallocated) +/// * `E` is the reference counter (last one for sum/subtraction efficiency). The max value (the +/// one with every bit set to 1) is reserved and should not be used #[derive(Clone, Debug)] #[repr(transparent)] pub(crate) struct CounterMarker { @@ -60,6 +62,8 @@ impl CounterMarker { #[inline] pub(crate) fn increment_counter(&self) -> Result<(), OverflowError> { + debug_assert!(self.counter() != COUNTER_MASK); // Check for reserved value + if self.counter() == MAX { utils::cold(); // This branch of the if is rarely taken Err(OverflowError) @@ -71,6 +75,8 @@ impl CounterMarker { #[inline] pub(crate) fn decrement_counter(&self) -> Result<(), OverflowError> { + debug_assert!(self.counter() != COUNTER_MASK); // Check for reserved value + if self.counter() == 0 { utils::cold(); // This branch of the if is rarely taken Err(OverflowError) @@ -82,6 +88,8 @@ impl CounterMarker { #[inline] pub(crate) fn increment_tracing_counter(&self) -> Result<(), OverflowError> { + debug_assert!(self.tracing_counter() != COUNTER_MASK); // Check for reserved value + if self.tracing_counter() == MAX { utils::cold(); // This branch of the if is rarely taken Err(OverflowError) @@ -94,6 +102,8 @@ impl CounterMarker { #[inline] pub(crate) fn _decrement_tracing_counter(&self) -> Result<(), OverflowError> { + debug_assert!(self.tracing_counter() != COUNTER_MASK); // Check for reserved value + if self.tracing_counter() == 0 { utils::cold(); // This branch of the if is rarely taken Err(OverflowError) @@ -106,24 +116,24 @@ impl CounterMarker { #[inline] pub(crate) fn counter(&self) -> u32 { - self.counter.get() & COUNTER_MASK + let rc = self.counter.get() & COUNTER_MASK; + debug_assert!(rc != COUNTER_MASK); // Check for reserved value + rc } #[inline] pub(crate) fn tracing_counter(&self) -> u32 { - (self.counter.get() & TRACING_COUNTER_MASK) >> 14 + let tc = (self.counter.get() & TRACING_COUNTER_MASK) >> 14; + debug_assert!(tc != COUNTER_MASK); // Check for reserved value + tc } #[inline] pub(crate) fn reset_tracing_counter(&self) { + debug_assert!(self.tracing_counter() != COUNTER_MASK); // Check for reserved value self.counter.set(self.counter.get() & !TRACING_COUNTER_MASK); } - #[inline] - pub(crate) fn is_in_possible_cycles(&self) -> bool { - (self.counter.get() & BITS_MASK) == IN_POSSIBLE_CYCLES - } - #[cfg(feature = "finalization")] #[inline] pub(crate) fn needs_finalization(&self) -> bool { @@ -133,7 +143,7 @@ impl CounterMarker { #[cfg(feature = "finalization")] #[inline] pub(crate) fn set_finalized(&self, finalized: bool) { - self.set_bit(finalized, FINALIZED_MASK); + self.set_bits(finalized, FINALIZED_MASK); } #[cfg(feature = "weak-ptrs")] @@ -145,17 +155,19 @@ impl CounterMarker { #[cfg(feature = "weak-ptrs")] #[inline] pub(crate) fn set_allocated_for_metadata(&self, allocated_for_metadata: bool) { - self.set_bit(allocated_for_metadata, METADATA_MASK); + self.set_bits(allocated_for_metadata, METADATA_MASK); } - #[cfg(any(feature = "weak-ptrs", feature = "finalization"))] - #[inline(always)] - fn set_bit(&self, value: bool, mask: u32) { - if value { - self.counter.set(self.counter.get() | mask); - } else { - self.counter.set(self.counter.get() & !mask); - } + #[cfg(feature = "weak-ptrs")] + #[inline] + pub(crate) fn is_dropped(&self) -> bool { + (self.counter.get() & TRACING_COUNTER_MASK) == TRACING_COUNTER_MASK + } + + #[cfg(feature = "weak-ptrs")] + #[inline] + pub(crate) fn set_dropped(&self, dropped: bool) { + self.set_bits(dropped, TRACING_COUNTER_MASK); } #[inline] @@ -166,27 +178,42 @@ impl CounterMarker { } #[inline] - pub(crate) fn is_traced(&self) -> bool { - (self.counter.get() & BITS_MASK) == TRACED + pub(crate) fn is_in_possible_cycles(&self) -> bool { + (self.counter.get() & BITS_MASK) == IN_POSSIBLE_CYCLES } #[inline] - pub(crate) fn is_traced_or_dropped(&self) -> bool { - // true if (self.counter & BITS_MASK) is equal to 10 or 11, - // so if the first bit is 1 - (self.counter.get() & FIRST_BIT_MASK) == FIRST_BIT_MASK + pub(crate) fn is_in_list(&self) -> bool { + (self.counter.get() & BITS_MASK) == IN_LIST } - #[cfg(any(feature = "weak-ptrs", all(test, feature = "std")))] + #[allow(dead_code)] // TODO Remove when actually used #[inline] - pub(crate) fn is_dropped(&self) -> bool { - (self.counter.get() & BITS_MASK) == DROPPED + pub(crate) fn is_in_queue(&self) -> bool { + (self.counter.get() & BITS_MASK) == IN_QUEUE + } + + #[inline] + pub(crate) fn is_in_list_or_queue(&self) -> bool { + // true if (self.counter & BITS_MASK) is equal to 10 or 11, + // so if the first bit is 1 + (self.counter.get() & FIRST_BIT_MASK) == FIRST_BIT_MASK } #[inline] pub(crate) fn mark(&self, new_mark: Mark) { self.counter.set((self.counter.get() & !BITS_MASK) | (new_mark as u32)); } + + #[cfg(any(feature = "weak-ptrs", feature = "finalization"))] + #[inline(always)] + fn set_bits(&self, value: bool, mask: u32) { + if value { + self.counter.set(self.counter.get() | mask); + } else { + self.counter.set(self.counter.get() & !mask); + } + } } #[derive(Copy, Clone, Debug)] @@ -194,7 +221,7 @@ impl CounterMarker { pub(crate) enum Mark { NonMarked = NON_MARKED, PossibleCycles = IN_POSSIBLE_CYCLES, - Traced = TRACED, - #[allow(dead_code)] // Used only in weak ptrs, silence warnings - Dropped = DROPPED, + InList = IN_LIST, + #[allow(dead_code)] // TODO Remove when actually used + InQueue = IN_QUEUE, } diff --git a/src/lib.rs b/src/lib.rs index 9da9c9d..bb0225e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -299,7 +299,7 @@ fn __collect(state: &State, possible_cycles: &PossibleCycles) { counter_marker.tracing_counter(), counter_marker.counter() ); - debug_assert!(counter_marker.is_traced()); + debug_assert!(counter_marker.is_in_list()); }); #[cfg(feature = "finalization")] @@ -382,7 +382,7 @@ fn deallocate_list(to_deallocate_list: LinkedList, state: &State) { #[cfg(feature = "weak-ptrs")] while let Some(ptr) = self.list.remove_first() { // Always set the mark, since it has been cleared by remove_first - unsafe { ptr.as_ref() }.counter_marker().mark(Mark::Dropped); + unsafe { ptr.as_ref() }.counter_marker().set_dropped(true); } // If not using weak pointers, just call the list's drop implementation @@ -404,7 +404,7 @@ fn deallocate_list(to_deallocate_list: LinkedList, state: &State) { to_deallocate_list.iter().for_each(|ptr| { // SAFETY: ptr is valid to access and drop in place unsafe { - debug_assert!(ptr.as_ref().counter_marker().is_traced()); + debug_assert!(ptr.as_ref().counter_marker().is_in_list()); #[cfg(feature = "weak-ptrs")] ptr.as_ref().drop_metadata(); diff --git a/src/tests/counter_marker.rs b/src/tests/counter_marker.rs index e1a6bd5..5c04dc0 100644 --- a/src/tests/counter_marker.rs +++ b/src/tests/counter_marker.rs @@ -3,14 +3,28 @@ use crate::counter_marker::*; fn assert_not_marked(counter: &CounterMarker) { assert!(counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); - assert!(!counter.is_traced()); - assert!(!counter.is_dropped()); + assert!(!counter.is_in_list()); + assert!(!counter.is_in_queue()); + assert!(!counter.is_in_list_or_queue()); + assert!(!counter.is_in_list_or_queue()); +} + +fn assert_default_settings(_counter: &CounterMarker) { + #[cfg(feature = "finalization")] + assert!(_counter.needs_finalization()); + + #[cfg(feature = "weak-ptrs")] + { + assert!(!_counter.has_allocated_for_metadata()); + assert!(!_counter.is_dropped()); + } } #[test] fn test_new() { fn test(counter: CounterMarker) { assert_not_marked(&counter); + assert_default_settings(&counter); assert_eq!(counter.counter(), 1); assert_eq!(counter.tracing_counter(), 1); @@ -23,83 +37,119 @@ fn test_new() { #[cfg(feature = "finalization")] #[test] fn test_is_to_finalize() { - let counter = CounterMarker::new_with_counter_to_one(false); - assert_not_marked(&counter); - assert!(counter.needs_finalization()); - - let counter = CounterMarker::new_with_counter_to_one(false); - assert_not_marked(&counter); - counter.set_finalized(true); - assert!(!counter.needs_finalization()); - - let counter = CounterMarker::new_with_counter_to_one(false); - assert_not_marked(&counter); - counter.set_finalized(false); - assert!(counter.needs_finalization()); - - let counter = CounterMarker::new_with_counter_to_one(true); - assert_not_marked(&counter); - assert!(!counter.needs_finalization()); - - let counter = CounterMarker::new_with_counter_to_one(true); - assert_not_marked(&counter); - counter.set_finalized(true); - assert!(!counter.needs_finalization()); - - let counter = CounterMarker::new_with_counter_to_one(true); - assert_not_marked(&counter); - counter.set_finalized(false); - assert!(counter.needs_finalization()); + fn assert_not_marked_fin(counter: &CounterMarker) { + assert_not_marked(counter); + #[cfg(feature = "weak-ptrs")] + { + assert!(!counter.has_allocated_for_metadata()); + assert!(!counter.is_dropped()); + } + } + + fn test(already_fin: bool) { + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_fin(&counter); + assert_eq!(!already_fin, counter.needs_finalization()); + + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_fin(&counter); + counter.set_finalized(true); + assert!(!counter.needs_finalization()); + + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_fin(&counter); + counter.set_finalized(false); + assert!(counter.needs_finalization()); + } + + test(true); + test(false); } #[cfg(feature = "weak-ptrs")] #[test] fn test_weak_ptrs_exists() { - let counter = CounterMarker::new_with_counter_to_one(false); - assert_not_marked(&counter); - assert!(!counter.has_allocated_for_metadata()); - - let counter = CounterMarker::new_with_counter_to_one(false); - assert_not_marked(&counter); - counter.set_allocated_for_metadata(true); - assert!(counter.has_allocated_for_metadata()); - - let counter = CounterMarker::new_with_counter_to_one(false); - assert_not_marked(&counter); - counter.set_allocated_for_metadata(false); - assert!(!counter.has_allocated_for_metadata()); - - let counter = CounterMarker::new_with_counter_to_one(true); - assert_not_marked(&counter); - assert!(!counter.has_allocated_for_metadata()); - - let counter = CounterMarker::new_with_counter_to_one(true); - assert_not_marked(&counter); - counter.set_allocated_for_metadata(true); - assert!(counter.has_allocated_for_metadata()); - - let counter = CounterMarker::new_with_counter_to_one(true); - assert_not_marked(&counter); - counter.set_allocated_for_metadata(false); - assert!(!counter.has_allocated_for_metadata()); + fn assert_not_marked_weak_ptrs(counter: &CounterMarker, _already_fin: bool) { + assert_not_marked(counter); + + assert!(!counter.is_dropped()); + + #[cfg(feature = "finalization")] + assert_eq!(!_already_fin, counter.needs_finalization()); + } + + fn test(already_fin: bool) { + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_weak_ptrs(&counter, already_fin); + assert!(!counter.has_allocated_for_metadata()); + + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_weak_ptrs(&counter, already_fin); + counter.set_allocated_for_metadata(true); + assert!(counter.has_allocated_for_metadata()); + + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_weak_ptrs(&counter, already_fin); + counter.set_allocated_for_metadata(false); + assert!(!counter.has_allocated_for_metadata()); + } + + test(true); + test(false); +} + +#[cfg(feature = "weak-ptrs")] +#[test] +fn test_dropped() { + fn assert_not_marked_dropped(counter: &CounterMarker, _already_fin: bool) { + assert_not_marked(counter); + + assert!(!counter.has_allocated_for_metadata()); + + #[cfg(feature = "finalization")] + assert_eq!(!_already_fin, counter.needs_finalization()); + } + + fn test(already_fin: bool) { + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_dropped(&counter, already_fin); + assert!(!counter.is_dropped()); + + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_dropped(&counter, already_fin); + counter.set_dropped(true); + assert!(counter.is_dropped()); + + let counter = CounterMarker::new_with_counter_to_one(already_fin); + assert_not_marked_dropped(&counter, already_fin); + counter.set_dropped(false); + assert!(!counter.is_dropped()); + } + + test(true); + test(false); } #[test] fn test_increment_decrement() { fn test(counter: CounterMarker) { assert_not_marked(&counter); + assert_default_settings(&counter); assert_eq!(counter.counter(), 1); assert_not_marked(&counter); + assert_default_settings(&counter); assert_eq!(counter.tracing_counter(), 1); assert_not_marked(&counter); + assert_default_settings(&counter); assert!(counter.increment_counter().is_ok()); assert_not_marked(&counter); + assert_default_settings(&counter); assert_eq!(counter.counter(), 2); assert_eq!(counter.tracing_counter(), 1); @@ -107,6 +157,7 @@ fn test_increment_decrement() { assert!(counter.increment_tracing_counter().is_ok()); assert_not_marked(&counter); + assert_default_settings(&counter); assert_eq!(counter.counter(), 2); assert_eq!(counter.tracing_counter(), 2); @@ -114,11 +165,13 @@ fn test_increment_decrement() { assert!(counter.decrement_counter().is_ok()); assert_not_marked(&counter); + assert_default_settings(&counter); assert_eq!(counter.counter(), 1); assert!(counter._decrement_tracing_counter().is_ok()); assert_not_marked(&counter); + assert_default_settings(&counter); assert_eq!(counter.counter(), 1); assert_eq!(counter.tracing_counter(), 1); @@ -149,6 +202,7 @@ fn test_increment_decrement() { } assert_not_marked(&counter); + assert_default_settings(&counter); } test(CounterMarker::new_with_counter_to_one(false)); @@ -159,35 +213,44 @@ fn test_increment_decrement() { fn test_marks() { fn test(counter: CounterMarker) { assert_not_marked(&counter); + assert_default_settings(&counter); counter.mark(Mark::NonMarked); assert_not_marked(&counter); + assert_default_settings(&counter); counter.mark(Mark::PossibleCycles); assert!(counter.is_not_marked()); assert!(counter.is_in_possible_cycles()); - assert!(!counter.is_traced()); - assert!(!counter.is_dropped()); + assert!(!counter.is_in_list()); + assert!(!counter.is_in_queue()); + assert!(!counter.is_in_list_or_queue()); + assert_default_settings(&counter); - counter.mark(Mark::Traced); + counter.mark(Mark::InList); assert!(!counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); - assert!(counter.is_traced()); - assert!(!counter.is_dropped()); + assert!(counter.is_in_list()); + assert!(!counter.is_in_queue()); + assert!(counter.is_in_list_or_queue()); + assert_default_settings(&counter); - counter.mark(Mark::Dropped); + counter.mark(Mark::InQueue); assert!(!counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); - assert!(!counter.is_traced()); - assert!(counter.is_dropped()); + assert!(!counter.is_in_list()); + assert!(counter.is_in_queue()); + assert!(counter.is_in_list_or_queue()); + assert_default_settings(&counter); counter.mark(Mark::NonMarked); assert_not_marked(&counter); + assert_default_settings(&counter); } test(CounterMarker::new_with_counter_to_one(false)); @@ -203,10 +266,12 @@ fn test_reset_tracing_counter() { let _ = counter.increment_tracing_counter(); assert_ne!(counter.tracing_counter(), 0); + assert_default_settings(&counter); counter.reset_tracing_counter(); assert_eq!(counter.tracing_counter(), 0); + assert_default_settings(&counter); } test(CounterMarker::new_with_counter_to_one(false)); diff --git a/src/tests/lists.rs b/src/tests/lists.rs index 571c323..48f8dbf 100644 --- a/src/tests/lists.rs +++ b/src/tests/lists.rs @@ -143,7 +143,7 @@ fn test_remove_first(mut list: impl ListMethods) { // Mark to test the removal of the mark list.iter().for_each(|ptr| unsafe { - ptr.as_ref().counter_marker().mark(Mark::Traced) + ptr.as_ref().counter_marker().mark(Mark::InList) }); // Iterate over the list to get the elements in the correct order as in the list @@ -248,7 +248,7 @@ fn test_mark_self_and_append() { let vec_to_append = new_collection(&elements_to_append, &mut to_append); let list_size = list.iter().inspect(|elem| unsafe { - elem.as_ref().counter_marker().mark(Mark::Traced); + elem.as_ref().counter_marker().mark(Mark::InList); }).count(); let to_append_size = to_append.iter().inspect(|elem| unsafe { elem.as_ref().counter_marker().mark(Mark::PossibleCycles); @@ -281,7 +281,7 @@ fn test_mark_self_and_append_empty_list() { let vec = new_collection(&elements, &mut list); list.iter().for_each(|elem| unsafe { - elem.as_ref().counter_marker().mark(Mark::Traced); + elem.as_ref().counter_marker().mark(Mark::InList); }); unsafe { @@ -559,7 +559,7 @@ mod queue { // Mark to test the removal of the mark queue.into_iter().for_each(|ptr| unsafe { - ptr.as_ref().counter_marker().mark(Mark::Traced) + ptr.as_ref().counter_marker().mark(Mark::InList) }); for elem in vec { @@ -603,7 +603,7 @@ mod queue { // Mark to test the removal of the mark queue.into_iter().for_each(|ptr| unsafe { - ptr.as_ref().counter_marker().mark(Mark::Traced) + ptr.as_ref().counter_marker().mark(Mark::InList) }); drop(queue); diff --git a/src/weak/mod.rs b/src/weak/mod.rs index 975bc38..64ac7a7 100644 --- a/src/weak/mod.rs +++ b/src/weak/mod.rs @@ -104,18 +104,18 @@ impl Weak { // SAFETY: self.cc is still allocated and can be dereferenced let counter_marker = unsafe { self.cc.as_ref() }.counter_marker(); - // Return 0 if the object is traced and the collector is dropping. This is necessary since it's UB to access + // Return 0 if the object is in a list (or queue) and the collector is dropping. This is necessary since it's UB to access // Ccs from destructors, so calling upgrade on weak ptrs to such Ccs must be prevented. - // This check does this, since such Ccs will be traced at this point. Also, given that deallocations are done after + // This check does this, since such Ccs will be in a list at this point. Also, given that deallocations are done after // calling every destructor (this is an implementation detail), it's safe to access the counter_marker here. - // Lastly, if the state cannot be accessed just return 0 to avoid giving a Cc when calling upgrade + // Lastly, if the state cannot be accessed just return 0 to avoid giving a Cc when calling upgrade. // Return 0 also in the case the object was dropped, since weak pointers can survive the object itself let counter = counter_marker.counter(); // Checking if the counter is already 0 avoids doing extra useless work, since the returned value would be the same if counter == 0 || counter_marker.is_dropped() || ( - counter_marker.is_traced() && try_state(|state| state.is_dropping()).unwrap_or(true) + counter_marker.is_in_list_or_queue() && try_state(|state| state.is_dropping()).unwrap_or(true) ) { 0 } else { From 27a22cf101eb66340301587fd9b5166e8ad7f329 Mon Sep 17 00:00:00 2001 From: fren_gor Date: Fri, 2 Aug 2024 22:11:43 +0200 Subject: [PATCH 06/13] Implement breadth-first tracing --- src/cc.rs | 151 ++++++++++-------------------------- src/counter_marker.rs | 5 +- src/lib.rs | 84 +++++++++++++++++--- src/lists.rs | 16 ++-- src/tests/cc.rs | 27 ++++--- src/tests/counter_marker.rs | 8 +- src/tests/lists.rs | 14 +--- src/trace.rs | 6 +- src/utils.rs | 19 +++++ 9 files changed, 166 insertions(+), 164 deletions(-) diff --git a/src/cc.rs b/src/cc.rs index 99ebe55..eed3aab 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -326,9 +326,7 @@ unsafe impl Trace for Cc { #[inline] #[track_caller] fn trace(&self, ctx: &mut Context<'_>) { - if CcBox::trace(self.inner.cast(), ctx) { - self.inner().get_elem().trace(ctx); - } + CcBox::trace(self.inner.cast(), ctx); } } @@ -501,26 +499,19 @@ impl Finalize for CcBox { pub(crate) fn remove_from_list(ptr: NonNull>) { let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); - // Check if ptr is in possible_cycles list if counter_marker.is_in_possible_cycles() { - // ptr is in the list, remove it let _ = POSSIBLE_CYCLES.try_with(|pc| { - // Confirm is_in_possible_cycles() in debug builds #[cfg(feature = "pedantic-debug-assertions")] - debug_assert!(pc.contains(ptr)); + debug_assert!(pc.iter().contains(ptr)); counter_marker.mark(Mark::NonMarked); pc.remove(ptr); }); } else { - // ptr is not in the list - - // Confirm !is_in_possible_cycles() in debug builds. - // This is safe to do since we're not putting the CcBox into the list #[cfg(feature = "pedantic-debug-assertions")] debug_assert! { POSSIBLE_CYCLES.try_with(|pc| { - !pc.contains(ptr) + !pc.iter().contains(ptr) }).unwrap_or(true) }; } @@ -531,29 +522,28 @@ pub(crate) fn add_to_list(ptr: NonNull>) { let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); let _ = POSSIBLE_CYCLES.try_with(|pc| { - // Check if ptr is in possible_cycles list since we have to move it at its start if counter_marker.is_in_possible_cycles() { - // Confirm is_in_possible_cycles() in debug builds #[cfg(feature = "pedantic-debug-assertions")] - debug_assert!(pc.contains(ptr)); + debug_assert!(pc.iter().contains(ptr)); pc.remove(ptr); - // In this case we don't need to update the mark since we put it back into the list + // Already marked } else { - // Confirm !is_in_possible_cycles() in debug builds #[cfg(feature = "pedantic-debug-assertions")] - debug_assert!(!pc.contains(ptr)); + debug_assert!(!pc.iter().contains(ptr)); debug_assert!(counter_marker.is_not_marked()); - // Mark it counter_marker.mark(Mark::PossibleCycles); } - // Add to the list - // - // Make sure this operation is the first after the if-else, since the CcBox is in - // an invalid state now (it's marked Mark::PossibleCycles, but it isn't into the list) + + #[cfg(debug_assertions)] // pc.add(...) may panic in debug builds + let drop_guard = ResetMarkDropGuard::new(ptr); + pc.add(ptr); + + #[cfg(debug_assertions)] + mem::forget(drop_guard); }); } @@ -609,115 +599,58 @@ impl CcBox<()> { } } - pub(super) fn start_tracing(ptr: NonNull, ctx: &mut Context<'_>) { - let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); - match ctx.inner() { - ContextInner::Counting { root_list, .. } => { - // ptr is NOT into POSSIBLE_CYCLES list: ptr has just been removed from - // POSSIBLE_CYCLES by rust_cc::collect() (see lib.rs) before calling this function - - root_list.add(ptr); - - // Reset trace_counter - counter_marker.reset_tracing_counter(); - - // Element is surely not already marked, marking - counter_marker.mark(Mark::InList); - }, - ContextInner::RootTracing { .. } => { - // ptr is a root - - // Nothing to do here, ptr is already unmarked - debug_assert!(counter_marker.is_not_marked()); - }, - } - - // ptr is surely to trace - // - // This function is called from collect_cycles(), which doesn't know the - // exact type of the element inside CcBox, so trace it using the vtable - CcBox::trace_inner(ptr, ctx); - } - - /// Returns whether `ptr.elem` should be traced. - /// - /// This function returns a `bool` instead of directly tracing the element inside the CcBox, since this way - /// we can avoid using the vtable most of the times (the responsibility of tracing the inner element is passed - /// to the caller, which *might* have more information on the type inside CcBox than us). #[inline(never)] // Don't inline this function, it's huge - #[must_use = "the element inside ptr is not traced by CcBox::trace"] - fn trace(ptr: NonNull, ctx: &mut Context<'_>) -> bool { - #[inline(always)] - fn non_root(counter_marker: &CounterMarker) -> bool { - counter_marker.tracing_counter() == counter_marker.counter() - } - + fn trace(ptr: NonNull, ctx: &mut Context<'_>) { let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); match ctx.inner() { ContextInner::Counting { + possible_cycles, root_list, non_root_list, + queue, } => { - if !counter_marker.is_in_list() { - // Not already marked - - // Make sure ptr is not in POSSIBLE_CYCLES list - remove_from_list(ptr); + if counter_marker.is_in_list_or_queue() { + // Check counters invariant (tracing_counter is always less or equal to counter) + // Only < is used here since tracing_counter will be incremented (by 1) + debug_assert!(counter_marker.tracing_counter() < counter_marker.counter()); - counter_marker.reset_tracing_counter(); let res = counter_marker.increment_tracing_counter(); debug_assert!(res.is_ok()); - // Check invariant (tracing_counter is always less or equal to counter) - debug_assert!(counter_marker.tracing_counter() <= counter_marker.counter()); + if counter_marker.is_in_list() && counter_marker.counter() == counter_marker.tracing_counter() { + // ptr is in root_list - if non_root(counter_marker) { + #[cfg(feature = "pedantic-debug-assertions")] + debug_assert!(root_list.iter().contains(ptr)); + + root_list.remove(ptr); non_root_list.add(ptr); - } else { - root_list.add(ptr); } - - // Marking here since the previous debug_asserts might panic - // before ptr is actually added to root_list or non_root_list - counter_marker.mark(Mark::InList); - - // Continue tracing - true } else { - // Check counters invariant (tracing_counter is always less or equal to counter) - // Only < is used here since tracing_counter will be incremented (by 1) - debug_assert!(counter_marker.tracing_counter() < counter_marker.counter()); + if counter_marker.is_in_possible_cycles() { + counter_marker.mark(Mark::NonMarked); + possible_cycles.remove(ptr); + } + counter_marker.reset_tracing_counter(); let res = counter_marker.increment_tracing_counter(); debug_assert!(res.is_ok()); - if non_root(counter_marker) { - // Already marked, so ptr was put in root_list - root_list.remove(ptr); - non_root_list.add(ptr); - } - - // Don't continue tracing - false + queue.add(ptr); + counter_marker.mark(Mark::InQueue); } }, - ContextInner::RootTracing { non_root_list, root_list } => { - if counter_marker.is_in_list() { - // Marking NonMarked since ptr will be removed from any list it's into. Also, marking - // NonMarked will avoid tracing this CcBox again (thanks to the if condition) - counter_marker.mark(Mark::NonMarked); + ContextInner::RootTracing { non_root_list, queue } => { + if counter_marker.is_in_list() && counter_marker.counter() == counter_marker.tracing_counter() { + // ptr is in non_root_list - if non_root(counter_marker) { - non_root_list.remove(ptr); - } else { - root_list.remove(ptr); - } + #[cfg(feature = "pedantic-debug-assertions")] + debug_assert!(non_root_list.iter().contains(ptr)); - // Continue root tracing - true - } else { - // Don't continue tracing - false + counter_marker.mark(Mark::NonMarked); + non_root_list.remove(ptr); + queue.add(ptr); + counter_marker.mark(Mark::InQueue); } }, } diff --git a/src/counter_marker.rs b/src/counter_marker.rs index 66d74b3..b0a2a3b 100644 --- a/src/counter_marker.rs +++ b/src/counter_marker.rs @@ -5,7 +5,6 @@ use crate::utils; const NON_MARKED: u32 = 0u32; const IN_POSSIBLE_CYCLES: u32 = 1u32 << (u32::BITS - 2); const IN_LIST: u32 = 2u32 << (u32::BITS - 2); -#[allow(dead_code)] // TODO Remove when actually used const IN_QUEUE: u32 = 3u32 << (u32::BITS - 2); const COUNTER_MASK: u32 = 0b11111111111111u32; // First 14 bits set to 1 @@ -187,9 +186,8 @@ impl CounterMarker { (self.counter.get() & BITS_MASK) == IN_LIST } - #[allow(dead_code)] // TODO Remove when actually used #[inline] - pub(crate) fn is_in_queue(&self) -> bool { + pub(crate) fn _is_in_queue(&self) -> bool { (self.counter.get() & BITS_MASK) == IN_QUEUE } @@ -222,6 +220,5 @@ pub(crate) enum Mark { NonMarked = NON_MARKED, PossibleCycles = IN_POSSIBLE_CYCLES, InList = IN_LIST, - #[allow(dead_code)] // TODO Remove when actually used InQueue = IN_QUEUE, } diff --git a/src/lib.rs b/src/lib.rs index bb0225e..6193365 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -281,13 +281,10 @@ fn __collect(state: &State, possible_cycles: &PossibleCycles) { let mut non_root_list = LinkedList::new(); { let mut root_list = LinkedList::new(); + let mut queue = LinkedQueue::new(); - while let Some(ptr) = possible_cycles.remove_first() { - // remove_first already marks ptr as NonMarked - trace_counting(ptr, &mut root_list, &mut non_root_list); - } - - trace_roots(root_list, &mut non_root_list); + trace_counting(possible_cycles, &mut root_list, &mut non_root_list, &mut queue); + trace_roots(root_list, &mut non_root_list, queue); } if !non_root_list.is_empty() { @@ -439,23 +436,88 @@ fn deallocate_list(to_deallocate_list: LinkedList, state: &State) { } fn trace_counting( + possible_cycles: &PossibleCycles, + root_list: &mut LinkedList, + non_root_list: &mut LinkedList, + queue: &mut LinkedQueue, +) { + while let Some(ptr) = possible_cycles.remove_first() { + // Reset the tracing counter before tracing + unsafe { ptr.as_ref() }.counter_marker().reset_tracing_counter(); + __trace_counting(ptr, possible_cycles, root_list, non_root_list, queue); + } + + while let Some(ptr) = queue.poll() { + // The tracing counter has already been reset by CcBox::trace when ptr was inserted into the queue + __trace_counting(ptr, possible_cycles, root_list, non_root_list, queue); + } + + debug_assert!(possible_cycles.is_empty()); + debug_assert!(queue.is_empty()); +} + +fn __trace_counting( ptr: NonNull>, + possible_cycles: &PossibleCycles, root_list: &mut LinkedList, non_root_list: &mut LinkedList, + queue: &mut LinkedQueue, ) { + let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); + + // Mark as InQueue so that CcBox::trace will only increment the tracing counter + counter_marker.mark(Mark::InQueue); + + // Reset the mark if a panic happens during tracing + let drop_guard = ResetMarkDropGuard::new(ptr); + let mut ctx = Context::new(ContextInner::Counting { + possible_cycles, root_list, non_root_list, + queue, }); + CcBox::trace_inner(ptr, &mut ctx); - CcBox::start_tracing(ptr, &mut ctx); + if counter_marker.counter() == counter_marker.tracing_counter() { + non_root_list.add(ptr); + } else { + root_list.add(ptr); + } + + mem::forget(drop_guard); + counter_marker.mark(Mark::InList); } -fn trace_roots(mut root_list: LinkedList, non_root_list: &mut LinkedList) { +fn trace_roots( + mut root_list: LinkedList, + non_root_list: &mut LinkedList, + mut queue: LinkedQueue, +) { while let Some(ptr) = root_list.remove_first() { - let mut ctx = Context::new(ContextInner::RootTracing { non_root_list, root_list: &mut root_list }); - CcBox::start_tracing(ptr, &mut ctx); + __trace_roots(ptr, non_root_list, &mut queue); } - mem::forget(root_list); // root_list is empty, no need run List::drop + while let Some(ptr) = queue.poll() { + __trace_roots(ptr, non_root_list, &mut queue); + } + + debug_assert!(queue.is_empty()); + debug_assert!(root_list.is_empty()); + + mem::forget(root_list); // No need to run its destructor, it's already empty + mem::forget(queue); // No need to run its destructor, it's already empty +} + +fn __trace_roots( + ptr: NonNull>, + non_root_list: &mut LinkedList, + queue: &mut LinkedQueue, +) { + let mut ctx = Context::new(ContextInner::RootTracing { + non_root_list, + queue, + }); + + CcBox::trace_inner(ptr, &mut ctx); } diff --git a/src/lists.rs b/src/lists.rs index 1240867..2ba1fd6 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -143,6 +143,14 @@ pub(crate) struct Iter<'a> { _phantom: PhantomData<&'a CcBox<()>>, } +impl Iter<'_> { + #[inline] + #[cfg(any(feature = "pedantic-debug-assertions", all(test, feature = "std")))] // Only used in pedantic-debug-assertions or unit tests + pub(crate) fn contains(mut self, ptr: NonNull>) -> bool { + self.any(|elem| elem == ptr) + } +} + impl<'a> Iterator for Iter<'a> { type Item = NonNull>; @@ -290,12 +298,6 @@ impl PossibleCycles { self.first().is_none() } - #[inline] - #[cfg(any(feature = "pedantic-debug-assertions", all(test, feature = "std")))] // Only used in pedantic-debug-assertions or unit tests - pub(crate) fn contains(&self, ptr: NonNull>) -> bool { - self.iter().any(|elem| elem == ptr) - } - /// # Safety /// * The elements in `to_append` must be already marked with `mark` mark /// * `to_append_size` must be the size of `to_append` @@ -366,13 +368,11 @@ impl<'a> IntoIterator for &'a PossibleCycles { } } -#[allow(dead_code)] // TODO Remove when actually used pub(crate) struct LinkedQueue { first: Option>>, last: Option>>, } -#[allow(dead_code)] // TODO Remove when actually used impl LinkedQueue { #[inline] pub(crate) const fn new() -> Self { diff --git a/src/tests/cc.rs b/src/tests/cc.rs index 0a63f07..04eae1b 100644 --- a/src/tests/cc.rs +++ b/src/tests/cc.rs @@ -145,11 +145,12 @@ fn test_trait_object() { static TRACED: Cell = Cell::new(false); } - struct MyTraitObject(u8); + struct MyTraitObject(u8, RefCell>>); unsafe impl Trace for MyTraitObject { fn trace(&self, ctx: &mut Context<'_>) { self.0.trace(ctx); + self.1.trace(ctx); TRACED.with(|traced| traced.set(true)); } } @@ -177,27 +178,25 @@ fn test_trait_object() { } { - let cc = Cc::new(MyTraitObject(5)) as Cc; + let cc = Cc::new(MyTraitObject(5, RefCell::new(None))); + *cc.1.borrow_mut() = Some(cc.clone()); - assert_eq!(cc.strong_count(), 1); + let cc: Cc = cc; - let cc_cloned = cc.clone(); - assert_eq!(cc_cloned.strong_count(), 2); assert_eq!(cc.strong_count(), 2); + let cc_cloned = cc.clone(); + assert_eq!(cc_cloned.strong_count(), 3); + assert_eq!(cc.strong_count(), 3); + drop(cc_cloned); - assert_eq!(cc.strong_count(), 1); + assert_eq!(cc.strong_count(), 2); let inner = cc.deref(); inner.hello(); - let mut l1 = LinkedList::new(); - let mut l2 = LinkedList::new(); - - cc.trace(&mut Context::new(ContextInner::Counting { - root_list: &mut l1, - non_root_list: &mut l2, - })); + drop(cc); + collect_cycles(); } assert!( @@ -216,7 +215,7 @@ fn test_trait_object() { #[cfg(feature = "finalization")] assert!( FINALIZED.with(|dropped| dropped.get()), - "MyTraitObject hasn't been traced" + "MyTraitObject hasn't been finalized" ); } diff --git a/src/tests/counter_marker.rs b/src/tests/counter_marker.rs index 5c04dc0..f10797c 100644 --- a/src/tests/counter_marker.rs +++ b/src/tests/counter_marker.rs @@ -4,7 +4,7 @@ fn assert_not_marked(counter: &CounterMarker) { assert!(counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(!counter.is_in_list()); - assert!(!counter.is_in_queue()); + assert!(!counter._is_in_queue()); assert!(!counter.is_in_list_or_queue()); assert!(!counter.is_in_list_or_queue()); } @@ -225,7 +225,7 @@ fn test_marks() { assert!(counter.is_not_marked()); assert!(counter.is_in_possible_cycles()); assert!(!counter.is_in_list()); - assert!(!counter.is_in_queue()); + assert!(!counter._is_in_queue()); assert!(!counter.is_in_list_or_queue()); assert_default_settings(&counter); @@ -234,7 +234,7 @@ fn test_marks() { assert!(!counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(counter.is_in_list()); - assert!(!counter.is_in_queue()); + assert!(!counter._is_in_queue()); assert!(counter.is_in_list_or_queue()); assert_default_settings(&counter); @@ -243,7 +243,7 @@ fn test_marks() { assert!(!counter.is_not_marked()); assert!(!counter.is_in_possible_cycles()); assert!(!counter.is_in_list()); - assert!(counter.is_in_queue()); + assert!(counter._is_in_queue()); assert!(counter.is_in_list_or_queue()); assert_default_settings(&counter); diff --git a/src/tests/lists.rs b/src/tests/lists.rs index 48f8dbf..28aaf3e 100644 --- a/src/tests/lists.rs +++ b/src/tests/lists.rs @@ -13,7 +13,7 @@ use crate::utils::cc_dealloc; fn assert_contains(list: &impl ListMethods, mut elements: Vec) { list.iter().for_each(|ptr| { // Test contains - assert!(list.contains(ptr)); + assert!(list.iter().contains(ptr)); let elem = unsafe { *ptr.cast::>().as_ref().get_elem() }; let index = elements.iter().position(|&i| i == elem); @@ -389,8 +389,6 @@ trait ListMethods: CommonMethods { fn iter(&self) -> Iter; - fn contains(&self, ptr: NonNull>) -> bool; - fn assert_size(&self, expected_size: usize); } @@ -421,10 +419,6 @@ impl ListMethods for LinkedList { self.iter() } - fn contains(&self, ptr: NonNull>) -> bool { - self.iter().any(|elem| elem == ptr) - } - fn assert_size(&self, expected_size: usize) { assert_eq!(expected_size, self.iter().count()); } @@ -457,10 +451,6 @@ impl ListMethods for PossibleCycles { self.iter() } - fn contains(&self, ptr: NonNull>) -> bool { - self.contains(ptr) - } - fn assert_size(&self, expected_size: usize) { assert_eq!(expected_size, self.size()); } @@ -490,7 +480,7 @@ mod queue { fn assert_queue_contains(queue: &LinkedQueue, mut elements: Vec) { queue.into_iter().for_each(|ptr| { // Test contains - assert!(queue.into_iter().any(|elem| elem == ptr)); + assert!(queue.into_iter().contains(ptr)); let elem = unsafe { *ptr.cast::>().as_ref().get_elem() }; let index = elements.iter().position(|&i| i == elem); diff --git a/src/trace.rs b/src/trace.rs index acb0b90..aa969a5 100644 --- a/src/trace.rs +++ b/src/trace.rs @@ -21,7 +21,7 @@ use std::{ ffi::{OsStr, OsString} }; -use crate::lists::LinkedList; +use crate::lists::{LinkedList, LinkedQueue, PossibleCycles}; /// Trait to finalize objects before freeing them. /// @@ -138,12 +138,14 @@ pub struct Context<'a> { pub(crate) enum ContextInner<'a> { Counting { + possible_cycles: &'a PossibleCycles, root_list: &'a mut LinkedList, non_root_list: &'a mut LinkedList, + queue: &'a mut LinkedQueue, }, RootTracing { - root_list: &'a mut LinkedList, non_root_list: &'a mut LinkedList, + queue: &'a mut LinkedQueue, }, } diff --git a/src/utils.rs b/src/utils.rs index 5277eb5..ff7a8c3 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -2,6 +2,7 @@ use alloc::alloc::{alloc, dealloc, handle_alloc_error, Layout}; use core::ptr::NonNull; use crate::{CcBox, Trace}; +use crate::counter_marker::Mark; use crate::state::State; #[inline] @@ -44,6 +45,24 @@ pub(crate) unsafe fn dealloc_other(ptr: NonNull) { #[cold] pub(crate) fn cold() {} +pub(crate) struct ResetMarkDropGuard { + ptr: NonNull>, +} + +impl ResetMarkDropGuard { + #[inline] + pub(crate) fn new(ptr: NonNull>) -> Self { + Self { ptr } + } +} + +impl Drop for ResetMarkDropGuard { + #[inline] + fn drop(&mut self) { + unsafe { self.ptr.as_ref() }.counter_marker().mark(Mark::NonMarked); + } +} + #[cfg(feature = "std")] pub(crate) use std::thread_local as rust_cc_thread_local; // Use the std's macro when std is enabled From e663508b39127f1295a254dab6c4e43c3770bb11 Mon Sep 17 00:00:00 2001 From: fren_gor Date: Sat, 3 Aug 2024 00:13:13 +0200 Subject: [PATCH 07/13] Derive `SmartPointer` for `Cc` --- src/cc.rs | 14 +++----------- src/lib.rs | 2 +- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/cc.rs b/src/cc.rs index 1f2d75c..2938485 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -13,8 +13,7 @@ use core::hash::{Hash, Hasher}; use core::panic::{RefUnwindSafe, UnwindSafe}; #[cfg(feature = "nightly")] use core::{ - marker::Unsize, - ops::CoerceUnsized, + marker::SmartPointer, ptr::{metadata, DynMetadata}, }; @@ -30,20 +29,13 @@ use crate::weak::weak_counter_marker::WeakCounterMarker; /// A thread-local cycle collected pointer. /// /// See the [module-level documentation][`mod@crate`] for more details. +#[cfg_attr(feature = "nightly", derive(SmartPointer))] #[repr(transparent)] -pub struct Cc { +pub struct Cc<#[cfg_attr(feature = "nightly", pointee)] T: ?Sized + Trace + 'static> { inner: NonNull>, _phantom: PhantomData>, // Make Cc !Send and !Sync } -#[cfg(feature = "nightly")] -impl CoerceUnsized> for Cc -where - T: ?Sized + Trace + Unsize + 'static, - U: ?Sized + Trace + 'static, -{ -} - impl Cc { /// Creates a new `Cc`. /// diff --git a/src/lib.rs b/src/lib.rs index 67221c6..a671421 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -131,7 +131,7 @@ cleanable.clean(); //! [`Sync`]: `std::marker::Sync` //! [`Rc`]: `std::rc::Rc` -#![cfg_attr(feature = "nightly", feature(unsize, coerce_unsized, ptr_metadata))] +#![cfg_attr(feature = "nightly", feature(unsize, coerce_unsized, ptr_metadata, derive_smart_pointer))] #![cfg_attr(all(feature = "nightly", not(feature = "std")), feature(thread_local))] // no-std related unstable features #![cfg_attr(doc_auto_cfg, feature(doc_auto_cfg))] #![cfg_attr(not(feature = "std"), no_std)] From b74fc58c75b49855dfdf7e31f726073892647dce Mon Sep 17 00:00:00 2001 From: fren_gor Date: Sun, 4 Aug 2024 06:28:31 +0200 Subject: [PATCH 08/13] Reset the TC when objs enter possible cycles --- src/cc.rs | 7 ++++--- src/lib.rs | 9 +++------ src/lists.rs | 1 + src/trace.rs | 3 +-- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/cc.rs b/src/cc.rs index eed3aab..4f135f8 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -534,6 +534,7 @@ pub(crate) fn add_to_list(ptr: NonNull>) { debug_assert!(counter_marker.is_not_marked()); + counter_marker.reset_tracing_counter(); counter_marker.mark(Mark::PossibleCycles); } @@ -604,7 +605,6 @@ impl CcBox<()> { let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); match ctx.inner() { ContextInner::Counting { - possible_cycles, root_list, non_root_list, queue, @@ -628,8 +628,9 @@ impl CcBox<()> { } } else { if counter_marker.is_in_possible_cycles() { - counter_marker.mark(Mark::NonMarked); - possible_cycles.remove(ptr); + let res = counter_marker.increment_tracing_counter(); + debug_assert!(res.is_ok()); + return; } counter_marker.reset_tracing_counter(); diff --git a/src/lib.rs b/src/lib.rs index 6193365..c1b05bb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -442,14 +442,13 @@ fn trace_counting( queue: &mut LinkedQueue, ) { while let Some(ptr) = possible_cycles.remove_first() { - // Reset the tracing counter before tracing - unsafe { ptr.as_ref() }.counter_marker().reset_tracing_counter(); - __trace_counting(ptr, possible_cycles, root_list, non_root_list, queue); + // The tracing counter has already been reset by add_to_list(...) + __trace_counting(ptr, root_list, non_root_list, queue); } while let Some(ptr) = queue.poll() { // The tracing counter has already been reset by CcBox::trace when ptr was inserted into the queue - __trace_counting(ptr, possible_cycles, root_list, non_root_list, queue); + __trace_counting(ptr, root_list, non_root_list, queue); } debug_assert!(possible_cycles.is_empty()); @@ -458,7 +457,6 @@ fn trace_counting( fn __trace_counting( ptr: NonNull>, - possible_cycles: &PossibleCycles, root_list: &mut LinkedList, non_root_list: &mut LinkedList, queue: &mut LinkedQueue, @@ -472,7 +470,6 @@ fn __trace_counting( let drop_guard = ResetMarkDropGuard::new(ptr); let mut ctx = Context::new(ContextInner::Counting { - possible_cycles, root_list, non_root_list, queue, diff --git a/src/lists.rs b/src/lists.rs index 2ba1fd6..30b27a6 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -307,6 +307,7 @@ impl PossibleCycles { if let Some(mut prev) = self.first.get() { for elem in self.iter() { unsafe { + elem.as_ref().counter_marker().reset_tracing_counter(); elem.as_ref().counter_marker().mark(mark); } prev = elem; diff --git a/src/trace.rs b/src/trace.rs index aa969a5..134b437 100644 --- a/src/trace.rs +++ b/src/trace.rs @@ -21,7 +21,7 @@ use std::{ ffi::{OsStr, OsString} }; -use crate::lists::{LinkedList, LinkedQueue, PossibleCycles}; +use crate::lists::{LinkedList, LinkedQueue}; /// Trait to finalize objects before freeing them. /// @@ -138,7 +138,6 @@ pub struct Context<'a> { pub(crate) enum ContextInner<'a> { Counting { - possible_cycles: &'a PossibleCycles, root_list: &'a mut LinkedList, non_root_list: &'a mut LinkedList, queue: &'a mut LinkedQueue, From 57f736fd051da7a4b4dfe626916685fb11f66b67 Mon Sep 17 00:00:00 2001 From: fren_gor Date: Tue, 6 Aug 2024 05:14:13 +0200 Subject: [PATCH 09/13] Use two `Cell` in CounterMarker This improves performance when weak pointers are enabled. --- src/cc.rs | 2 +- src/counter_marker.rs | 89 +++++++++++++++++++++---------------------- src/weak/mod.rs | 2 +- 3 files changed, 46 insertions(+), 47 deletions(-) diff --git a/src/cc.rs b/src/cc.rs index 4f135f8..755265b 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -114,7 +114,7 @@ impl Cc { /// Returns the number of [`Cc`]s to the pointed allocation. #[inline] pub fn strong_count(&self) -> u32 { - self.counter_marker().counter() + self.counter_marker().counter() as u32 } /// Returns `true` if the strong reference count is `1`, `false` otherwise. diff --git a/src/counter_marker.rs b/src/counter_marker.rs index b0a2a3b..6391f38 100644 --- a/src/counter_marker.rs +++ b/src/counter_marker.rs @@ -2,29 +2,28 @@ use core::cell::Cell; use crate::utils; -const NON_MARKED: u32 = 0u32; -const IN_POSSIBLE_CYCLES: u32 = 1u32 << (u32::BITS - 2); -const IN_LIST: u32 = 2u32 << (u32::BITS - 2); -const IN_QUEUE: u32 = 3u32 << (u32::BITS - 2); +const NON_MARKED: u16 = 0u16; +const IN_POSSIBLE_CYCLES: u16 = 1u16 << (u16::BITS - 2); +const IN_LIST: u16 = 2u16 << (u16::BITS - 2); +const IN_QUEUE: u16 = 3u16 << (u16::BITS - 2); -const COUNTER_MASK: u32 = 0b11111111111111u32; // First 14 bits set to 1 -const TRACING_COUNTER_MASK: u32 = COUNTER_MASK << 14; // 14 bits set to 1 followed by 14 bits set to 0 -const FINALIZED_MASK: u32 = 1u32 << (u32::BITS - 4); -const METADATA_MASK: u32 = 1u32 << (u32::BITS - 3); -const BITS_MASK: u32 = !(COUNTER_MASK | TRACING_COUNTER_MASK | FINALIZED_MASK | METADATA_MASK); -const FIRST_BIT_MASK: u32 = 1u32 << (u32::BITS - 1); +const COUNTER_MASK: u16 = 0b11111111111111u16; // First 14 bits set to 1 +const FIRST_BIT_MASK: u16 = 1u16 << (u16::BITS - 1); +const FINALIZED_MASK: u16 = 1u16 << (u16::BITS - 2); +const BITS_MASK: u16 = !COUNTER_MASK; -const INITIAL_VALUE: u32 = COUNTER_MASK + 2; // +2 means that tracing counter and counter are both set to 1 -const INITIAL_VALUE_FINALIZED: u32 = INITIAL_VALUE | FINALIZED_MASK; +const INITIAL_VALUE: u16 = 1u16; +const INITIAL_VALUE_TRACING_COUNTER: u16 = INITIAL_VALUE | NON_MARKED; +const INITIAL_VALUE_FINALIZED: u16 = INITIAL_VALUE | FINALIZED_MASK; // pub(crate) to make it available in tests -pub(crate) const MAX: u32 = COUNTER_MASK - 1; +pub(crate) const MAX: u16 = COUNTER_MASK - 1; /// Internal representation: /// ```text -/// +-----------+----------+----------+------------+------------+ -/// | A: 2 bits | B: 1 bit | C: 1 bit | D: 14 bits | E: 14 bits | Total: 32 bits -/// +-----------+----------+----------+------------+------------+ +/// +-----------+------------+ +----------+----------+------------+ +/// | A: 2 bits | B: 14 bits | | C: 1 bit | D: 1 bit | E: 14 bits | Total: 32 bits (16 + 16) +/// +-----------+------------+ +----------+----------+------------+ /// ``` /// /// * `A` has 4 possible states: @@ -32,16 +31,15 @@ pub(crate) const MAX: u32 = COUNTER_MASK - 1; /// * `IN_POSSIBLE_CYCLES`: in `possible_cycles` list (implies `NON_MARKED`) /// * `IN_LIST`: in `root_list` or `non_root_list` /// * `IN_QUEUE`: in queue to be traced -/// * `B` is `1` when metadata has been allocated, `0` otherwise -/// * `C` is `1` when the element inside `CcBox` has already been finalized, `0` otherwise -/// * `D` is the tracing counter. The max value (the one with every bit set to 1) is reserved +/// * `B` is the tracing counter. The max value (the one with every bit set to 1) is reserved /// and indicates that the allocated value has already been dropped (but not yet deallocated) -/// * `E` is the reference counter (last one for sum/subtraction efficiency). The max value (the -/// one with every bit set to 1) is reserved and should not be used +/// * `C` is `1` when metadata has been allocated, `0` otherwise +/// * `D` is `1` when the element inside `CcBox` has already been finalized, `0` otherwise +/// * `E` is the reference counter. The max value (the one with every bit set to 1) is reserved and should not be used #[derive(Clone, Debug)] -#[repr(transparent)] pub(crate) struct CounterMarker { - counter: Cell, + tracing_counter: Cell, + counter: Cell, } pub(crate) struct OverflowError; @@ -51,6 +49,7 @@ impl CounterMarker { #[must_use] pub(crate) fn new_with_counter_to_one(already_finalized: bool) -> CounterMarker { CounterMarker { + tracing_counter: Cell::new(INITIAL_VALUE_TRACING_COUNTER), counter: Cell::new(if !already_finalized { INITIAL_VALUE } else { @@ -94,7 +93,7 @@ impl CounterMarker { Err(OverflowError) } else { // Increment trace_counter and not counter - self.counter.set(self.counter.get() + (1u32 << 14)); + self.tracing_counter.set(self.tracing_counter.get() + 1); Ok(()) } } @@ -108,21 +107,21 @@ impl CounterMarker { Err(OverflowError) } else { // Decrement trace_counter and not counter - self.counter.set(self.counter.get() - (1u32 << 14)); + self.tracing_counter.set(self.tracing_counter.get() - 1); Ok(()) } } #[inline] - pub(crate) fn counter(&self) -> u32 { + pub(crate) fn counter(&self) -> u16 { let rc = self.counter.get() & COUNTER_MASK; debug_assert!(rc != COUNTER_MASK); // Check for reserved value rc } #[inline] - pub(crate) fn tracing_counter(&self) -> u32 { - let tc = (self.counter.get() & TRACING_COUNTER_MASK) >> 14; + pub(crate) fn tracing_counter(&self) -> u16 { + let tc = self.tracing_counter.get() & COUNTER_MASK; debug_assert!(tc != COUNTER_MASK); // Check for reserved value tc } @@ -130,92 +129,92 @@ impl CounterMarker { #[inline] pub(crate) fn reset_tracing_counter(&self) { debug_assert!(self.tracing_counter() != COUNTER_MASK); // Check for reserved value - self.counter.set(self.counter.get() & !TRACING_COUNTER_MASK); + self.tracing_counter.set(self.tracing_counter.get() & !COUNTER_MASK); } #[cfg(feature = "finalization")] #[inline] pub(crate) fn needs_finalization(&self) -> bool { - (self.counter.get() & FINALIZED_MASK) == 0u32 + (self.counter.get() & FINALIZED_MASK) == 0u16 } #[cfg(feature = "finalization")] #[inline] pub(crate) fn set_finalized(&self, finalized: bool) { - self.set_bits(finalized, FINALIZED_MASK); + Self::set_bits(&self.counter, finalized, FINALIZED_MASK); } #[cfg(feature = "weak-ptrs")] #[inline] pub(crate) fn has_allocated_for_metadata(&self) -> bool { - (self.counter.get() & METADATA_MASK) == METADATA_MASK + (self.counter.get() & FIRST_BIT_MASK) == FIRST_BIT_MASK } #[cfg(feature = "weak-ptrs")] #[inline] pub(crate) fn set_allocated_for_metadata(&self, allocated_for_metadata: bool) { - self.set_bits(allocated_for_metadata, METADATA_MASK); + Self::set_bits(&self.counter, allocated_for_metadata, FIRST_BIT_MASK); } #[cfg(feature = "weak-ptrs")] #[inline] pub(crate) fn is_dropped(&self) -> bool { - (self.counter.get() & TRACING_COUNTER_MASK) == TRACING_COUNTER_MASK + (self.tracing_counter.get() & COUNTER_MASK) == COUNTER_MASK } #[cfg(feature = "weak-ptrs")] #[inline] pub(crate) fn set_dropped(&self, dropped: bool) { - self.set_bits(dropped, TRACING_COUNTER_MASK); + Self::set_bits(&self.tracing_counter, dropped, COUNTER_MASK); } #[inline] pub(crate) fn is_not_marked(&self) -> bool { // true if (self.counter & BITS_MASK) is equal to 01 or 00, // so if the first bit is 0 - (self.counter.get() & FIRST_BIT_MASK) == 0u32 + (self.tracing_counter.get() & FIRST_BIT_MASK) == 0u16 } #[inline] pub(crate) fn is_in_possible_cycles(&self) -> bool { - (self.counter.get() & BITS_MASK) == IN_POSSIBLE_CYCLES + (self.tracing_counter.get() & BITS_MASK) == IN_POSSIBLE_CYCLES } #[inline] pub(crate) fn is_in_list(&self) -> bool { - (self.counter.get() & BITS_MASK) == IN_LIST + (self.tracing_counter.get() & BITS_MASK) == IN_LIST } #[inline] pub(crate) fn _is_in_queue(&self) -> bool { - (self.counter.get() & BITS_MASK) == IN_QUEUE + (self.tracing_counter.get() & BITS_MASK) == IN_QUEUE } #[inline] pub(crate) fn is_in_list_or_queue(&self) -> bool { // true if (self.counter & BITS_MASK) is equal to 10 or 11, // so if the first bit is 1 - (self.counter.get() & FIRST_BIT_MASK) == FIRST_BIT_MASK + (self.tracing_counter.get() & FIRST_BIT_MASK) == FIRST_BIT_MASK } #[inline] pub(crate) fn mark(&self, new_mark: Mark) { - self.counter.set((self.counter.get() & !BITS_MASK) | (new_mark as u32)); + self.tracing_counter.set((self.tracing_counter.get() & !BITS_MASK) | (new_mark as u16)); } #[cfg(any(feature = "weak-ptrs", feature = "finalization"))] #[inline(always)] - fn set_bits(&self, value: bool, mask: u32) { + fn set_bits(cell: &Cell, value: bool, mask: u16) { if value { - self.counter.set(self.counter.get() | mask); + cell.set(cell.get() | mask); } else { - self.counter.set(self.counter.get() & !mask); + cell.set(cell.get() & !mask); } } } #[derive(Copy, Clone, Debug)] -#[repr(u32)] +#[repr(u16)] pub(crate) enum Mark { NonMarked = NON_MARKED, PossibleCycles = IN_POSSIBLE_CYCLES, diff --git a/src/weak/mod.rs b/src/weak/mod.rs index 64ac7a7..361a1b1 100644 --- a/src/weak/mod.rs +++ b/src/weak/mod.rs @@ -119,7 +119,7 @@ impl Weak { ) { 0 } else { - counter + counter as u32 } } else { 0 From 5d63639e46df447540c7ac52d6d2ef99ff941a3a Mon Sep 17 00:00:00 2001 From: fren_gor Date: Tue, 6 Aug 2024 06:16:29 +0200 Subject: [PATCH 10/13] Update CONTRIBUTING.md --- CONTRIBUTING.md | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4ec4f4a..f782443 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -10,7 +10,7 @@ Also, remember to open the pull requests toward the `dev` branch. The `main` bra The core idea behind the algorithm is the same as the one presented by Lins in ["Cyclic Reference Counting With Lazy Mark-Scan"](https://kar.kent.ac.uk/22347/1/CyclicLin.pdf) and by Bacon and Rajan in ["Concurrent Cycle Collection in Reference Counted Systems"](https://pages.cs.wisc.edu/~cymen/misc/interests/Bacon01Concurrent.pdf). -However, the implementation differs in order to make the collector faster and more resilient to random panics and failures in general. +However, the implementation differs in order to make the collector faster and more resilient to panics and failures. > [!IMPORTANT] > `rust-cc` is *not* strictly an implementation of the algorithm shown in the linked papers and it's never been @@ -23,24 +23,33 @@ The `POSSIBLE_CYCLES` is an (intrusive) list which contains the possible roots o Sometimes (see [`crate::trigger_collection`](./src/lib.rs)), when creating a new `Cc` or when `collect_cycles` is called, the objects inside the `POSSIBLE_CYCLES` list are checked to see if they are part of a garbage cycle. -Therefore, they undergo two tracing passes: -- **Trace Counting:** during this phase, starting from the elements inside `POSSIBLE_CYCLES`, - objects are traced to count the amount of pointers to each `CcBox` that is reachable from the list's `Cc`s. +Therefore, they undergo two tracing phases: +- **Trace Counting:** during this phase, a breadth-first traversal of the heap is performed starting from the elements inside + `POSSIBLE_CYCLES` to count the number of pointers to each `CcBox` that is reachable from the list's `Cc`s. The `tracing_counter` "field" (see the [`counter_marker` module](./src/counter_marker.rs) for more info) is used to keep track of this number.
About tracing_counter

In the papers, Lins, Bacon and Rajan decrement the RC itself instead of using another counter. However, if during tracing there was a panic, it would be hard for `rust-cc` to restore the RC correctly. This is the reason for the choice of having another counter. - The invariant regarding this second counter is that it must always be between 0 and RC inclusively. + The invariant regarding this second counter is that it must always be between 0 and RC (inclusively).

- **Trace Roots:** now, every `CcBox` which has the RC strictly greater than `tracing_counter` can be considered a root, - since it must exist a `Cc` pointer which points at it that hasn't been traced before. Thus, a trace is started from these roots, - and all objects not reached during this trace are finalized/deallocated (the story is more complicated because of possible + since it must exist a `Cc` pointer which points at it that hasn't been traced in the previous phase. Thus, a trace is started from these roots, + and all objects not reached during this trace are finalized/deallocated (this is actually more complex due to possible object resurrections, see the comments in [lib.rs](./src/lib.rs)). Note that this second phase is correct only if the graph formed by the pointers is not changed between the two phases. Thus, this is a key requirement of the `Trace` trait and one of the reasons it is marked `unsafe`. +## Using lists and queues + +When debug assertions are enabled, the `add` method may panic before actually updating the list to contain the added object. +Similarly, the `remove` method may panic after having removed the object from the list. + +Thus, marking should be done only: + - after the call to the `add` method + - before the call to the `remove` method. + ## Writing tests Every unit test should start with a call to `tests::reset_state()` to make sure errors in other tests don't impact the current one. From 9c2c926da4a6ea7a41ee690e1ba09beb783fabc2 Mon Sep 17 00:00:00 2001 From: fren_gor Date: Tue, 6 Aug 2024 06:22:30 +0200 Subject: [PATCH 11/13] Don't move objects in front of PC in `add_to_list` --- src/cc.rs | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/cc.rs b/src/cc.rs index 3609230..a1add6e 100644 --- a/src/cc.rs +++ b/src/cc.rs @@ -513,31 +513,27 @@ pub(crate) fn remove_from_list(ptr: NonNull>) { pub(crate) fn add_to_list(ptr: NonNull>) { let counter_marker = unsafe { ptr.as_ref() }.counter_marker(); - let _ = POSSIBLE_CYCLES.try_with(|pc| { - if counter_marker.is_in_possible_cycles() { - #[cfg(feature = "pedantic-debug-assertions")] - debug_assert!(pc.iter().contains(ptr)); - - pc.remove(ptr); - // Already marked - } else { + if !counter_marker.is_in_possible_cycles() { + let _ = POSSIBLE_CYCLES.try_with(|pc| { #[cfg(feature = "pedantic-debug-assertions")] debug_assert!(!pc.iter().contains(ptr)); debug_assert!(counter_marker.is_not_marked()); + pc.add(ptr); counter_marker.reset_tracing_counter(); counter_marker.mark(Mark::PossibleCycles); - } - - #[cfg(debug_assertions)] // pc.add(...) may panic in debug builds - let drop_guard = ResetMarkDropGuard::new(ptr); - - pc.add(ptr); + }); + } else { + // Already in the list - #[cfg(debug_assertions)] - mem::forget(drop_guard); - }); + #[cfg(feature = "pedantic-debug-assertions")] + debug_assert! { + POSSIBLE_CYCLES.try_with(|pc| { + pc.iter().contains(ptr) + }).unwrap_or(true) + }; + } } // Functions in common between every CcBox<_> From 94fa5fe491fcefa2379f244ec35783b08977b22f Mon Sep 17 00:00:00 2001 From: fren_gor Date: Tue, 6 Aug 2024 06:23:34 +0200 Subject: [PATCH 12/13] Update version to 0.5.0 --- Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index af35b42..05ef205 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,7 +14,7 @@ edition.workspace = true members = ["derive"] [workspace.package] -version = "0.4.0" # Also update in [dependencies.rust-cc-derive.version] +version = "0.5.0" # Also update in [dependencies.rust-cc-derive.version] authors = ["fren_gor "] repository = "https://github.com/frengor/rust-cc" categories = ["memory-management", "no-std"] @@ -49,7 +49,7 @@ std = ["slotmap?/std", "thiserror/std"] pedantic-debug-assertions = [] [dependencies] -rust-cc-derive = { path = "./derive", version = "=0.4.0", optional = true } +rust-cc-derive = { path = "./derive", version = "=0.5.0", optional = true } slotmap = { version = "1.0", optional = true } thiserror = { version = "1.0", package = "thiserror-core", default-features = false } From d2051fa2d0d1c9b5238f03d67e0ea0aed6147c9d Mon Sep 17 00:00:00 2001 From: fren_gor Date: Wed, 31 Jul 2024 07:06:46 +0200 Subject: [PATCH 13/13] Update iai-callgrind to 0.12.2 --- .github/workflows/bench.yml | 8 ++++++-- Cargo.toml | 6 +++++- benches/bench.rs | 7 +++++-- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 527b562..6c2d370 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -5,7 +5,6 @@ on: types: [labeled] env: - IAI_CALLGRIND_VERSION: 0.7.3 CARGO_TERM_COLOR: always IAI_CALLGRIND_COLOR: never CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse @@ -27,7 +26,12 @@ jobs: - uses: taiki-e/install-action@valgrind - uses: taiki-e/install-action@cargo-binstall - name: Install iai-callgrind-runner - run: cargo binstall --no-confirm --no-symlinks iai-callgrind-runner --version ${IAI_CALLGRIND_VERSION} + run: | + version=$(cargo metadata --format-version=1 |\ + jq '.packages[] | select(.name == "iai-callgrind").version' |\ + tr -d '"' + ) + cargo binstall --no-confirm --no-symlinks iai-callgrind-runner --version "${version}" - name: Bench base branch run: | cargo update diff --git a/Cargo.toml b/Cargo.toml index 05ef205..9ac93f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,7 +54,7 @@ slotmap = { version = "1.0", optional = true } thiserror = { version = "1.0", package = "thiserror-core", default-features = false } [dev-dependencies] -iai-callgrind = "=0.7.3" # Also update IAI_CALLGRIND_VERSION in .github/workflows/bench.yml +iai-callgrind = "=0.12.2" rand = "0.8.3" trybuild = "1.0.85" test-case = "3.3.1" @@ -64,6 +64,10 @@ name = "bench" harness = false required-features = ["std", "derive"] +[profile.bench] +debug = true # Required by iai-callgrind +strip = false # Required by iai-callgrind + [lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(doc_auto_cfg)'] } diff --git a/benches/bench.rs b/benches/bench.rs index 1cbd4ee..51bdbec 100644 --- a/benches/bench.rs +++ b/benches/bench.rs @@ -8,7 +8,7 @@ mod benches { } use std::hint::black_box; -use iai_callgrind::{library_benchmark, library_benchmark_group, main}; +use iai_callgrind::{library_benchmark, library_benchmark_group, LibraryBenchmarkConfig, main}; use crate::benches::binary_trees::count_binary_trees; use crate::benches::binary_trees_with_parent_pointers::count_binary_trees_with_parent; use crate::benches::large_linked_list::large_linked_list; @@ -53,4 +53,7 @@ library_benchmark_group!( benchmarks = large_linked_list_bench ); -main!(library_benchmark_groups = stress_tests_group, binary_trees_group, linked_lists_group); +main!( + config = LibraryBenchmarkConfig::default().raw_callgrind_args(["--branch-sim=yes"]); + library_benchmark_groups = stress_tests_group, binary_trees_group, linked_lists_group +);