NULL and movement check in process_edge

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.",

src/scheduler/gc_work.rs

@@ -273,11 +273,8 @@ pub(crate) struct ProcessEdgesWorkTracer<E: ProcessEdgesWork> {
 impl<E: ProcessEdgesWork> ObjectTracer for ProcessEdgesWorkTracer<E> {
  /// Forward the `trace_object` call to the underlying `ProcessEdgesWork`,
  /// and flush as soon as the underlying buffer of `process_edges_work` is full.
- ///
- /// This function is inlined because `trace_object` is probably the hottest function in MMTk.
- /// If this function is called in small closures, please profile the program and make sure the
- /// closure is inlined, too.
  fn trace_object(&mut self, object: ObjectReference) -> ObjectReference {
+ debug_assert!(!object.is_null());
  let result = self.process_edges_work.trace_object(object);
  self.flush_if_full();
  result
@@ -663,8 +660,12 @@ pub trait ProcessEdgesWork:
  /// trace the object and store back the new object reference if necessary.
  fn process_edge(&mut self, slot: EdgeOf<Self>) {
  let object = slot.load();
+ if object.is_null() {
+ return;
+ }
  let new_object = self.trace_object(object);
- if Self::OVERWRITE_REFERENCE {
+ debug_assert!(!new_object.is_null());
+ if Self::OVERWRITE_REFERENCE && new_object != object {
  slot.store(new_object);
  }
  }
@@ -721,9 +722,7 @@ impl<VM: VMBinding> ProcessEdgesWork for SFTProcessEdges<VM> {
  fn trace_object(&mut self, object: ObjectReference) -> ObjectReference {
  use crate::policy::sft::GCWorkerMutRef;
 
- if object.is_null() {
- return object;
- }
+ debug_assert!(!object.is_null());
 
  // Erase <VM> type parameter
  let worker = GCWorkerMutRef::new(self.worker());
@@ -997,9 +996,7 @@ impl<VM: VMBinding, P: PlanTraceObject<VM> + Plan<VM = VM>, const KIND: TraceKin
  }
 
  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
@@ -1008,8 +1005,12 @@ impl<VM: VMBinding, P: PlanTraceObject<VM> + Plan<VM = VM>, const KIND: TraceKin
 
  fn process_edge(&mut self, slot: EdgeOf<Self>) {
  let object = slot.load();
+ if object.is_null() {
+ return;
+ }
  let new_object = self.trace_object(object);
- if P::may_move_objects::<KIND>() {
+ debug_assert!(!new_object.is_null());
+ if P::may_move_objects::<KIND>() && new_object != object {
  slot.store(new_object);
  }
  }

src/util/reference_processor.rs

@@ -216,20 +216,23 @@ impl ReferenceProcessor {
  e: &mut E,
  referent: ObjectReference,
  ) -> ObjectReference {
+ debug_assert!(!referent.is_null());
  e.trace_object(referent)
  }
 
  fn get_forwarded_reference<E: ProcessEdgesWork>(
  e: &mut E,
  object: ObjectReference,
  ) -> ObjectReference {
+ debug_assert!(!object.is_null());
  e.trace_object(object)
  }
 
  fn keep_referent_alive<E: ProcessEdgesWork>(
  e: &mut E,
  referent: ObjectReference,
  ) -> ObjectReference {
+ debug_assert!(!referent.is_null());
  e.trace_object(referent)
  }
 
@@ -285,23 +288,29 @@ impl ReferenceProcessor {
  reference: ObjectReference,
  ) -> ObjectReference {
  let old_referent = <E::VM as VMBinding>::VMReferenceGlue::get_referent(reference);
- let new_referent = ReferenceProcessor::get_forwarded_referent(trace, old_referent);
- <E::VM as VMBinding>::VMReferenceGlue::set_referent(reference, new_referent);
- let new_reference = ReferenceProcessor::get_forwarded_reference(trace, reference);
  {
  use crate::vm::ObjectModel;
  trace!(
  "Forwarding reference: {} (size: {})",
  reference,
  <E::VM as VMBinding>::VMObjectModel::get_current_size(reference)
  );
+ }
+
+ if !<E::VM as VMBinding>::VMReferenceGlue::is_referent_cleared(old_referent) {
+ let new_referent = ReferenceProcessor::get_forwarded_referent(trace, old_referent);
+ <E::VM as VMBinding>::VMReferenceGlue::set_referent(reference, new_referent);
+
  trace!(
  " referent: {} (forwarded to {})",
  old_referent,
  new_referent
  );
- trace!(" reference: forwarded to {}", new_reference);
  }
+
+ let new_reference = ReferenceProcessor::get_forwarded_reference(trace, reference);
+ trace!(" reference: forwarded to {}", new_reference);
+
  debug_assert!(
  !new_reference.is_null(),
  "reference {:?}'s forwarding pointer is NULL",

src/vm/edge_shape.rs

@@ -7,8 +7,13 @@ use atomic::Atomic;
 use crate::util::constants::{BYTES_IN_ADDRESS, LOG_BYTES_IN_ADDRESS};
 use crate::util::{Address, ObjectReference};
 
-/// An abstract edge. An edge holds an object reference. When we load from it, we get an
-/// ObjectReference; we can also store an ObjectReference into it.
+/// An `Edge` represents a slot in an object (a.k.a. a field), on the stack (i.e. a local variable)
+/// or any other places (such as global variables). A slot may hold an object reference. We can
+/// load the object reference from it, and we can store an ObjectReference into it. For some VMs,
+/// a slot may sometimes not hold an object reference. For example, it can hold a special `NULL`
+/// pointer which does not point to any object, or it can hold a tagged non-reference value, such
+/// as small integers and special values such as `true`, `false`, `null` (a.k.a. "none", "nil",
+/// etc. for other VMs), `undefined`, etc.
 ///
 /// This intends to abstract out the differences of reference field representation among different
 /// VMs. If the VM represent a reference field as a word that holds the pointer to the object, it
@@ -40,10 +45,31 @@ use crate::util::{Address, ObjectReference};
 /// semantics, such as whether it holds strong or weak references. If a VM holds a weak reference
 /// in a word as a pointer, it can also use `SimpleEdge` for weak reference fields.
 pub trait Edge: Copy + Send + Debug + PartialEq + Eq + Hash {
- /// Load object reference from the edge.
+ /// Load object reference from the slot.
+ ///
+ /// If the slot is not holding an object reference (For example, if it is holding NULL or a
+ /// tagged non-reference value. See trait-level doc comment.), this method should return
+ /// `ObjectReference::NULL`.
+ ///
+ /// If the slot holds an object reference with tag bits, the returned value shall be the object
+ /// reference with the tag bits removed.
  fn load(&self) -> ObjectReference;
 
- /// Store the object reference `object` into the edge.
+ /// Store the object reference `object` into the slot.
+ ///
+ /// If the slot holds an object reference with tag bits, this method must preserve the tag
+ /// bits while updating the object reference so that it points to the forwarded object given by
+ /// the parameter `object`.
+ ///
+ /// FIXME: This design is inefficient for handling object references with tag bits. Consider
+ /// introducing a new updating function to do the load, trace and store in one function.
+ /// See: <https://github.com/mmtk/mmtk-core/issues/1033>
+ ///
+ /// FIXME: This method is currently used by both moving GC algorithms and the subsuming write
+ /// barrier ([`crate::memory_manager::object_reference_write`]). The two reference writing
+ /// operations have different semantics, and need to be implemented differently if the VM
+ /// supports offsetted or tagged references.
+ /// See: <https://github.com/mmtk/mmtk-core/issues/1038>
  fn store(&self, object: ObjectReference);
 
  /// Prefetch the edge so that a subsequent `load` will be faster.

src/vm/scanning.rs

@@ -26,11 +26,14 @@ impl<ES: Edge, F: FnMut(ES)> EdgeVisitor<ES> for F {
 
 /// Callback trait of scanning functions that directly trace through edges.
 pub trait ObjectTracer {
- /// Call this function for the content of each edge,
- /// and assign the returned value back to the edge.
+ /// Call this function to trace through an object graph edge which points to `object`.
+ /// `object` must point to a valid object, and cannot be `ObjectReference::NULL`.
  ///
- /// Note: This function is performance-critical.
- /// Implementations should consider inlining if necessary.
+ /// The return value is the new object reference for `object` if it is moved, or `object` if
+ /// not moved. If moved, the caller should update the slot that holds the reference to
+ /// `object` so that it points to the new location.
+ ///
+ /// Note: This function is performance-critical, therefore must be implemented efficiently.
  fn trace_object(&mut self, object: ObjectReference) -> ObjectReference;
 }