Skip to content

Commit

Permalink
Remove incorrect asserts in Arena::mark_debt and Arena::mark_all.
Browse files Browse the repository at this point in the history
I think these methods were not properly updated after their semantics
changed at some point, they should do *nothing* when in the Collecting
phase, rather than either panic or (in release mode) potentially
incorrectly return a `MarkedArena`.

Adds a test to make sure the arena phases progress properly and that
these methods do nothing in the Collecting phase, as they advertise.
  • Loading branch information
kyren committed Apr 22, 2024
1 parent d3de3ab commit 42b901e
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 6 deletions.
6 changes: 2 additions & 4 deletions src/arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,7 @@ impl<R: for<'a> Rootable<'a>> Arena<R> {
}
}

debug_assert!(self.context.phase() == Phase::Mark);
if !self.context.gray_remaining() {
if self.context.phase() == Phase::Mark && !self.context.gray_remaining() {
Some(MarkedArena(self))
} else {
None
Expand Down Expand Up @@ -301,8 +300,7 @@ impl<R: for<'a> Rootable<'a>> Arena<R> {
}
}

debug_assert!(self.context.phase() == Phase::Mark);
if !self.context.gray_remaining() {
if self.context.phase() == Phase::Mark && !self.context.gray_remaining() {
Some(MarkedArena(self))
} else {
None
Expand Down
72 changes: 70 additions & 2 deletions tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use rand::distributions::Distribution;
use std::{collections::HashMap, rc::Rc};

use gc_arena::{
lock::RefLock, metrics::Pacing, unsafe_empty_collect, unsize, Arena, Collect, DynamicRootSet,
Gc, GcWeak, Rootable,
lock::RefLock, metrics::Pacing, unsafe_empty_collect, unsize, Arena, Collect, CollectionPhase,
DynamicRootSet, Gc, GcWeak, Rootable,
};

#[test]
Expand Down Expand Up @@ -909,6 +909,74 @@ fn transitive_death() {
});
}

#[test]
fn test_phases() {
#[derive(Collect)]
#[collect(no_drop)]
struct TestRoot<'gc> {
test: Gc<'gc, [u8; 1024 * 64]>,
}

let mut arena = Arena::<Rootable![TestRoot<'_>]>::new(|mc| {
let test = Gc::new(mc, [0; 1024 * 64]);
TestRoot { test }
});
arena.collect_all();

// The collector must start out in the sleeping phase.
assert_eq!(arena.collection_phase(), CollectionPhase::Sleeping);

while arena.collection_phase() == CollectionPhase::Sleeping {
// Keep accumulating debt to keep the collector moving.
arena.mutate(|mc, _| {
Gc::new(mc, 0);
});
// This cannot move past the Marked phase.
arena.mark_debt();
}

// Assert that the collector has woken up into the Marking / Marked phase.
assert!(matches!(
arena.collection_phase(),
CollectionPhase::Marking | CollectionPhase::Marked
));

loop {
// Keep accumulating debt to keep the collector moving.
arena.mutate(|mc, _| {
Gc::new(mc, 0);
});

// If `mark_debt()` returns, we know we must be in the Marked phase.
if arena.mark_debt().is_some() {
assert!(arena.collection_phase() == CollectionPhase::Marked);
break;
}
}

while matches!(
arena.collection_phase(),
CollectionPhase::Marked | CollectionPhase::Collecting
) {
// Keep accumulating debt to keep the collector moving.
arena.mutate(|mc, _| {
Gc::new(mc, 0);
});
// This should not move from Collecting to Marking in one call, it must pass through
// Sleeping.
arena.collect_debt();

if arena.collection_phase() == CollectionPhase::Collecting {
// Assert that mark_debt() and mark_all() do nothing while in the Collecting phase.
assert!(arena.mark_debt().is_none());
assert!(arena.mark_all().is_none());
}
}

// We must end back up at Sleeping.
assert!(arena.collection_phase() == CollectionPhase::Sleeping);
}

#[test]
fn ui() {
let t = trybuild::TestCases::new();
Expand Down

0 comments on commit 42b901e

Please sign in to comment.