From 099ddbe128e343e8ba5bc81e72ecdff0ab30c6f5 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 9 Aug 2023 19:38:22 +1200 Subject: [PATCH] Tidy up mutator scan API (#875) This PR closes https://github.com/mmtk/mmtk-core/issues/496. * Remove Scanning::SCAN_MUTATORS_IN_SAFEPOINT, and Scanning::SINGLE_THREAD_MUTATOR_SCANNING. * Remove Scanning::scan_roots_in_all_mutator_threads. * Remove Collection::prepare_mutator * Update comments for a few methods --- src/plan/markcompact/gc_work.rs | 5 +-- src/scheduler/gc_work.rs | 58 ++++------------------------ src/scheduler/mod.rs | 4 -- src/util/sanity/sanity_checker.rs | 2 +- src/vm/collection.rs | 16 ++------ src/vm/scanning.rs | 32 +++++---------- vmbindings/dummyvm/src/collection.rs | 9 ----- vmbindings/dummyvm/src/scanning.rs | 3 -- 8 files changed, 22 insertions(+), 107 deletions(-) diff --git a/src/plan/markcompact/gc_work.rs b/src/plan/markcompact/gc_work.rs index 22c615589b..eded813463 100644 --- a/src/plan/markcompact/gc_work.rs +++ b/src/plan/markcompact/gc_work.rs @@ -49,12 +49,9 @@ impl GCWork for UpdateReferences { .worker_group .get_and_clear_worker_live_bytes(); - // TODO investigate why the following will create duplicate edges - // scheduler.work_buckets[WorkBucketStage::RefForwarding] - // .add(ScanStackRoots::>::new()); for mutator in VM::VMActivePlan::mutators() { mmtk.scheduler.work_buckets[WorkBucketStage::SecondRoots] - .add(ScanStackRoot::>(mutator)); + .add(ScanMutatorRoots::>(mutator)); } mmtk.scheduler.work_buckets[WorkBucketStage::SecondRoots] diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs index ac802b4f8c..bd69b0eae6 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -177,8 +177,6 @@ impl GCWork for ReleaseCollector { /// Stop all mutators /// -/// Schedule a `ScanStackRoots` immediately after a mutator is paused -/// /// TODO: Smaller work granularity #[derive(Default)] pub struct StopMutators(PhantomData); @@ -194,33 +192,13 @@ impl GCWork for StopMutators { trace!("stop_all_mutators start"); mmtk.plan.base().prepare_for_stack_scanning(); ::VMCollection::stop_all_mutators(worker.tls, |mutator| { - mmtk.scheduler.work_buckets[WorkBucketStage::Prepare].add(ScanStackRoot::(mutator)); + // TODO: The stack scanning work won't start immediately, as the `Prepare` bucket is not opened yet (the bucket is opened in notify_mutators_paused). + // Should we push to Unconstrained instead? + mmtk.scheduler.work_buckets[WorkBucketStage::Prepare] + .add(ScanMutatorRoots::(mutator)); }); trace!("stop_all_mutators end"); mmtk.scheduler.notify_mutators_paused(mmtk); - if ::VMScanning::SCAN_MUTATORS_IN_SAFEPOINT { - // Prepare mutators if necessary - // FIXME: This test is probably redundant. JikesRVM requires to call `prepare_mutator` once after mutators are paused - if !mmtk.plan.base().stacks_prepared() { - for mutator in ::VMActivePlan::mutators() { - ::VMCollection::prepare_mutator( - worker.tls, - mutator.get_tls(), - mutator, - ); - } - } - // Scan mutators - if ::VMScanning::SINGLE_THREAD_MUTATOR_SCANNING { - mmtk.scheduler.work_buckets[WorkBucketStage::Prepare] - .add(ScanStackRoots::::new()); - } else { - for mutator in ::VMActivePlan::mutators() { - mmtk.scheduler.work_buckets[WorkBucketStage::Prepare] - .add(ScanStackRoot::(mutator)); - } - } - } mmtk.scheduler.work_buckets[WorkBucketStage::Prepare].add(ScanVMSpecificRoots::::new()); } } @@ -465,33 +443,11 @@ impl GCWork for VMPostForwarding { } } -#[derive(Default)] -pub struct ScanStackRoots(PhantomData); - -impl ScanStackRoots { - pub fn new() -> Self { - Self(PhantomData) - } -} - -impl GCWork for ScanStackRoots { - fn do_work(&mut self, worker: &mut GCWorker, mmtk: &'static MMTK) { - trace!("ScanStackRoots"); - let factory = ProcessEdgesWorkRootsWorkFactory::::new(mmtk); - ::VMScanning::scan_roots_in_all_mutator_threads(worker.tls, factory); - ::VMScanning::notify_initial_thread_scan_complete(false, worker.tls); - for mutator in ::VMActivePlan::mutators() { - mutator.flush(); - } - mmtk.plan.common().base.set_gc_status(GcStatus::GcProper); - } -} - -pub struct ScanStackRoot(pub &'static mut Mutator); +pub struct ScanMutatorRoots(pub &'static mut Mutator); -impl GCWork for ScanStackRoot { +impl GCWork for ScanMutatorRoots { fn do_work(&mut self, worker: &mut GCWorker, mmtk: &'static MMTK) { - trace!("ScanStackRoot for mutator {:?}", self.0.get_tls()); + trace!("ScanMutatorRoots for mutator {:?}", self.0.get_tls()); let base = &mmtk.plan.base(); let mutators = ::VMActivePlan::number_of_mutators(); let factory = ProcessEdgesWorkRootsWorkFactory::::new(mmtk); diff --git a/src/scheduler/mod.rs b/src/scheduler/mod.rs index 3851490766..cedbbf16cd 100644 --- a/src/scheduler/mod.rs +++ b/src/scheduler/mod.rs @@ -25,7 +25,3 @@ pub use controller::GCController; pub(crate) mod gc_work; pub use gc_work::ProcessEdgesWork; -// TODO: We shouldn't need to expose ScanStackRoot. However, OpenJDK uses it. -// We should do some refactoring related to Scanning::SCAN_MUTATORS_IN_SAFEPOINT -// to make sure this type is not exposed to the bindings. -pub use gc_work::ScanStackRoot; diff --git a/src/util/sanity/sanity_checker.rs b/src/util/sanity/sanity_checker.rs index 8639cb2077..4cb0a19a88 100644 --- a/src/util/sanity/sanity_checker.rs +++ b/src/util/sanity/sanity_checker.rs @@ -83,7 +83,7 @@ impl GCWork for ScheduleSanityGC

{ // in openjdk binding before the second round of roots scanning. // for mutator in ::VMActivePlan::mutators() { // scheduler.work_buckets[WorkBucketStage::Prepare] - // .add(ScanStackRoot::>(mutator)); + // .add(ScanMutatorRoots::>(mutator)); // } { let sanity_checker = mmtk.sanity_checker.lock().unwrap(); diff --git a/src/vm/collection.rs b/src/vm/collection.rs index 9caf05ff6f..7c916eb302 100644 --- a/src/vm/collection.rs +++ b/src/vm/collection.rs @@ -1,4 +1,3 @@ -use crate::plan::MutatorContext; use crate::util::alloc::AllocationError; use crate::util::opaque_pointer::*; use crate::vm::VMBinding; @@ -16,9 +15,12 @@ pub trait Collection { /// This method is called by a single thread in MMTk (the GC controller). /// This method should not return until all the threads are yielded. /// The actual thread synchronization mechanism is up to the VM, and MMTk does not make assumptions on that. + /// MMTk provides a callback function and expects the binding to use the callback for each mutator when it + /// is ready for stack scanning. Usually a stack can be scanned as soon as the thread stops in the yieldpoint. /// /// Arguments: /// * `tls`: The thread pointer for the GC controller/coordinator. + /// * `mutator_visitor`: A callback. Call it with a mutator as argument to notify MMTk that the mutator is ready to be scanned. fn stop_all_mutators(tls: VMWorkerThread, mutator_visitor: F) where F: FnMut(&'static mut Mutator); @@ -54,18 +56,6 @@ pub trait Collection { /// In either case, the `Box` inside should be passed back to the called function. fn spawn_gc_thread(tls: VMThread, ctx: GCThreadContext); - /// Allow VM-specific behaviors for a mutator after all the mutators are stopped and before any actual GC work starts. - /// - /// Arguments: - /// * `tls_worker`: The thread pointer for the worker thread performing this call. - /// * `tls_mutator`: The thread pointer for the target mutator thread. - /// * `m`: The mutator context for the thread. - fn prepare_mutator>( - tls_worker: VMWorkerThread, - tls_mutator: VMMutatorThread, - m: &T, - ); - /// Inform the VM of an out-of-memory error. The binding should hook into the VM's error /// routine for OOM. Note that there are two different categories of OOM: /// * Critical OOM: This is the case where the OS is unable to mmap or acquire more memory. diff --git a/src/vm/scanning.rs b/src/vm/scanning.rs index 3a262f7542..6c62a14664 100644 --- a/src/vm/scanning.rs +++ b/src/vm/scanning.rs @@ -121,14 +121,6 @@ pub trait RootsWorkFactory: Clone + Send + 'static { /// VM-specific methods for scanning roots/objects. pub trait Scanning { - /// Scan stack roots after all mutators are paused. - const SCAN_MUTATORS_IN_SAFEPOINT: bool = true; - - /// Scan all the mutators within a single work packet. - /// - /// `SCAN_MUTATORS_IN_SAFEPOINT` should also be enabled - const SINGLE_THREAD_MUTATOR_SCANNING: bool = true; - /// Return true if the given object supports edge enqueuing. /// /// - If this returns true, MMTk core will call `scan_object` on the object. @@ -203,20 +195,16 @@ pub trait Scanning { /// * `tls`: The GC thread that is performing the thread scan. fn notify_initial_thread_scan_complete(partial_scan: bool, tls: VMWorkerThread); - /// Scan all the mutators for roots. - /// - /// The `memory_manager::is_mmtk_object` function can be used in this function if - /// - the "is_mmtk_object" feature is enabled. - /// - /// Arguments: - /// * `tls`: The GC thread that is performing this scanning. - /// * `factory`: The VM uses it to create work packets for scanning roots. - fn scan_roots_in_all_mutator_threads( - tls: VMWorkerThread, - factory: impl RootsWorkFactory, - ); - - /// Scan one mutator for roots. + /// Scan one mutator for stack roots. + /// + /// Some VM bindings may not be able to implement this method. + /// For example, the VM binding may only be able to enumerate all threads and + /// scan them while enumerating, but cannot scan stacks individually when given + /// the references of threads. + /// In that case, it can leave this method empty, and deal with stack + /// roots in [`Collection::scan_vm_specific_roots`]. However, in that case, MMTk + /// does not know those roots are stack roots, and cannot perform any possible + /// optimization for the stack roots. /// /// The `memory_manager::is_mmtk_object` function can be used in this function if /// - the "is_mmtk_object" feature is enabled. diff --git a/vmbindings/dummyvm/src/collection.rs b/vmbindings/dummyvm/src/collection.rs index 9d45d8b102..f82e793fd3 100644 --- a/vmbindings/dummyvm/src/collection.rs +++ b/vmbindings/dummyvm/src/collection.rs @@ -3,7 +3,6 @@ use mmtk::util::opaque_pointer::*; use mmtk::vm::Collection; use mmtk::vm::GCThreadContext; use mmtk::Mutator; -use mmtk::MutatorContext; pub struct VMCollection {} @@ -24,12 +23,4 @@ impl Collection for VMCollection { } fn spawn_gc_thread(_tls: VMThread, _ctx: GCThreadContext) {} - - fn prepare_mutator>( - _tls_w: VMWorkerThread, - _tls_m: VMMutatorThread, - _mutator: &T, - ) { - unimplemented!() - } } diff --git a/vmbindings/dummyvm/src/scanning.rs b/vmbindings/dummyvm/src/scanning.rs index 789e7aec14..8698106664 100644 --- a/vmbindings/dummyvm/src/scanning.rs +++ b/vmbindings/dummyvm/src/scanning.rs @@ -10,9 +10,6 @@ use mmtk::Mutator; pub struct VMScanning {} impl Scanning for VMScanning { - fn scan_roots_in_all_mutator_threads(_tls: VMWorkerThread, _factory: impl RootsWorkFactory) { - unimplemented!() - } fn scan_roots_in_mutator_thread( _tls: VMWorkerThread, _mutator: &'static mut Mutator,