From 645051b37dbef8cdbc38357138e4d1e2d444b411 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Mon, 31 Jul 2023 05:35:18 +0000 Subject: [PATCH 1/4] Tidy up mutator scan API * 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 --- Cargo.toml | 3 +- src/scheduler/gc_work.rs | 47 ++-------------------------- src/vm/collection.rs | 16 ++-------- src/vm/scanning.rs | 28 ++++------------- vmbindings/dummyvm/src/collection.rs | 9 ------ vmbindings/dummyvm/src/scanning.rs | 3 -- 6 files changed, 13 insertions(+), 93 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index dc47df85b7..1b0758134f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,7 +47,8 @@ spin = "0.9.5" static_assertions = "1.1.0" strum = "0.24" strum_macros = "0.24" -sysinfo = "0.29" +# Fix on 0.29.5 so we have MSRV 1.61. Remove this when we update MSRV +sysinfo = "=0.29.5" [dev-dependencies] paste = "1.0.8" diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs index 8cdef9a04e..0c7811cc2b 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -182,33 +182,12 @@ impl GCWork for StopMutators { trace!("stop_all_mutators start"); mmtk.plan.base().prepare_for_stack_scanning(); ::VMCollection::stop_all_mutators(worker.tls, |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(ScanStackRoot::(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()); } } @@ -431,28 +410,6 @@ 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); impl GCWork for ScanStackRoot { diff --git a/src/vm/collection.rs b/src/vm/collection.rs index 9caf05ff6f..c1e939ef38 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 provide 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 for each mutator to notify MMTk when a mutator is ready to be stack 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..8a6cea3fd9 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,12 @@ 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. MMTk assumes a binding is able to scan + /// any given thread for its stack roots. If a binding cannot scan the stack for + /// a given thread, 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, From eda3e695a03a669f3a7ba34e7e1d4871dbc4f473 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Mon, 31 Jul 2023 19:03:40 +0800 Subject: [PATCH 2/4] Work around stack overflow in array_from_fn It should fix a crash in JikesRVM caused by overflown stack overwriting objects in the VM space. --- src/util/rust_util/mod.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/util/rust_util/mod.rs b/src/util/rust_util/mod.rs index 7d9b6df4ff..b60161f507 100644 --- a/src/util/rust_util/mod.rs +++ b/src/util/rust_util/mod.rs @@ -127,14 +127,24 @@ mod initialize_once_tests { } /// This implements `std::array::from_fn` introduced in Rust 1.63. -/// We should replace this with the standard counterpart after bumping MSRV. -pub(crate) fn array_from_fn(cb: F) -> [T; N] +/// We should replace this with the standard counterpart after bumping MSRV, +/// but we also need to evaluate whether it would use too much stack space (see code comments). +pub(crate) fn array_from_fn(mut cb: F) -> [T; N] where F: FnMut(usize) -> T, { - let mut index_array = [0; N]; - for (index, item) in index_array.iter_mut().enumerate() { - *item = index; + // Note on unsafety: An alternative to the unsafe implementation below is creating a fixed + // array of `[0, 1, 2, ..., N-1]` and using the `.map(cb)` method to create the result. + // However, the `array::map` method implemented in the standard library consumes a surprisingly + // large amount of stack space. For VMs that run on confined stacks, such as JikesRVM, that + // would cause stack overflow. Therefore we implement it manually using unsafe primitives. + let mut result_array: MaybeUninit<[T; N]> = MaybeUninit::zeroed(); + let array_ptr = result_array.as_mut_ptr(); + for index in 0..N { + let item = cb(index); + unsafe { + std::ptr::addr_of_mut!((*array_ptr)[index]).write(item); + } } - index_array.map(cb) + unsafe { result_array.assume_init() } } From b6a1b8ed826af7c5d042c67021c8cfb4653332fa Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Tue, 8 Aug 2023 01:04:40 +0000 Subject: [PATCH 3/4] Rename ScanStackRoot to ScanMutatorRoots --- src/plan/markcompact/gc_work.rs | 5 +---- src/scheduler/gc_work.rs | 11 +++++------ src/scheduler/mod.rs | 4 ---- src/util/sanity/sanity_checker.rs | 2 +- src/vm/collection.rs | 4 ++-- 5 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/plan/markcompact/gc_work.rs b/src/plan/markcompact/gc_work.rs index 9f86b7acc2..1344c75826 100644 --- a/src/plan/markcompact/gc_work.rs +++ b/src/plan/markcompact/gc_work.rs @@ -43,12 +43,9 @@ impl GCWork for UpdateReferences { #[cfg(feature = "extreme_assertions")] mmtk.edge_logger.reset(); - // 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 5be1f4e033..12240b3717 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -165,8 +165,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); @@ -184,7 +182,8 @@ impl GCWork for StopMutators { ::VMCollection::stop_all_mutators(worker.tls, |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(ScanStackRoot::(mutator)); + mmtk.scheduler.work_buckets[WorkBucketStage::Prepare] + .add(ScanMutatorRoots::(mutator)); }); trace!("stop_all_mutators end"); mmtk.scheduler.notify_mutators_paused(mmtk); @@ -410,11 +409,11 @@ impl GCWork for VMPostForwarding { } } -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 c1e939ef38..7c916eb302 100644 --- a/src/vm/collection.rs +++ b/src/vm/collection.rs @@ -15,12 +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 provide a callback function and expects the binding to use the callback for each mutator when it + /// 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 for each mutator to notify MMTk when a mutator is ready to be stack scanned. + /// * `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); From 7851ba32966e29e1400a9abbac22f4595d3f133a Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Tue, 8 Aug 2023 18:26:59 +1200 Subject: [PATCH 4/4] Update src/vm/scanning.rs Co-authored-by: Kunshan Wang --- src/vm/scanning.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/vm/scanning.rs b/src/vm/scanning.rs index 8a6cea3fd9..6c62a14664 100644 --- a/src/vm/scanning.rs +++ b/src/vm/scanning.rs @@ -195,9 +195,13 @@ 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 one mutator for stack roots. MMTk assumes a binding is able to scan - /// any given thread for its stack roots. If a binding cannot scan the stack for - /// a given thread, it can leave this method empty, and deal with stack + /// 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.