From 42437157c09557dad0413e12c59b41a3af0e42fa Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Fri, 24 May 2024 03:37:40 +0000 Subject: [PATCH 1/6] Remove GC in harness_begin. full_heap_system_gc defaults to true --- src/memory_manager.rs | 5 +++-- src/mmtk.rs | 6 ++---- src/util/options.rs | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/memory_manager.rs b/src/memory_manager.rs index 6f41a3951b..97503260e5 100644 --- a/src/memory_manager.rs +++ b/src/memory_manager.rs @@ -710,8 +710,9 @@ pub fn add_phantom_candidate(mmtk: &MMTK, reff: ObjectReferen mmtk.reference_processors.add_phantom_candidate(reff); } -/// Generic hook to allow benchmarks to be harnessed. We do a full heap -/// GC, and then start recording statistics for MMTk. +/// Generic hook to allow benchmarks to be harnessed. It is recommended that +/// the application/benchmark should trigger a GC through [`crate::memory_manager::handle_user_collection_request`] +/// before calling `harness_begin`. /// /// Arguments: /// * `mmtk`: A reference to an MMTk instance. diff --git a/src/mmtk.rs b/src/mmtk.rs index eea56e7e45..be7819143d 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -318,12 +318,10 @@ impl MMTK { self.scheduler.respawn_gc_threads_after_forking(tls); } - /// Generic hook to allow benchmarks to be harnessed. MMTk will trigger a GC - /// to clear any residual garbage and start collecting statistics for the benchmark. + /// Generic hook to allow benchmarks to be harnessed. /// This is usually called by the benchmark harness as its last step before the actual benchmark. - pub fn harness_begin(&self, tls: VMMutatorThread) { + pub fn harness_begin(&self, _tls: VMMutatorThread) { probe!(mmtk, harness_begin); - self.handle_user_collection_request(tls, true, true); self.inside_harness.store(true, Ordering::SeqCst); self.stats.start_all(); self.scheduler.enable_stat(); diff --git a/src/util/options.rs b/src/util/options.rs index 3d1e70a965..a83781b9a0 100644 --- a/src/util/options.rs +++ b/src/util/options.rs @@ -807,7 +807,7 @@ options! { nursery: NurserySize [env_var: true, command_line: true] [|v: &NurserySize| v.validate()] = NurserySize::ProportionalBounded { min: DEFAULT_PROPORTIONAL_MIN_NURSERY, max: DEFAULT_PROPORTIONAL_MAX_NURSERY }, /// Should a major GC be performed when a system GC is required? - full_heap_system_gc: bool [env_var: true, command_line: true] [always_valid] = false, + full_heap_system_gc: bool [env_var: true, command_line: true] [always_valid] = true, /// Should finalization be disabled? no_finalizer: bool [env_var: true, command_line: true] [always_valid] = false, /// Should reference type processing be disabled? From 9f3040374be56a39a26d7dd1e7c973b3b79d5050 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Fri, 24 May 2024 03:58:10 +0000 Subject: [PATCH 2/6] Add migration guide. Remove unnecessary args in handle_user_collection_request --- docs/userguide/src/migration/prefix.md | 17 +++++++++++++++++ src/memory_manager.rs | 2 +- src/mmtk.rs | 18 ++++-------------- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/docs/userguide/src/migration/prefix.md b/docs/userguide/src/migration/prefix.md index c32108bba8..88224957a3 100644 --- a/docs/userguide/src/migration/prefix.md +++ b/docs/userguide/src/migration/prefix.md @@ -75,6 +75,23 @@ See also: - PR: - Example: +### Remove GC in `harness_begin` + +```admonish tldr +`harness_begin` no longer triggers a GC before collecting statistics. The user application +is responsible to trigger a GC before calling `harness_begin`. +``` + +Not API change, but worth noting: + +* module `mmtk::memory_manager` + * `harness_begin` + * The function used to trigger a forced full heap GC before starting collecting statistics. + Now it no longer triggers the GC. + * The user applications and benchmarks are responsible to trigger a GC before calling `harness_begin`. + * `handle_user_collection_request` + * The runtime option `full_heap_sytem_gc` defaults to `true` now (it used to be `false` by default). + The GC triggered by this function will be a full heap GC by default. ## 0.25.0 diff --git a/src/memory_manager.rs b/src/memory_manager.rs index 97503260e5..e5d10aa23e 100644 --- a/src/memory_manager.rs +++ b/src/memory_manager.rs @@ -572,7 +572,7 @@ pub fn total_bytes(mmtk: &MMTK) -> usize { /// * `mmtk`: A reference to an MMTk instance. /// * `tls`: The thread that triggers this collection request. pub fn handle_user_collection_request(mmtk: &MMTK, tls: VMMutatorThread) { - mmtk.handle_user_collection_request(tls, false, false); + mmtk.handle_user_collection_request(tls); } /// Is the object alive? diff --git a/src/mmtk.rs b/src/mmtk.rs index be7819143d..1f63b21bf6 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -401,17 +401,13 @@ impl MMTK { } /// The application code has requested a collection. This is just a GC hint, and - /// we may ignore it. + /// we may ignore it (See `ignore_system_gc` and `full_heap_system_gc` in [`mmtk::util::options::Options`]). /// /// # Arguments /// * `tls`: The mutator thread that requests the GC - /// * `force`: The request cannot be ignored (except for NoGC) - /// * `exhaustive`: The requested GC should be exhaustive. This is also a hint. pub fn handle_user_collection_request( &self, - tls: VMMutatorThread, - force: bool, - exhaustive: bool, + tls: VMMutatorThread ) { use crate::vm::Collection; if !self.get_plan().constraints().collects_garbage { @@ -419,17 +415,11 @@ impl MMTK { return; } - if force || !*self.options.ignore_system_gc && VM::VMCollection::is_collection_enabled() { + if !*self.options.ignore_system_gc && VM::VMCollection::is_collection_enabled() { info!("User triggering collection"); - if exhaustive { - if let Some(gen) = self.get_plan().generational() { - gen.force_full_heap_collection(); - } - } - self.state .user_triggered_collection - .store(true, Ordering::Relaxed); + .store(true, Ordering::SeqCst); self.gc_requester.request(); VM::VMCollection::block_for_gc(tls); } From a19a2a43a722c2af9df1df35187597ac0be68b45 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Fri, 24 May 2024 04:06:02 +0000 Subject: [PATCH 3/6] cargo fmt --- src/mmtk.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/mmtk.rs b/src/mmtk.rs index 1f63b21bf6..e960c241de 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -405,10 +405,7 @@ impl MMTK { /// /// # Arguments /// * `tls`: The mutator thread that requests the GC - pub fn handle_user_collection_request( - &self, - tls: VMMutatorThread - ) { + pub fn handle_user_collection_request(&self, tls: VMMutatorThread) { use crate::vm::Collection; if !self.get_plan().constraints().collects_garbage { warn!("User attempted a collection request, but the plan can not do GC. The request is ignored."); From ab6de725c9b71560bd784dd5e3789427d13d2f7c Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Fri, 24 May 2024 04:19:26 +0000 Subject: [PATCH 4/6] Fix wrong doc --- src/mmtk.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mmtk.rs b/src/mmtk.rs index e960c241de..2cbbce2c91 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -401,7 +401,7 @@ impl MMTK { } /// The application code has requested a collection. This is just a GC hint, and - /// we may ignore it (See `ignore_system_gc` and `full_heap_system_gc` in [`mmtk::util::options::Options`]). + /// we may ignore it (See `ignore_system_gc` and `full_heap_system_gc` in [`crate::util::options::Options`]). /// /// # Arguments /// * `tls`: The mutator thread that requests the GC From b257517ff46b6bba43706b4ea5c961199e8474c5 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 5 Jun 2024 00:57:41 +0000 Subject: [PATCH 5/6] Introduce exhaustive:bool for handle_user_collection_request. Remove ful_heap_system_gc from options. --- src/global_state.rs | 7 ++++++- src/memory_manager.rs | 9 +++++++-- src/mmtk.rs | 6 +++++- src/plan/generational/global.rs | 7 ++++++- src/plan/immix/global.rs | 5 ++++- src/plan/sticky/immix/global.rs | 8 +++++++- src/util/options.rs | 2 -- 7 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/global_state.rs b/src/global_state.rs index f355e697f2..0d306051a6 100644 --- a/src/global_state.rs +++ b/src/global_state.rs @@ -25,6 +25,8 @@ pub struct GlobalState { pub(crate) emergency_collection: AtomicBool, /// Is the current GC triggered by the user? pub(crate) user_triggered_collection: AtomicBool, + /// Is the current user triggered GC an exhaustive GC (e.g. full heap)? + pub(crate) user_triggered_exhaustive_gc: AtomicBool, /// Is the current GC triggered internally by MMTK? This is unused for now. We may have internally triggered GC /// for a concurrent plan. pub(crate) internal_triggered_collection: AtomicBool, @@ -123,7 +125,9 @@ impl GlobalState { self.internal_triggered_collection .store(false, Ordering::SeqCst); self.user_triggered_collection - .store(false, Ordering::Relaxed); + .store(false, Ordering::SeqCst); + self.user_triggered_exhaustive_gc + .store(false, Ordering::SeqCst); } /// Are the stacks scanned? @@ -204,6 +208,7 @@ impl Default for GlobalState { stacks_prepared: AtomicBool::new(false), emergency_collection: AtomicBool::new(false), user_triggered_collection: AtomicBool::new(false), + user_triggered_exhaustive_gc: AtomicBool::new(false), internal_triggered_collection: AtomicBool::new(false), last_internal_triggered_collection: AtomicBool::new(false), allocation_success: AtomicBool::new(false), diff --git a/src/memory_manager.rs b/src/memory_manager.rs index e5d10aa23e..b747d7fd35 100644 --- a/src/memory_manager.rs +++ b/src/memory_manager.rs @@ -571,8 +571,13 @@ pub fn total_bytes(mmtk: &MMTK) -> usize { /// Arguments: /// * `mmtk`: A reference to an MMTk instance. /// * `tls`: The thread that triggers this collection request. -pub fn handle_user_collection_request(mmtk: &MMTK, tls: VMMutatorThread) { - mmtk.handle_user_collection_request(tls); +/// * `exhaustive`: The GC should be exhaustive (e.g. full heap GC, compaction GCs, etc.) +pub fn handle_user_collection_request( + mmtk: &MMTK, + tls: VMMutatorThread, + exhaustive: bool, +) { + mmtk.handle_user_collection_request(tls, exhaustive); } /// Is the object alive? diff --git a/src/mmtk.rs b/src/mmtk.rs index 2cbbce2c91..d3b68ed623 100644 --- a/src/mmtk.rs +++ b/src/mmtk.rs @@ -405,7 +405,8 @@ impl MMTK { /// /// # Arguments /// * `tls`: The mutator thread that requests the GC - pub fn handle_user_collection_request(&self, tls: VMMutatorThread) { + /// * `exhaustive`: The GC should be exhaustive (e.g. full heap GC, compaction GCs, etc.) + pub fn handle_user_collection_request(&self, tls: VMMutatorThread, exhaustive: bool) { use crate::vm::Collection; if !self.get_plan().constraints().collects_garbage { warn!("User attempted a collection request, but the plan can not do GC. The request is ignored."); @@ -417,6 +418,9 @@ impl MMTK { self.state .user_triggered_collection .store(true, Ordering::SeqCst); + self.state + .user_triggered_exhaustive_gc + .store(exhaustive, Ordering::SeqCst); self.gc_requester.request(); VM::VMCollection::block_for_gc(tls); } diff --git a/src/plan/generational/global.rs b/src/plan/generational/global.rs index 55054c7388..87d3645ba2 100644 --- a/src/plan/generational/global.rs +++ b/src/plan/generational/global.rs @@ -152,7 +152,12 @@ impl CommonGenPlan { .global_state .user_triggered_collection .load(Ordering::SeqCst) - && *self.common.base.options.full_heap_system_gc + && self + .common + .base + .global_state + .user_triggered_exhaustive_gc + .load(Ordering::SeqCst) { trace!("full heap: user triggered"); // User triggered collection, and we force full heap for user triggered collection diff --git a/src/plan/immix/global.rs b/src/plan/immix/global.rs index 6292b01aa2..17450eac94 100644 --- a/src/plan/immix/global.rs +++ b/src/plan/immix/global.rs @@ -180,7 +180,10 @@ impl Immix { .cur_collection_attempts .load(Ordering::SeqCst), plan.base().global_state.is_user_triggered_collection(), - *plan.base().options.full_heap_system_gc, + plan.base() + .global_state + .user_triggered_exhaustive_gc + .load(Ordering::SeqCst), ); if in_defrag { diff --git a/src/plan/sticky/immix/global.rs b/src/plan/sticky/immix/global.rs index 4a0d9ab14c..9628f908eb 100644 --- a/src/plan/sticky/immix/global.rs +++ b/src/plan/sticky/immix/global.rs @@ -354,7 +354,13 @@ impl StickyImmix { .global_state .user_triggered_collection .load(Ordering::SeqCst) - && *self.immix.common.base.options.full_heap_system_gc + && self + .immix + .common + .base + .global_state + .user_triggered_exhaustive_gc + .load(Ordering::SeqCst) { // User triggered collection, and we force full heap for user triggered collection true diff --git a/src/util/options.rs b/src/util/options.rs index a83781b9a0..30a6644f0f 100644 --- a/src/util/options.rs +++ b/src/util/options.rs @@ -806,8 +806,6 @@ options! { /// to 10% of the heap size while using the default value for max nursery. nursery: NurserySize [env_var: true, command_line: true] [|v: &NurserySize| v.validate()] = NurserySize::ProportionalBounded { min: DEFAULT_PROPORTIONAL_MIN_NURSERY, max: DEFAULT_PROPORTIONAL_MAX_NURSERY }, - /// Should a major GC be performed when a system GC is required? - full_heap_system_gc: bool [env_var: true, command_line: true] [always_valid] = true, /// Should finalization be disabled? no_finalizer: bool [env_var: true, command_line: true] [always_valid] = false, /// Should reference type processing be disabled? From 7c872dda51cd9ca68ab8e49d900d2e6f0a3fa6b6 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 5 Jun 2024 01:02:59 +0000 Subject: [PATCH 6/6] Update migration guide --- docs/userguide/src/migration/prefix.md | 41 +++++++++++++++----------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/docs/userguide/src/migration/prefix.md b/docs/userguide/src/migration/prefix.md index 88224957a3..abb987469e 100644 --- a/docs/userguide/src/migration/prefix.md +++ b/docs/userguide/src/migration/prefix.md @@ -32,6 +32,29 @@ Notes for the mmtk-core developers: ## 0.26.0 +### Remove GC in `harness_begin` + +```admonish tldr +`harness_begin` no longer triggers a GC before collecting statistics. The user application +is responsible to trigger a GC before calling `harness_begin`. +``` + +API changes: +* module `memory_manager` + * `handle_user_collection_request` + * It now takes an argument `exhaustive` to indicate if the user triggered GC is + exhaustive (full heap) or not. +* type `Options` + * The runtime option `full_heap_sytem_gc` is removed. + +Not API change, but worth noting: + +* module `mmtk::memory_manager` + * `harness_begin` + * The function used to trigger a forced full heap GC before starting collecting statistics. + Now it no longer triggers the GC. + * The user applications and benchmarks are responsible to trigger a GC before calling `harness_begin`. + ### Rename "edge" to "slot" ```admonish tldr @@ -75,24 +98,6 @@ See also: - PR: - Example: -### Remove GC in `harness_begin` - -```admonish tldr -`harness_begin` no longer triggers a GC before collecting statistics. The user application -is responsible to trigger a GC before calling `harness_begin`. -``` - -Not API change, but worth noting: - -* module `mmtk::memory_manager` - * `harness_begin` - * The function used to trigger a forced full heap GC before starting collecting statistics. - Now it no longer triggers the GC. - * The user applications and benchmarks are responsible to trigger a GC before calling `harness_begin`. - * `handle_user_collection_request` - * The runtime option `full_heap_sytem_gc` defaults to `true` now (it used to be `false` by default). - The GC triggered by this function will be a full heap GC by default. - ## 0.25.0 ### `ObjectReference` is no longer nullable