NULL and movement check in process_edge (#1032)

This PR does two things:

1. `ProcessEdgesWork::process_edge` will skip slots that are not holding
references (i.e. if `Edge::load()` returns `ObjectReference::NULL`).
2. `ProcessEdgesWork::process_edge` will skip the `Edge::store()` if the
object is not moved.

Doing (1) removes unnecessary invocations of `trace_object()` as well as
the subsequent `Edge::store()`. It also allows slots to hold
non-reference tagged values. In that case, the VM binding can return
`ObjectReference::NULL` in `Edge::load()` so that mmtk-core will simply
skip the slot, fixing #1031

Doing (2) removes unnecessary `Edge::store()` operations in the case
where the objects are not moved during `trace_object`. It reduces the
STW time in most cases, fixing
#574

Fixes: #1031
Fixes: #574

Author: wks

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

@@ -56,9 +56,7 @@ impl<VM: VMBinding> ProcessEdgesWork for MyGCProcessEdges<VM> {
     }
 
     fn trace_object(&mut self, object: ObjectReference) -> ObjectReference {
-        if object.is_null() {
-            return object;
-        }
+        debug_assert!(!object.is_null());
         let worker = self.worker();
         let queue = &mut self.base.nodes;
         if self.plan.tospace().in_space(object) {

docs/userguide/src/tutorial/mygc/ss/exercise_solution.md

@@ -149,9 +149,7 @@ In `gc_work.rs`:
  the tospace and fromspace:
     ```rust
         fn trace_object(&mut self, object: ObjectReference) -> ObjectReference {
-            if object.is_null() {
-                return object;
-            }
+            debug_assert!(!object.is_null());
 
             // Add this!
             else if self.plan().youngspace().in_space(object) {

src/plan/generational/gc_work.rs

@@ -36,19 +36,25 @@ impl<VM: VMBinding, P: GenerationalPlanExt<VM> + PlanTraceObject<VM>> ProcessEdg
         Self { plan, base }
     }
     fn trace_object(&mut self, object: ObjectReference) -> ObjectReference {
-        if object.is_null() {
-            return object;
-        }
+        debug_assert!(!object.is_null());
+
         // We cannot borrow `self` twice in a call, so we extract `worker` as a local variable.
         let worker = self.worker();
         self.plan
             .trace_object_nursery(&mut self.base.nodes, object, worker)
     }
     fn process_edge(&mut self, slot: EdgeOf<Self>) {
         let object = slot.load();
+        if object.is_null() {
+            return;
+        }
         let new_object = self.trace_object(object);
         debug_assert!(!self.plan.is_object_in_nursery(new_object));
-        slot.store(new_object);
+        // Note: If `object` is a mature object, `trace_object` will not call `space.trace_object`,
+        // but will still return `object`.  In that case, we don't need to write it back.
+        if new_object != object {
+            slot.store(new_object);
+        }
     }
 
     fn create_scan_work(

src/policy/copyspace.rs

@@ -217,6 +217,7 @@ impl<VM: VMBinding> CopySpace<VM> {
         worker: &mut GCWorker<VM>,
     ) -> ObjectReference {
         trace!("copyspace.trace_object(, {:?}, {:?})", object, semantics,);
+        debug_assert!(!object.is_null());
 
         // If this is not from space, we do not need to trace it (the object has been copied to the tosapce)
         if !self.is_from_space() {

src/policy/immix/immixspace.rs

@@ -184,6 +184,7 @@ impl<VM: VMBinding> crate::policy::gc_work::PolicyTraceObject<VM> for ImmixSpace
         copy: Option<CopySemantics>,
         worker: &mut GCWorker<VM>,
     ) -> ObjectReference {
+        debug_assert!(!object.is_null());
         if KIND == TRACE_KIND_TRANSITIVE_PIN {
             self.trace_object_without_moving(queue, object)
         } else if KIND == TRACE_KIND_DEFRAG {

src/policy/immortalspace.rs

@@ -187,6 +187,7 @@ impl<VM: VMBinding> ImmortalSpace<VM> {
         queue: &mut Q,
         object: ObjectReference,
     ) -> ObjectReference {
+        debug_assert!(!object.is_null());
         #[cfg(feature = "vo_bit")]
         debug_assert!(
             crate::util::metadata::vo_bit::is_vo_bit_set::<VM>(object),

src/policy/largeobjectspace.rs

@@ -189,6 +189,7 @@ impl<VM: VMBinding> LargeObjectSpace<VM> {
         queue: &mut Q,
         object: ObjectReference,
     ) -> ObjectReference {
+        debug_assert!(!object.is_null());
         #[cfg(feature = "vo_bit")]
         debug_assert!(
             crate::util::metadata::vo_bit::is_vo_bit_set::<VM>(object),

src/policy/markcompactspace.rs

@@ -130,6 +130,7 @@ impl<VM: VMBinding> crate::policy::gc_work::PolicyTraceObject<VM> for MarkCompac
         _copy: Option<CopySemantics>,
         _worker: &mut GCWorker<VM>,
     ) -> ObjectReference {
+        debug_assert!(!object.is_null());
         debug_assert!(
             KIND != TRACE_KIND_TRANSITIVE_PIN,
             "MarkCompact does not support transitive pin trace."

src/policy/marksweepspace/malloc_ms/global.rs

@@ -400,9 +400,7 @@ impl<VM: VMBinding> MallocSpace<VM> {
         queue: &mut Q,
         object: ObjectReference,
     ) -> ObjectReference {
-        if object.is_null() {
-            return object;
-        }
+        debug_assert!(!object.is_null());
 
         assert!(
             self.in_space(object),

src/policy/marksweepspace/native_ms/global.rs

@@ -241,9 +241,7 @@ impl<VM: VMBinding> MarkSweepSpace<VM> {
         queue: &mut Q,
         object: ObjectReference,
     ) -> ObjectReference {
-        if object.is_null() {
-            return object;
-        }
+        debug_assert!(!object.is_null());
         debug_assert!(
             self.in_space(object),
             "Cannot mark an object {} that was not alloced by free list allocator.",