Fix issues in sanity GC (#1079)

This PR fixes two issues in sanity GC.
* It fixes #1078 by removing the
code to prepare/release mutator/collector, which seems unnecessary.
* It replaces the use of `ScanObjects` for cached root nodes with
`ProcessRootNode`. Otherwise, the assertion `assert!(!self.roots())`
will fail in `ScanObjectsWork::do_work_common`.

Author: qinsoon

.github/workflows/merge-check.yml

@@ -25,3 +25,9 @@ jobs:
           checkInterval: 600
         env:
           GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
+      - name: Check result
+        if: ${{ steps.waitforstatuschecks.outputs.status != 'success' }}
+        uses: actions/github-script@v3
+        with:
+          script: |
+            core.setFailed('Status checks failed')

docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs

@@ -78,8 +78,8 @@ impl<VM: VMBinding> ProcessEdgesWork for MyGCProcessEdges<VM> {
         }
     }
 
-    fn create_scan_work(&self, nodes: Vec<ObjectReference>, roots: bool) -> ScanObjects<Self> {
-        ScanObjects::<Self>::new(nodes, false, roots, self.bucket)
+    fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> ScanObjects<Self> {
+        ScanObjects::<Self>::new(nodes, false, self.bucket)
     }
 }
 // ANCHOR_END: mygc_process_edges_impl

src/plan/generational/gc_work.rs

@@ -57,12 +57,8 @@ impl<VM: VMBinding, P: GenerationalPlanExt<VM> + PlanTraceObject<VM>> ProcessEdg
         }
     }
 
-    fn create_scan_work(
-        &self,
-        nodes: Vec<ObjectReference>,
-        roots: bool,
-    ) -> Self::ScanObjectsWorkType {
-        PlanScanObjects::new(self.plan, nodes, false, roots, self.bucket)
+    fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType {
+        PlanScanObjects::new(self.plan, nodes, false, self.bucket)
     }
 }
 
@@ -122,7 +118,7 @@ impl<E: ProcessEdgesWork> GCWork<E::VM> for ProcessModBuf<E> {
             // Scan objects in the modbuf and forward pointers
             let modbuf = std::mem::take(&mut self.modbuf);
             GCWork::do_work(
-                &mut ScanObjects::<E>::new(modbuf, false, false, WorkBucketStage::Closure),
+                &mut ScanObjects::<E>::new(modbuf, false, WorkBucketStage::Closure),
                 worker,
                 mmtk,
             )

src/scheduler/gc_work.rs

@@ -297,7 +297,7 @@ impl<E: ProcessEdgesWork> ProcessEdgesWorkTracer<E> {
     fn flush(&mut self) {
         let next_nodes = self.process_edges_work.pop_nodes();
         assert!(!next_nodes.is_empty());
-        let work_packet = self.process_edges_work.create_scan_work(next_nodes, false);
+        let work_packet = self.process_edges_work.create_scan_work(next_nodes);
         let worker = self.process_edges_work.worker();
         worker.scheduler().work_buckets[self.stage].add(work_packet);
     }
@@ -641,18 +641,14 @@ pub trait ProcessEdgesWork:
     ///
     /// `roots` indicates if we are creating a packet for root scanning.  It is only true when this
     /// method is called to handle `RootsWorkFactory::create_process_pinning_roots_work`.
-    fn create_scan_work(
-        &self,
-        nodes: Vec<ObjectReference>,
-        roots: bool,
-    ) -> Self::ScanObjectsWorkType;
+    fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType;
 
     /// Flush the nodes in ProcessEdgesBase, and create a ScanObjects work packet for it. If the node set is empty,
     /// this method will simply return with no work packet created.
     fn flush(&mut self) {
         let nodes = self.pop_nodes();
         if !nodes.is_empty() {
-            self.start_or_dispatch_scan_work(self.create_scan_work(nodes, false));
+            self.start_or_dispatch_scan_work(self.create_scan_work(nodes));
         }
     }
 
@@ -732,8 +728,8 @@ impl<VM: VMBinding> ProcessEdgesWork for SFTProcessEdges<VM> {
         sft.sft_trace_object(&mut self.base.nodes, object, worker)
     }
 
-    fn create_scan_work(&self, nodes: Vec<ObjectReference>, roots: bool) -> ScanObjects<Self> {
-        ScanObjects::<Self>::new(nodes, false, roots, self.bucket)
+    fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> ScanObjects<Self> {
+        ScanObjects::<Self>::new(nodes, false, self.bucket)
     }
 }
 pub(crate) struct ProcessEdgesWorkRootsWorkFactory<
@@ -815,10 +811,6 @@ pub trait ScanObjectsWork<VM: VMBinding>: GCWork<VM> + Sized {
     /// The associated ProcessEdgesWork for processing the edges of the objects in this packet.
     type E: ProcessEdgesWork<VM = VM>;
 
-    /// Return true if the objects in this packet are pointed by roots, in which case we need to
-    /// call trace_object on them.
-    fn roots(&self) -> bool;
-
     /// Called after each object is scanned.
     fn post_scan_object(&self, object: ObjectReference);
 
@@ -836,7 +828,6 @@ pub trait ScanObjectsWork<VM: VMBinding>: GCWork<VM> + Sized {
         _mmtk: &'static MMTK<<Self::E as ProcessEdgesWork>::VM>,
     ) {
         let tls = worker.tls;
-        debug_assert!(!self.roots());
 
         // Scan the nodes in the buffer.
         let objects_to_scan = buffer;
@@ -904,22 +895,15 @@ pub struct ScanObjects<Edges: ProcessEdgesWork> {
     buffer: Vec<ObjectReference>,
     #[allow(unused)]
     concurrent: bool,
-    roots: bool,
     phantom: PhantomData<Edges>,
     bucket: WorkBucketStage,
 }
 
 impl<Edges: ProcessEdgesWork> ScanObjects<Edges> {
-    pub fn new(
-        buffer: Vec<ObjectReference>,
-        concurrent: bool,
-        roots: bool,
-        bucket: WorkBucketStage,
-    ) -> Self {
+    pub fn new(buffer: Vec<ObjectReference>, concurrent: bool, bucket: WorkBucketStage) -> Self {
         Self {
             buffer,
             concurrent,
-            roots,
             phantom: PhantomData,
             bucket,
         }
@@ -929,10 +913,6 @@ impl<Edges: ProcessEdgesWork> ScanObjects<Edges> {
 impl<VM: VMBinding, E: ProcessEdgesWork<VM = VM>> ScanObjectsWork<VM> for ScanObjects<E> {
     type E = E;
 
-    fn roots(&self) -> bool {
-        self.roots
-    }
-
     fn get_bucket(&self) -> WorkBucketStage {
         self.bucket
     }
@@ -942,7 +922,7 @@ impl<VM: VMBinding, E: ProcessEdgesWork<VM = VM>> ScanObjectsWork<VM> for ScanOb
     }
 
     fn make_another(&self, buffer: Vec<ObjectReference>) -> Self {
-        Self::new(buffer, self.concurrent, false, self.bucket)
+        Self::new(buffer, self.concurrent, self.bucket)
     }
 }
 
@@ -987,12 +967,8 @@ impl<VM: VMBinding, P: PlanTraceObject<VM> + Plan<VM = VM>, const KIND: TraceKin
         Self { plan, base }
     }
 
-    fn create_scan_work(
-        &self,
-        nodes: Vec<ObjectReference>,
-        roots: bool,
-    ) -> Self::ScanObjectsWorkType {
-        PlanScanObjects::<Self, P>::new(self.plan, nodes, false, roots, self.bucket)
+    fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType {
+        PlanScanObjects::<Self, P>::new(self.plan, nodes, false, self.bucket)
     }
 
     fn trace_object(&mut self, object: ObjectReference) -> ObjectReference {
@@ -1041,7 +1017,6 @@ pub struct PlanScanObjects<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceO
     buffer: Vec<ObjectReference>,
     #[allow(dead_code)]
     concurrent: bool,
-    roots: bool,
     phantom: PhantomData<E>,
     bucket: WorkBucketStage,
 }
@@ -1051,14 +1026,12 @@ impl<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceObject<E::VM>> PlanScan
         plan: &'static P,
         buffer: Vec<ObjectReference>,
         concurrent: bool,
-        roots: bool,
         bucket: WorkBucketStage,
     ) -> Self {
         Self {
             plan,
             buffer,
             concurrent,
-            roots,
             phantom: PhantomData,
             bucket,
         }
@@ -1070,10 +1043,6 @@ impl<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceObject<E::VM>> ScanObje
 {
     type E = E;
 
-    fn roots(&self) -> bool {
-        self.roots
-    }
-
     fn get_bucket(&self) -> WorkBucketStage {
         self.bucket
     }
@@ -1083,7 +1052,7 @@ impl<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceObject<E::VM>> ScanObje
     }
 
     fn make_another(&self, buffer: Vec<ObjectReference>) -> Self {
-        Self::new(self.plan, buffer, self.concurrent, false, self.bucket)
+        Self::new(self.plan, buffer, self.concurrent, self.bucket)
     }
 }
 
@@ -1101,7 +1070,11 @@ impl<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceObject<E::VM>> GCWork<E
 /// but creates the work to scan these objects using E. This is necessary to guarantee that these objects do not move
 /// (`I` should trace them without moving) as we do not have the information about the edges pointing to them.
 
-struct ProcessRootNode<VM: VMBinding, I: ProcessEdgesWork<VM = VM>, E: ProcessEdgesWork<VM = VM>> {
+pub(crate) struct ProcessRootNode<
+    VM: VMBinding,
+    I: ProcessEdgesWork<VM = VM>,
+    E: ProcessEdgesWork<VM = VM>,
+> {
     phantom: PhantomData<(VM, I, E)>,
     roots: Vec<ObjectReference>,
     bucket: WorkBucketStage,
@@ -1165,7 +1138,7 @@ impl<VM: VMBinding, I: ProcessEdgesWork<VM = VM>, E: ProcessEdgesWork<VM = VM>>
         };
 
         let process_edges_work = E::new(vec![], false, mmtk, self.bucket);
-        let work = process_edges_work.create_scan_work(scanned_root_objects, false);
+        let work = process_edges_work.create_scan_work(scanned_root_objects);
         crate::memory_manager::add_work_packet(mmtk, self.bucket, work);
 
         trace!("ProcessRootNode End");
@@ -1210,11 +1183,7 @@ impl<VM: VMBinding> ProcessEdgesWork for UnsupportedProcessEdges<VM> {
         panic!("unsupported!")
     }
 
-    fn create_scan_work(
-        &self,
-        _nodes: Vec<ObjectReference>,
-        _roots: bool,
-    ) -> Self::ScanObjectsWorkType {
+    fn create_scan_work(&self, _nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType {
         panic!("unsupported!")
     }
 }

src/util/sanity/sanity_checker.rs

@@ -96,12 +96,12 @@ impl<P: Plan> GCWork<P::VM> for ScheduleSanityGC<P> {
                 );
             }
             for roots in &sanity_checker.root_nodes {
-                scheduler.work_buckets[WorkBucketStage::Closure].add(ScanObjects::<
+                scheduler.work_buckets[WorkBucketStage::Closure].add(ProcessRootNode::<
+                    P::VM,
+                    SanityGCProcessEdges<P::VM>,
                     SanityGCProcessEdges<P::VM>,
                 >::new(
                     roots.clone(),
-                    false,
-                    true,
                     WorkBucketStage::Closure,
                 ));
             }
@@ -132,14 +132,6 @@ impl<P: Plan> GCWork<P::VM> for SanityPrepare<P> {
             let mut sanity_checker = mmtk.sanity_checker.lock().unwrap();
             sanity_checker.refs.clear();
         }
-        for mutator in <P::VM as VMBinding>::VMActivePlan::mutators() {
-            mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
-                .add(PrepareMutator::<P::VM>::new(mutator));
-        }
-        for w in &mmtk.scheduler.worker_group.workers_shared {
-            let result = w.designated_work.push(Box::new(PrepareCollector));
-            debug_assert!(result.is_ok());
-        }
     }
 }
 
@@ -157,14 +149,6 @@ impl<P: Plan> GCWork<P::VM> for SanityRelease<P> {
     fn do_work(&mut self, _worker: &mut GCWorker<P::VM>, mmtk: &'static MMTK<P::VM>) {
         info!("Sanity GC release");
         mmtk.sanity_checker.lock().unwrap().clear_roots_cache();
-        for mutator in <P::VM as VMBinding>::VMActivePlan::mutators() {
-            mmtk.scheduler.work_buckets[WorkBucketStage::Release]
-                .add(ReleaseMutator::<P::VM>::new(mutator));
-        }
-        for w in &mmtk.scheduler.worker_group.workers_shared {
-            let result = w.designated_work.push(Box::new(ReleaseCollector));
-            debug_assert!(result.is_ok());
-        }
         mmtk.sanity_end();
     }
 }
@@ -243,11 +227,7 @@ impl<VM: VMBinding> ProcessEdgesWork for SanityGCProcessEdges<VM> {
         object
     }
 
-    fn create_scan_work(
-        &self,
-        nodes: Vec<ObjectReference>,
-        roots: bool,
-    ) -> Self::ScanObjectsWorkType {
-        ScanObjects::<Self>::new(nodes, false, roots, WorkBucketStage::Closure)
+    fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType {
+        ScanObjects::<Self>::new(nodes, false, WorkBucketStage::Closure)
     }
 }