Extract set_collection_kind and set_gc_status (#957)

qinsoon · web-flow · commit 29d482cf7b47 · 2023-10-16T06:19:46.000Z
This PR extracts `set_collection_kind` and `set_gc_status` from each plan's `schedule_collection`, and calls them directly in the `ScheduleCollection` work packet. This removes duplicated code, and also makes our implementation consistent with Java MMTk. For generational plans, in `schedule_collection` in each plan, our current code calls `requires_full_heap_collection` first, and then call `set_collection_kind`. With the changes in the PR, `requires_full_heap_collection` is called after `set_collection_kind`, which is the same as in Java MMTk ([super.collectionPhase(phaseId);](https://github.com/JikesRVM/JikesRVM/blob/5072f19761115d987b6ee162f49a03522d36c697/MMTk/src/org/mmtk/plan/generational/Gen.java#L172-L173)). The implications are: 1. When `set_collection_kind` forces a full heap GC, and `set_collection_kind` is called before `requires_full_heap_collection`, we will do a full heap GC in the current GC (which is the expected behavior in Java MMTk). 2. `set_collection_kind` may increase `cur_collection_attempt`, which makes it more likely to trigger an emergency GC, and makes it more likely to trigger a full heap GC. In summary, we miss some opportunities where we should do a full heap GC. With the changes, we should properly trigger full heap GCs when we should.
diff --git a/docs/userguide/src/tutorial/code/mygc_semispace/global.rs b/docs/userguide/src/tutorial/code/mygc_semispace/global.rs
@@ -1,7 +1,6 @@
 // ANCHOR: imports_no_gc_work
 use crate::plan::global::BasePlan; //Modify
 use crate::plan::global::CommonPlan; // Add
-use crate::plan::global::GcStatus; // Add
 use crate::plan::global::{CreateGeneralPlanArgs, CreateSpecificPlanArgs};
 use crate::plan::mygc::mutator::ALLOCATOR_MAPPING;
 use crate::plan::mygc::gc_work::MyGCWorkContext;
@@ -77,8 +76,6 @@ impl<VM: VMBinding> Plan for MyGC<VM> {
     // Modify
     // ANCHOR: schedule_collection
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
         scheduler.schedule_common_work::<MyGCWorkContext<VM>>(self);
     }
     // ANCHOR_END: schedule_collection
diff --git a/src/plan/generational/copying/global.rs b/src/plan/generational/copying/global.rs
@@ -8,7 +8,6 @@ use crate::plan::global::BasePlan;
 use crate::plan::global::CommonPlan;
 use crate::plan::global::CreateGeneralPlanArgs;
 use crate::plan::global::CreateSpecificPlanArgs;
-use crate::plan::global::GcStatus;
 use crate::plan::AllocationSemantics;
 use crate::plan::Plan;
 use crate::plan::PlanConstraints;
@@ -73,8 +72,6 @@ impl<VM: VMBinding> Plan for GenCopy<VM> {
 
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {
         let is_full_heap = self.requires_full_heap_collection();
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
         if is_full_heap {
             scheduler.schedule_common_work::<GenCopyGCWorkContext<VM>>(self);
         } else {
diff --git a/src/plan/generational/immix/global.rs b/src/plan/generational/immix/global.rs
@@ -6,7 +6,6 @@ use crate::plan::global::BasePlan;
 use crate::plan::global::CommonPlan;
 use crate::plan::global::CreateGeneralPlanArgs;
 use crate::plan::global::CreateSpecificPlanArgs;
-use crate::plan::global::GcStatus;
 use crate::plan::AllocationSemantics;
 use crate::plan::Plan;
 use crate::plan::PlanConstraints;
@@ -104,10 +103,6 @@ impl<VM: VMBinding> Plan for GenImmix<VM> {
     #[allow(clippy::branches_sharing_code)]
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<Self::VM>) {
         let is_full_heap = self.requires_full_heap_collection();
-
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
-
         if !is_full_heap {
             debug!("Nursery GC");
             scheduler.schedule_common_work::<GenImmixNurseryGCWorkContext<VM>>(self);
diff --git a/src/plan/global.rs b/src/plan/global.rs
@@ -691,7 +691,7 @@ impl<VM: VMBinding> BasePlan<VM> {
         self.vm_space.release();
     }
 
-    pub fn set_collection_kind<P: Plan>(&self, plan: &P) {
+    pub fn set_collection_kind(&self, plan: &dyn Plan<VM = VM>) {
         self.cur_collection_attempts.store(
             if self.is_user_triggered_collection() {
                 1
diff --git a/src/plan/immix/global.rs b/src/plan/immix/global.rs
@@ -4,7 +4,6 @@ use crate::plan::global::BasePlan;
 use crate::plan::global::CommonPlan;
 use crate::plan::global::CreateGeneralPlanArgs;
 use crate::plan::global::CreateSpecificPlanArgs;
-use crate::plan::global::GcStatus;
 use crate::plan::AllocationSemantics;
 use crate::plan::Plan;
 use crate::plan::PlanConstraints;
@@ -73,8 +72,6 @@ impl<VM: VMBinding> Plan for Immix<VM> {
     }
 
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
         Self::schedule_immix_full_heap_collection::<
             Immix<VM>,
             ImmixGCWorkContext<VM, TRACE_KIND_FAST>,
diff --git a/src/plan/markcompact/global.rs b/src/plan/markcompact/global.rs
@@ -4,7 +4,6 @@ use super::gc_work::{
     UpdateReferences,
 };
 use crate::plan::global::CommonPlan;
-use crate::plan::global::GcStatus;
 use crate::plan::global::{BasePlan, CreateGeneralPlanArgs, CreateSpecificPlanArgs};
 use crate::plan::markcompact::mutator::ALLOCATOR_MAPPING;
 use crate::plan::AllocationSemantics;
@@ -80,9 +79,6 @@ impl<VM: VMBinding> Plan for MarkCompact<VM> {
     }
 
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
-
         // TODO use schedule_common once it can work with markcompact
         // self.common()
         //     .schedule_common::<Self, MarkingProcessEdges<VM>, NoCopy<VM>>(
diff --git a/src/plan/marksweep/global.rs b/src/plan/marksweep/global.rs
@@ -2,7 +2,6 @@ use crate::plan::global::BasePlan;
 use crate::plan::global::CommonPlan;
 use crate::plan::global::CreateGeneralPlanArgs;
 use crate::plan::global::CreateSpecificPlanArgs;
-use crate::plan::global::GcStatus;
 use crate::plan::marksweep::gc_work::MSGCWorkContext;
 use crate::plan::marksweep::mutator::ALLOCATOR_MAPPING;
 use crate::plan::AllocationSemantics;
@@ -50,8 +49,6 @@ pub const MS_CONSTRAINTS: PlanConstraints = PlanConstraints {
 
 impl<VM: VMBinding> Plan for MarkSweep<VM> {
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
         scheduler.schedule_common_work::<MSGCWorkContext<VM>>(self);
     }
 
diff --git a/src/plan/pageprotect/global.rs b/src/plan/pageprotect/global.rs
@@ -2,7 +2,6 @@ use super::gc_work::PPGCWorkContext;
 use super::mutator::ALLOCATOR_MAPPING;
 use crate::plan::global::CreateGeneralPlanArgs;
 use crate::plan::global::CreateSpecificPlanArgs;
-use crate::plan::global::GcStatus;
 use crate::plan::AllocationSemantics;
 use crate::plan::Plan;
 use crate::plan::PlanConstraints;
@@ -40,8 +39,6 @@ impl<VM: VMBinding> Plan for PageProtect<VM> {
     }
 
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
         scheduler.schedule_common_work::<PPGCWorkContext<VM>>(self);
     }
 
diff --git a/src/plan/semispace/global.rs b/src/plan/semispace/global.rs
@@ -2,7 +2,6 @@ use super::gc_work::SSGCWorkContext;
 use crate::plan::global::CommonPlan;
 use crate::plan::global::CreateGeneralPlanArgs;
 use crate::plan::global::CreateSpecificPlanArgs;
-use crate::plan::global::GcStatus;
 use crate::plan::semispace::mutator::ALLOCATOR_MAPPING;
 use crate::plan::AllocationSemantics;
 use crate::plan::Plan;
@@ -67,8 +66,6 @@ impl<VM: VMBinding> Plan for SemiSpace<VM> {
     }
 
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
         scheduler.schedule_common_work::<SSGCWorkContext<VM>>(self);
     }
 
diff --git a/src/plan/sticky/immix/global.rs b/src/plan/sticky/immix/global.rs
@@ -3,7 +3,6 @@ use crate::plan::global::CommonPlan;
 use crate::plan::global::CreateGeneralPlanArgs;
 use crate::plan::global::CreateSpecificPlanArgs;
 use crate::plan::immix;
-use crate::plan::GcStatus;
 use crate::plan::PlanConstraints;
 use crate::policy::immix::ImmixSpace;
 use crate::policy::sft::SFT;
@@ -80,9 +79,6 @@ impl<VM: VMBinding> Plan for StickyImmix<VM> {
     }
 
     fn schedule_collection(&'static self, scheduler: &crate::scheduler::GCWorkScheduler<Self::VM>) {
-        self.base().set_collection_kind::<Self>(self);
-        self.base().set_gc_status(GcStatus::GcPrepare);
-
         let is_full_heap = self.requires_full_heap_collection();
         self.gc_full_heap.store(is_full_heap, Ordering::SeqCst);
 
diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs
@@ -14,14 +14,14 @@ pub struct ScheduleCollection;
 
 impl<VM: VMBinding> GCWork<VM> for ScheduleCollection {
     fn do_work(&mut self, worker: &mut GCWorker<VM>, mmtk: &'static MMTK<VM>) {
-        mmtk.get_plan().schedule_collection(worker.scheduler());
-
+        let plan = mmtk.get_plan();
         // Tell GC trigger that GC started.
-        // We now know what kind of GC this is (e.g. nursery vs mature in gen copy, defrag vs fast in Immix)
-        // TODO: Depending on the OS scheduling, other workers can run so fast that they can finish
-        // everything in the `Unconstrained` and the `Prepare` buckets before we execute the next
-        // statement. Consider if there is a better place to call `on_gc_start`.
-        mmtk.get_plan().base().gc_trigger.policy.on_gc_start(mmtk);
+        plan.base().gc_trigger.policy.on_gc_start(mmtk);
+
+        plan.base().set_collection_kind(plan);
+        plan.base().set_gc_status(GcStatus::GcPrepare);
+
+        plan.schedule_collection(worker.scheduler());
     }
 }
 

Original file line number	Diff line number	Diff line change
`@@ -691,7 +691,7 @@ impl<VM: VMBinding> BasePlan<VM> {`
`691`	`691`	`self.vm_space.release();`
`692`	`692`	`}`
`693`	`693`
`694`		`- pub fn set_collection_kind<P: Plan>(&self, plan: &P) {`
	`694`	`+ pub fn set_collection_kind(&self, plan: &dyn Plan<VM = VM>) {`
`695`	`695`	`self.cur_collection_attempts.store(`
`696`	`696`	`if self.is_user_triggered_collection() {`
`697`	`697`	`1`
Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,6 @@ use super::gc_work::PPGCWorkContext;`
`2`	`2`	`use super::mutator::ALLOCATOR_MAPPING;`
`3`	`3`	`use crate::plan::global::CreateGeneralPlanArgs;`
`4`	`4`	`use crate::plan::global::CreateSpecificPlanArgs;`
`5`		`-use crate::plan::global::GcStatus;`
`6`	`5`	`use crate::plan::AllocationSemantics;`
`7`	`6`	`use crate::plan::Plan;`
`8`	`7`	`use crate::plan::PlanConstraints;`
`@@ -40,8 +39,6 @@ impl<VM: VMBinding> Plan for PageProtect<VM> {`
`40`	`39`	`}`
`41`	`40`
`42`	`41`	`fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {`
`43`		`- self.base().set_collection_kind::<Self>(self);`
`44`		`- self.base().set_gc_status(GcStatus::GcPrepare);`
`45`	`42`	`scheduler.schedule_common_work::<PPGCWorkContext<VM>>(self);`
`46`	`43`	`}`
`47`	`44`
Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,6 @@ use super::gc_work::SSGCWorkContext;`
`2`	`2`	`use crate::plan::global::CommonPlan;`
`3`	`3`	`use crate::plan::global::CreateGeneralPlanArgs;`
`4`	`4`	`use crate::plan::global::CreateSpecificPlanArgs;`
`5`		`-use crate::plan::global::GcStatus;`
`6`	`5`	`use crate::plan::semispace::mutator::ALLOCATOR_MAPPING;`
`7`	`6`	`use crate::plan::AllocationSemantics;`
`8`	`7`	`use crate::plan::Plan;`
`@@ -67,8 +66,6 @@ impl<VM: VMBinding> Plan for SemiSpace<VM> {`
`67`	`66`	`}`
`68`	`67`
`69`	`68`	`fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {`
`70`		`- self.base().set_collection_kind::<Self>(self);`
`71`		`- self.base().set_gc_status(GcStatus::GcPrepare);`
`72`	`69`	`scheduler.schedule_common_work::<SSGCWorkContext<VM>>(self);`
`73`	`70`	`}`
`74`	`71`