From 3180d70a32fab0a96458dcf8d7578d01aae4dc1a Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Tue, 7 Jul 2026 04:25:01 +0000 Subject: [PATCH 1/4] Add disable_collection/enable_collection API, close concurrent-marking gap Bindings previously had to implement VMCollection::is_collection_enabled() and synchronize enabling/disabling collection across threads themselves. Replace that callback with disable_collection()/enable_collection()/ is_collection_enabled() on MMTK, backed by a nestable, lock-guarded counter in GCTrigger so a GC is never requested while collection is disabled. Also add wait_for_no_collection_in_progress(), which blocks until no GC is pending, mid-pause, or (for concurrent plans, via the existing ConcurrentPlan::concurrent_work_in_progress() hook) concurrently marking. Bindings that need a hard guarantee that no GC touches memory during some unsafe window (e.g. adopting a foreign thread, or recovering from a fatal signal) should call disable_collection() followed by wait_for_no_collection_in_progress(), the MMTk-native equivalent of waiting for an in-flight GC to finish. Without it, a collection that is already mid-cycle when collection is disabled is not affected until it completes, and for concurrent plans that includes the whole concurrent marking phase, not just a stop-the-world pause. Co-authored-by: Claude --- src/memory_manager.rs | 20 +++++ src/mmtk.rs | 42 ++++++++++ src/util/heap/gc_trigger.rs | 76 ++++++++++++++++--- src/util/test_util/mock_vm.rs | 6 -- src/vm/collection.rs | 19 ----- .../mock_test_allocate_nonmoving.rs | 7 +- ...k_test_allocate_with_disable_collection.rs | 9 +-- ...test_allocate_with_re_enable_collection.rs | 11 +-- 8 files changed, 135 insertions(+), 55 deletions(-) diff --git a/src/memory_manager.rs b/src/memory_manager.rs index aac4668c34f..cf34a153aa8 100644 --- a/src/memory_manager.rs +++ b/src/memory_manager.rs @@ -577,6 +577,26 @@ pub fn initialize_collection(mmtk: &'static MMTK, tls: VMThre mmtk.initialize_collection(tls); } +/// Wrapper for [`crate::mmtk::MMTK::disable_collection`]. +pub fn disable_collection(mmtk: &MMTK) { + mmtk.disable_collection(); +} + +/// Wrapper for [`crate::mmtk::MMTK::enable_collection`]. +pub fn enable_collection(mmtk: &MMTK) { + mmtk.enable_collection(); +} + +/// Wrapper for [`crate::mmtk::MMTK::is_collection_enabled`]. +pub fn is_collection_enabled(mmtk: &MMTK) -> bool { + mmtk.is_collection_enabled() +} + +/// Wrapper for [`crate::mmtk::MMTK::wait_for_no_collection_in_progress`]. +pub fn wait_for_no_collection_in_progress(mmtk: &MMTK) { + mmtk.wait_for_no_collection_in_progress(); +} + /// Process MMTk run-time options. Returns true if the option is processed successfully. /// /// Arguments: diff --git a/src/mmtk.rs b/src/mmtk.rs index 74e469319ea..c4adc986adf 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -399,6 +399,48 @@ impl MMTK { *self.state.gc_status.lock().unwrap() == GcStatus::GcProper } + /// Disable collection. MMTk will not trigger a GC while collection is disabled, though a GC + /// that has already been requested (or is already under way, including the concurrent phase + /// of a concurrent GC) is not aborted by this call. Use + /// [`MMTK::wait_for_no_collection_in_progress`] to wait for such a GC to fully finish. + /// + /// This call is nestable. Each call must be paired with a matching call to + /// [`MMTK::enable_collection`]. + pub fn disable_collection(&self) { + self.gc_trigger.disable_collection(); + } + + /// Re-enable collection. Calling it without a prior matching call to + /// [`MMTK::disable_collection`] is a logic error. + pub fn enable_collection(&self) { + self.gc_trigger.enable_collection(); + } + + /// Return whether collection is currently enabled. + pub fn is_collection_enabled(&self) -> bool { + self.gc_trigger.is_collection_enabled() + } + + /// Block the calling thread until there is no collection currently pending or in progress, + /// including the concurrent marking phase of a concurrent GC (during which mutators run + /// normally rather than being stopped). This does not itself prevent a new collection from + /// starting immediately afterwards; call [`MMTK::disable_collection`] first if that must + /// also be prevented. + /// + /// This is intended for infrequent, safety-sensitive call sites (e.g. adopting a foreign + /// thread, or recovering from a fatal signal) rather than as a general-purpose GC barrier, + /// so it is implemented as a simple bounded poll loop rather than a condition variable. + pub fn wait_for_no_collection_in_progress(&self) { + let concurrent_work_in_progress = || { + self.get_plan() + .concurrent() + .is_some_and(|c| c.concurrent_work_in_progress()) + }; + while self.gc_trigger.has_pending_or_active_stw() || concurrent_work_in_progress() { + std::thread::sleep(std::time::Duration::from_micros(50)); + } + } + /// Return true if the current GC is an emergency GC. /// /// An emergency GC happens when a normal GC cannot reclaim enough memory to satisfy allocation diff --git a/src/util/heap/gc_trigger.rs b/src/util/heap/gc_trigger.rs index fd64ac28a12..5cc506ff930 100644 --- a/src/util/heap/gc_trigger.rs +++ b/src/util/heap/gc_trigger.rs @@ -1,18 +1,17 @@ use atomic::Ordering; -use crate::global_state::GlobalState; +use crate::global_state::{GcStatus, GlobalState}; use crate::plan::Plan; use crate::policy::space::Space; use crate::scheduler::GCWorkScheduler; use crate::util::constants::BYTES_IN_PAGE; use crate::util::conversions; use crate::util::options::{GCTriggerSelector, Options, DEFAULT_MAX_NURSERY, DEFAULT_MIN_NURSERY}; -use crate::vm::Collection; use crate::vm::VMBinding; use crate::MMTK; use std::mem::MaybeUninit; use std::sync::atomic::{AtomicBool, AtomicUsize}; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; /// GCTrigger is responsible for triggering GCs based on the given policy. /// All the decisions about heap limit and GC triggering should be resolved here. @@ -27,6 +26,12 @@ pub struct GCTrigger { /// Set by mutators to trigger GC. It is atomic so that mutators can check if GC has already /// been requested efficiently in `poll` without acquiring any mutex. request_flag: AtomicBool, + /// Nesting depth for collection-disable scopes requested via [`GCTrigger::disable_collection`]. + /// A depth of 0 means collection is enabled. + collection_disable_depth: AtomicUsize, + /// Serializes [`GCTrigger::disable_collection`]/[`GCTrigger::enable_collection`] with the + /// decision to request a GC, so that a GC is never requested while collection is disabled. + collection_control_lock: Mutex<()>, scheduler: Arc>, options: Arc, state: Arc, @@ -63,6 +68,8 @@ impl GCTrigger { }, options, request_flag: AtomicBool::new(false), + collection_disable_depth: AtomicUsize::new(0), + collection_control_lock: Mutex::new(()), scheduler, state, } @@ -99,6 +106,59 @@ impl GCTrigger { self.request_flag.store(false, Ordering::Relaxed); } + /// Atomically check that collection is enabled, and if so, request a GC. This makes the + /// enabled-check and the request atomic with respect to [`GCTrigger::disable_collection`] + /// and [`GCTrigger::enable_collection`], so a GC is never requested after collection has + /// been disabled. Returns whether a GC was actually requested. + fn request_if_collection_enabled(&self) -> bool { + let _guard = self.collection_control_lock.lock().unwrap(); + if self.collection_disable_depth.load(Ordering::Acquire) > 0 { + return false; + } + self.request(); + true + } + + /// Disable collection. MMTk will not trigger a GC while collection is disabled, though a GC + /// that has already been requested (or is already under way, including the concurrent phase + /// of a concurrent GC) is not aborted by this call. Use + /// [`crate::MMTK::wait_for_no_collection_in_progress`] to wait for such a GC to fully finish. + /// + /// This call is nestable. Each call must be paired with a matching call to + /// [`GCTrigger::enable_collection`]. + pub fn disable_collection(&self) { + let _guard = self.collection_control_lock.lock().unwrap(); + self.collection_disable_depth + .fetch_add(1, Ordering::Release); + } + + /// Re-enable collection. Calling it without a prior matching call to + /// [`GCTrigger::disable_collection`] is a logic error. + pub fn enable_collection(&self) { + let _guard = self.collection_control_lock.lock().unwrap(); + let depth = self.collection_disable_depth.load(Ordering::Acquire); + assert!( + depth > 0, + "enable_collection() called without a matching disable_collection()" + ); + self.collection_disable_depth + .store(depth - 1, Ordering::Release); + } + + /// Return whether collection is currently enabled. + pub fn is_collection_enabled(&self) -> bool { + self.collection_disable_depth.load(Ordering::Acquire) == 0 + } + + /// Return whether a GC has been requested but not yet started (i.e. mutators have not all + /// stopped), or a stop-the-world pause is currently active. This does not account for the + /// concurrent marking phase of a concurrent GC, which mutators run through normally; see + /// [`crate::plan::concurrent::global::ConcurrentPlan::concurrent_work_in_progress`] for that. + pub(crate) fn has_pending_or_active_stw(&self) -> bool { + self.request_flag.load(Ordering::Acquire) + || *self.state.gc_status.lock().unwrap() != GcStatus::NotInGC + } + /// This method is called periodically by the allocation subsystem /// (by default, each time a page is consumed), and provides the /// collector with an opportunity to collect. @@ -107,7 +167,7 @@ impl GCTrigger { /// * `space_full`: Space request failed, must recover pages within 'space'. /// * `space`: The space that triggered the poll. This could `None` if the poll is not triggered by a space. pub fn poll(&self, space_full: bool, space: Option<&dyn Space>) -> bool { - if !VM::VMCollection::is_collection_enabled() { + if !self.is_collection_enabled() { return false; } @@ -127,8 +187,7 @@ impl GCTrigger { plan.get_reserved_pages(), plan.get_total_pages(), ); - self.request(); - return true; + return self.request_if_collection_enabled(); } false } @@ -145,7 +204,7 @@ impl GCTrigger { return false; } - if force || !*self.options.ignore_system_gc && VM::VMCollection::is_collection_enabled() { + if force || !*self.options.ignore_system_gc && self.is_collection_enabled() { info!("User triggering collection"); // TODO: this may not work reliably. If a GC has been triggered, this will not force it to be a full heap GC. if exhaustive { @@ -157,8 +216,7 @@ impl GCTrigger { self.state .user_triggered_collection .store(true, Ordering::Relaxed); - self.request(); - return true; + return self.request_if_collection_enabled(); } false diff --git a/src/util/test_util/mock_vm.rs b/src/util/test_util/mock_vm.rs index f7f09395406..9087d22c713 100644 --- a/src/util/test_util/mock_vm.rs +++ b/src/util/test_util/mock_vm.rs @@ -215,7 +215,6 @@ pub struct MockVM { pub schedule_finalization: MockMethod, pub post_forwarding: MockMethod, pub vm_live_bytes: MockMethod<(), usize>, - pub is_collection_enabled: MockMethod<(), bool>, pub create_gc_trigger: MockMethod<(), Box>>, // object model pub copy_object: MockMethod< @@ -290,7 +289,6 @@ impl Default for MockVM { schedule_finalization: MockMethod::new_default(), post_forwarding: MockMethod::new_default(), vm_live_bytes: MockMethod::new_default(), - is_collection_enabled: MockMethod::new_fixed(Box::new(|_| true)), create_gc_trigger: MockMethod::new_unimplemented(), copy_object: MockMethod::new_unimplemented(), @@ -455,10 +453,6 @@ impl crate::vm::Collection for MockVM { mock!(post_forwarding(tls)) } - fn is_collection_enabled() -> bool { - mock!(is_collection_enabled()) - } - fn vm_live_bytes() -> usize { mock!(vm_live_bytes()) } diff --git a/src/vm/collection.rs b/src/vm/collection.rs index 7c5fe976426..db0e32d6a92 100644 --- a/src/vm/collection.rs +++ b/src/vm/collection.rs @@ -179,25 +179,6 @@ pub trait Collection { 0 } - /// Callback function to ask the VM whether GC is enabled or disabled, allowing or disallowing MMTk - /// to trigger garbage collection. When collection is disabled, you can still allocate through MMTk, - /// but MMTk will not trigger a GC even if the heap is full. In such a case, the allocation will - /// exceed MMTk's heap size (the soft heap limit). However, there is no guarantee that the physical - /// allocation will succeed, and if it succeeds, there is no guarantee that further allocation will - /// keep succeeding. So if a VM disables collection, it needs to allocate with careful consideration - /// to make sure that the physical memory allows the amount of allocation. We highly recommend - /// to have GC always enabled (i.e. that this method always returns true). However, we support - /// this to accomodate some VMs that require this behavior. Note that - /// `handle_user_collection_request()` calls this function, too. If this function returns - /// false, `handle_user_collection_request()` will not trigger GC, either. Note also that any synchronization - /// involving enabling and disabling collections by mutator threads should be implemented by the VM. - fn is_collection_enabled() -> bool { - // By default, MMTk assumes that collections are always enabled, and the binding should define - // this method if the VM supports disabling GC, or if the VM cannot safely trigger GC until some - // initialization is done, such as initializing class metadata for scanning objects. - true - } - /// Ask the binding to create a [`GCTriggerPolicy`] if the option `gc_trigger` is set to /// `crate::util::options::GCTriggerSelector::Delegated`. fn create_gc_trigger() -> Box> { diff --git a/src/vm/tests/mock_tests/mock_test_allocate_nonmoving.rs b/src/vm/tests/mock_tests/mock_test_allocate_nonmoving.rs index 7c7df436fe3..f0313b3c8ed 100644 --- a/src/vm/tests/mock_tests/mock_test_allocate_nonmoving.rs +++ b/src/vm/tests/mock_tests/mock_test_allocate_nonmoving.rs @@ -6,12 +6,7 @@ use crate::plan::AllocationSemantics; #[test] pub fn allocate_nonmoving() { with_mockvm( - || -> MockVM { - MockVM { - is_collection_enabled: MockMethod::new_fixed(Box::new(|_| false)), - ..MockVM::default() - } - }, + MockVM::default, || { // 1MB heap const MB: usize = 1024 * 1024; diff --git a/src/vm/tests/mock_tests/mock_test_allocate_with_disable_collection.rs b/src/vm/tests/mock_tests/mock_test_allocate_with_disable_collection.rs index 373381649a8..5ae4d005b54 100644 --- a/src/vm/tests/mock_tests/mock_test_allocate_with_disable_collection.rs +++ b/src/vm/tests/mock_tests/mock_test_allocate_with_disable_collection.rs @@ -1,7 +1,6 @@ use crate::memory_manager; use crate::util::test_util::fixtures::*; use crate::util::test_util::mock_vm::*; -use crate::vm::tests::mock_tests::mock_test_prelude::MockMethod; use crate::AllocationSemantics; /// This test allocates after calling disable_collection(). When we exceed the heap limit, MMTk will NOT trigger a GC. @@ -9,16 +8,12 @@ use crate::AllocationSemantics; #[test] pub fn allocate_with_disable_collection() { with_mockvm( - || -> MockVM { - MockVM { - is_collection_enabled: MockMethod::new_fixed(Box::new(|_| false)), - ..MockVM::default() - } - }, + MockVM::default, || { // 1MB heap const MB: usize = 1024 * 1024; let mut fixture = MutatorFixture::create_with_heapsize(MB); + memory_manager::disable_collection(fixture.mmtk()); // Allocate half MB. It should be fine. let addr = memory_manager::alloc( diff --git a/src/vm/tests/mock_tests/mock_test_allocate_with_re_enable_collection.rs b/src/vm/tests/mock_tests/mock_test_allocate_with_re_enable_collection.rs index 296e4a20a23..65ee0f97b5c 100644 --- a/src/vm/tests/mock_tests/mock_test_allocate_with_re_enable_collection.rs +++ b/src/vm/tests/mock_tests/mock_test_allocate_with_re_enable_collection.rs @@ -15,11 +15,6 @@ pub fn allocate_with_re_enable_collection() { || -> MockVM { MockVM { block_for_gc: MockMethod::new_fixed(Box::new(|_| panic!("block_for_gc is called"))), - is_collection_enabled: MockMethod::new_sequence(vec![ - Box::new(|()| -> bool { true }), // gc is enabled but it shouldn't matter here - Box::new(|()| -> bool { false }), // gc is disabled - Box::new(|()| -> bool { true }), // gc is enabled again - ]), ..MockVM::default() } }, @@ -38,9 +33,11 @@ pub fn allocate_with_re_enable_collection() { assert!(!addr.is_zero()); // In the next allocation GC is disabled. So we can keep allocate without triggering a GC. + memory_manager::disable_collection(fixture.mmtk()); // Fill up the heap let _ = memory_manager::alloc(&mut fixture.mutator, MB, 8, 0, AllocationSemantics::Default); + memory_manager::enable_collection(fixture.mmtk()); // Attempt another allocation. This will trigger GC since GC is enabled again. let addr = @@ -48,11 +45,9 @@ pub fn allocate_with_re_enable_collection() { assert!(!addr.is_zero()); }, || { - // This ensures that block_for_gc is called for this test, and that the second allocation - // does not trigger GC since we expect is_collection_enabled to be called three times. + // This ensures block_for_gc is called for this test. read_mockvm(|mock| { assert!(mock.block_for_gc.is_called()); - assert!(mock.is_collection_enabled.call_count() == 3); }); }, ) From 5a90a387cb07f3aced51494de5a2fe13c00a607c Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 8 Jul 2026 05:41:26 +0000 Subject: [PATCH 2/4] Use thread::yield_now() in wait_for_no_collection_in_progress, add test Poll with a yield instead of a fixed sleep between checks, so the calling thread gives up its timeslice without imposing a fixed latency floor on how quickly it notices the collection has finished. Add a regression test: one thread triggers a GC and blocks in block_for_gc (simulating an in-progress collection), while a second thread calls disable_collection() followed by wait_for_no_collection_in_progress() and must remain blocked until the first thread's GC finishes. Also fix two benchmarks (mock_bench/alloc.rs, mock_bench/internal_pointer.rs) that configured the now-removed MockVM::is_collection_enabled field to keep GC out of the way; they now call memory_manager::disable_collection() directly. Co-authored-by: Claude --- benches/mock_bench/alloc.rs | 15 +-- benches/mock_bench/internal_pointer.rs | 8 +- src/mmtk.rs | 5 +- ...test_wait_for_no_collection_in_progress.rs | 114 ++++++++++++++++++ src/vm/tests/mock_tests/mod.rs | 1 + 5 files changed, 125 insertions(+), 18 deletions(-) create mode 100644 src/vm/tests/mock_tests/mock_test_wait_for_no_collection_in_progress.rs diff --git a/benches/mock_bench/alloc.rs b/benches/mock_bench/alloc.rs index 9a771cf05f8..6a5d7f84425 100644 --- a/benches/mock_bench/alloc.rs +++ b/benches/mock_bench/alloc.rs @@ -2,23 +2,12 @@ use criterion::Criterion; use mmtk::memory_manager; use mmtk::util::test_util::fixtures::*; -use mmtk::util::test_util::mock_method::*; -use mmtk::util::test_util::mock_vm::{write_mockvm, MockVM}; use mmtk::AllocationSemantics; pub fn bench(c: &mut Criterion) { - // Setting a larger heap, although the GC should be disabled in the MockVM + // Setting a larger heap, although the GC should be disabled below let mut fixture = MutatorFixture::create_with_heapsize(1 << 30); - { - write_mockvm(|mock| { - *mock = { - MockVM { - is_collection_enabled: MockMethod::new_fixed(Box::new(|_| false)), - ..MockVM::default() - } - } - }); - } + memory_manager::disable_collection(fixture.mmtk()); c.bench_function("alloc", |b| { b.iter(|| { diff --git a/benches/mock_bench/internal_pointer.rs b/benches/mock_bench/internal_pointer.rs index 00d8cf229be..0a9ed8f5679 100644 --- a/benches/mock_bench/internal_pointer.rs +++ b/benches/mock_bench/internal_pointer.rs @@ -1,14 +1,18 @@ use criterion::Criterion; +#[cfg(feature = "vo_bit")] +use mmtk::memory_manager; #[cfg(feature = "vo_bit")] use mmtk::util::test_util::fixtures::*; use mmtk::util::test_util::mock_method::*; use mmtk::util::test_util::mock_vm::{write_mockvm, MockVM}; pub fn bench(c: &mut Criterion) { - // Setting a larger heap, although the GC should be disabled in the MockVM + // Setting a larger heap, although the GC should be disabled below #[cfg(feature = "vo_bit")] let mut fixture = MutatorFixture::create_with_heapsize(1 << 30); + #[cfg(feature = "vo_bit")] + memory_manager::disable_collection(fixture.mmtk()); // Normal objects // 16KB object -- we want to make sure the object can fit into any normal space (e.g. immix space or mark sweep space) @@ -16,7 +20,6 @@ pub fn bench(c: &mut Criterion) { write_mockvm(|mock| { *mock = MockVM { get_object_size: MockMethod::new_fixed(Box::new(|_| NORMAL_OBJECT_SIZE)), - is_collection_enabled: MockMethod::new_fixed(Box::new(|_| false)), ..MockVM::default() } }); @@ -55,7 +58,6 @@ pub fn bench(c: &mut Criterion) { write_mockvm(|mock| { *mock = MockVM { get_object_size: MockMethod::new_fixed(Box::new(|_| LARGE_OBJECT_SIZE)), - is_collection_enabled: MockMethod::new_fixed(Box::new(|_| false)), ..MockVM::default() } }); diff --git a/src/mmtk.rs b/src/mmtk.rs index c4adc986adf..a100391fd8a 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -429,7 +429,8 @@ impl MMTK { /// /// This is intended for infrequent, safety-sensitive call sites (e.g. adopting a foreign /// thread, or recovering from a fatal signal) rather than as a general-purpose GC barrier, - /// so it is implemented as a simple bounded poll loop rather than a condition variable. + /// so it is implemented as a simple poll loop (yielding the calling thread between checks) + /// rather than a condition variable. pub fn wait_for_no_collection_in_progress(&self) { let concurrent_work_in_progress = || { self.get_plan() @@ -437,7 +438,7 @@ impl MMTK { .is_some_and(|c| c.concurrent_work_in_progress()) }; while self.gc_trigger.has_pending_or_active_stw() || concurrent_work_in_progress() { - std::thread::sleep(std::time::Duration::from_micros(50)); + std::thread::yield_now(); } } diff --git a/src/vm/tests/mock_tests/mock_test_wait_for_no_collection_in_progress.rs b/src/vm/tests/mock_tests/mock_test_wait_for_no_collection_in_progress.rs new file mode 100644 index 00000000000..8791c12522c --- /dev/null +++ b/src/vm/tests/mock_tests/mock_test_wait_for_no_collection_in_progress.rs @@ -0,0 +1,114 @@ +use super::mock_test_prelude::*; +use crate::util::{OpaquePointer, VMMutatorThread, VMThread}; +use std::sync::{Condvar, Mutex}; +use std::time::{Duration, Instant}; + +const TIMEOUT: Duration = Duration::from_secs(5); + +#[derive(Default)] +struct Sync { + gc_started: bool, + release_gc: bool, +} + +lazy_static! { + static ref SYNC: (Mutex, Condvar) = (Mutex::new(Sync::default()), Condvar::new()); +} + +/// Poll `condition` until it is true, or panic if `TIMEOUT` elapses first. Used instead of an +/// unbounded `JoinHandle::join()` so a regression turns into a clear test failure rather than a +/// hung test process. +fn wait_until(mut condition: impl FnMut() -> bool, msg: &str) { + let start = Instant::now(); + while !condition() { + assert!(start.elapsed() < TIMEOUT, "{}", msg); + std::thread::yield_now(); + } +} + +/// Regression test for issue mmtk/mmtk-julia#278: if a GC is already in progress on one thread, +/// another thread that calls `disable_collection()` followed by +/// `wait_for_no_collection_in_progress()` must block until that GC actually finishes, rather +/// than racing past it. +#[test] +pub fn wait_for_no_collection_in_progress_blocks_until_gc_finishes() { + with_mockvm( + || -> MockVM { + MockVM { + // Simulate a GC that is "in progress": signal that we have started, then block + // until the test tells us to finish. + block_for_gc: MockMethod::new_fixed(Box::new(|_| { + let (lock, cvar) = &*SYNC; + let mut sync = lock.lock().unwrap(); + sync.gc_started = true; + cvar.notify_all(); + while !sync.release_gc { + sync = cvar.wait(sync).unwrap(); + } + })), + ..MockVM::default() + } + }, + || { + let fixture = MutatorFixture::create_with_heapsize(1024 * 1024); + let mmtk = fixture.mmtk(); + + // Thread A: trigger a GC. `handle_user_collection_request` blocks the calling + // thread in (our mocked) `block_for_gc` until the GC finishes. + let gc_thread = std::thread::spawn(move || { + let tls = VMMutatorThread(VMThread(OpaquePointer::UNINITIALIZED)); + memory_manager::handle_user_collection_request(mmtk, tls); + }); + + // Wait until the GC has actually started (i.e. is "in progress"). + { + let (lock, cvar) = &*SYNC; + let mut sync = lock.lock().unwrap(); + while !sync.gc_started { + sync = cvar.wait(sync).unwrap(); + } + } + + // Thread B: disable collection and wait for no collection to be in progress. This + // must block, because the GC triggered by thread A is still running. + let waiter = std::thread::spawn(move || { + mmtk.disable_collection(); + mmtk.wait_for_no_collection_in_progress(); + }); + + // Give thread B a chance to run, then confirm it is still blocked. + std::thread::sleep(Duration::from_millis(200)); + assert!( + !waiter.is_finished(), + "wait_for_no_collection_in_progress() returned while a GC was still in progress" + ); + + // Let thread A's GC finish. + { + let (lock, cvar) = &*SYNC; + let mut sync = lock.lock().unwrap(); + sync.release_gc = true; + cvar.notify_all(); + } + wait_until( + || gc_thread.is_finished(), + "the GC-triggering thread did not finish in time", + ); + // This test does not spawn real GC worker threads, so nothing else clears the GC + // request that thread A made. Normally the scheduler does this once all mutators + // have stopped and the collection completes; simulate that here. + mmtk.gc_trigger.clear_request(); + + // Now that the GC has finished, thread B should unblock. + wait_until( + || waiter.is_finished(), + "wait_for_no_collection_in_progress() did not return after the GC finished", + ); + + mmtk.enable_collection(); + gc_thread.join().unwrap(); + waiter.join().unwrap(); + }, + no_cleanup, + ) +} diff --git a/src/vm/tests/mock_tests/mod.rs b/src/vm/tests/mock_tests/mod.rs index 93030261028..4f0b2e9fe31 100644 --- a/src/vm/tests/mock_tests/mod.rs +++ b/src/vm/tests/mock_tests/mod.rs @@ -72,6 +72,7 @@ mod mock_test_vm_layout_compressed_pointer; mod mock_test_vm_layout_default; mod mock_test_vm_layout_heap_start; mod mock_test_vm_layout_log_address_space; +mod mock_test_wait_for_no_collection_in_progress; mod mock_test_doc_avoid_resolving_allocator; mod mock_test_doc_mutator_storage; From 65cafe6f5a85ac6f11a19345a796662111f29ccc Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Fri, 10 Jul 2026 01:17:14 +0000 Subject: [PATCH 3/4] Clean up --- src/mmtk.rs | 2 +- src/util/heap/gc_trigger.rs | 37 +++++++++---------- ...test_wait_for_no_collection_in_progress.rs | 16 ++++---- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/mmtk.rs b/src/mmtk.rs index a100391fd8a..f8ae97ee7fb 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -437,7 +437,7 @@ impl MMTK { .concurrent() .is_some_and(|c| c.concurrent_work_in_progress()) }; - while self.gc_trigger.has_pending_or_active_stw() || concurrent_work_in_progress() { + while self.gc_trigger.has_pending_or_active_pause() || concurrent_work_in_progress() { std::thread::yield_now(); } } diff --git a/src/util/heap/gc_trigger.rs b/src/util/heap/gc_trigger.rs index 5cc506ff930..a86708d5ad3 100644 --- a/src/util/heap/gc_trigger.rs +++ b/src/util/heap/gc_trigger.rs @@ -86,9 +86,19 @@ impl GCTrigger { /// Request a GC. Called by mutators when polling (during allocation) and when handling user /// GC requests (e.g. `System.gc();` in Java). - fn request(&self) { + /// Atomically check that collection is enabled, and if so, request a GC. This makes the + /// enabled-check and the request atomic with respect to [`GCTrigger::disable_collection`] + /// and [`GCTrigger::enable_collection`], so a GC is never requested after collection has + /// been disabled. + /// Returns whether a GC was actually requested. + fn request(&self) -> bool { + let _guard = self.collection_control_lock.lock().unwrap(); + if self.collection_disable_depth.load(Ordering::Acquire) > 0 { + return false; + } + if self.request_flag.load(Ordering::Relaxed) { - return; + return true; } if !self.request_flag.swap(true, Ordering::Relaxed) { @@ -98,6 +108,8 @@ impl GCTrigger { probe!(mmtk, gc_requested); self.scheduler.request_schedule_collection(); } + + true } /// Clear the "GC requested" flag so that mutators can trigger the next GC. @@ -106,19 +118,6 @@ impl GCTrigger { self.request_flag.store(false, Ordering::Relaxed); } - /// Atomically check that collection is enabled, and if so, request a GC. This makes the - /// enabled-check and the request atomic with respect to [`GCTrigger::disable_collection`] - /// and [`GCTrigger::enable_collection`], so a GC is never requested after collection has - /// been disabled. Returns whether a GC was actually requested. - fn request_if_collection_enabled(&self) -> bool { - let _guard = self.collection_control_lock.lock().unwrap(); - if self.collection_disable_depth.load(Ordering::Acquire) > 0 { - return false; - } - self.request(); - true - } - /// Disable collection. MMTk will not trigger a GC while collection is disabled, though a GC /// that has already been requested (or is already under way, including the concurrent phase /// of a concurrent GC) is not aborted by this call. Use @@ -153,8 +152,8 @@ impl GCTrigger { /// Return whether a GC has been requested but not yet started (i.e. mutators have not all /// stopped), or a stop-the-world pause is currently active. This does not account for the /// concurrent marking phase of a concurrent GC, which mutators run through normally; see - /// [`crate::plan::concurrent::global::ConcurrentPlan::concurrent_work_in_progress`] for that. - pub(crate) fn has_pending_or_active_stw(&self) -> bool { + /// `ConcurrentPlan::concurrent_work_in_progress`. + pub(crate) fn has_pending_or_active_pause(&self) -> bool { self.request_flag.load(Ordering::Acquire) || *self.state.gc_status.lock().unwrap() != GcStatus::NotInGC } @@ -187,7 +186,7 @@ impl GCTrigger { plan.get_reserved_pages(), plan.get_total_pages(), ); - return self.request_if_collection_enabled(); + return self.request(); } false } @@ -216,7 +215,7 @@ impl GCTrigger { self.state .user_triggered_collection .store(true, Ordering::Relaxed); - return self.request_if_collection_enabled(); + return self.request(); } false diff --git a/src/vm/tests/mock_tests/mock_test_wait_for_no_collection_in_progress.rs b/src/vm/tests/mock_tests/mock_test_wait_for_no_collection_in_progress.rs index 8791c12522c..a4db5cddea7 100644 --- a/src/vm/tests/mock_tests/mock_test_wait_for_no_collection_in_progress.rs +++ b/src/vm/tests/mock_tests/mock_test_wait_for_no_collection_in_progress.rs @@ -1,3 +1,5 @@ +// GITHUB-CI: MMTK_PLAN=Immix,ConcurrentImmix + use super::mock_test_prelude::*; use crate::util::{OpaquePointer, VMMutatorThread, VMThread}; use std::sync::{Condvar, Mutex}; @@ -55,7 +57,7 @@ pub fn wait_for_no_collection_in_progress_blocks_until_gc_finishes() { // Thread A: trigger a GC. `handle_user_collection_request` blocks the calling // thread in (our mocked) `block_for_gc` until the GC finishes. - let gc_thread = std::thread::spawn(move || { + let thread_to_trigger_gc = std::thread::spawn(move || { let tls = VMMutatorThread(VMThread(OpaquePointer::UNINITIALIZED)); memory_manager::handle_user_collection_request(mmtk, tls); }); @@ -71,7 +73,7 @@ pub fn wait_for_no_collection_in_progress_blocks_until_gc_finishes() { // Thread B: disable collection and wait for no collection to be in progress. This // must block, because the GC triggered by thread A is still running. - let waiter = std::thread::spawn(move || { + let thread_to_disable_gc = std::thread::spawn(move || { mmtk.disable_collection(); mmtk.wait_for_no_collection_in_progress(); }); @@ -79,7 +81,7 @@ pub fn wait_for_no_collection_in_progress_blocks_until_gc_finishes() { // Give thread B a chance to run, then confirm it is still blocked. std::thread::sleep(Duration::from_millis(200)); assert!( - !waiter.is_finished(), + !thread_to_disable_gc.is_finished(), "wait_for_no_collection_in_progress() returned while a GC was still in progress" ); @@ -91,7 +93,7 @@ pub fn wait_for_no_collection_in_progress_blocks_until_gc_finishes() { cvar.notify_all(); } wait_until( - || gc_thread.is_finished(), + || thread_to_trigger_gc.is_finished(), "the GC-triggering thread did not finish in time", ); // This test does not spawn real GC worker threads, so nothing else clears the GC @@ -101,13 +103,13 @@ pub fn wait_for_no_collection_in_progress_blocks_until_gc_finishes() { // Now that the GC has finished, thread B should unblock. wait_until( - || waiter.is_finished(), + || thread_to_disable_gc.is_finished(), "wait_for_no_collection_in_progress() did not return after the GC finished", ); mmtk.enable_collection(); - gc_thread.join().unwrap(); - waiter.join().unwrap(); + thread_to_trigger_gc.join().unwrap(); + thread_to_disable_gc.join().unwrap(); }, no_cleanup, ) From e87dc7e4e0ef38ca3236e1844d65b5a4c760b9e2 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Mon, 13 Jul 2026 06:34:02 +0000 Subject: [PATCH 4/4] Expose has_pending_or_active_pause --- src/mmtk.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/mmtk.rs b/src/mmtk.rs index f8ae97ee7fb..988593808bf 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -442,6 +442,14 @@ impl MMTK { } } + /// Check if MMTK has any pending or active pauses. + /// MMTk may initiate a pause before MMTk requires a stop-the-world through + /// the [`crate::vm::Collection::stop_all_mutators()`] call. This function allows the VM to check + /// if MMTk has any pending pauses before `stop_all_mutators` is called. + pub fn has_pending_or_active_pause(&self) -> bool { + self.gc_trigger.has_pending_or_active_pause() + } + /// Return true if the current GC is an emergency GC. /// /// An emergency GC happens when a normal GC cannot reclaim enough memory to satisfy allocation