From dbf6e94e1055b463b6888f9c67fa23da7ab712f2 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Thu, 30 Nov 2023 14:05:50 +0800 Subject: [PATCH 1/9] Skip slots where `Edge::load()` returns NULL This allows the VM binding to report slots that hold tagged non-reference values as `ObjectReference::NULL` so that mmtk-core will not try to trace it. --- .../src/tutorial/code/mygc_semispace/gc_work.rs | 4 +--- .../src/tutorial/mygc/ss/exercise_solution.md | 4 +--- src/plan/generational/gc_work.rs | 8 +++++--- src/policy/marksweepspace/malloc_ms/global.rs | 4 +--- src/policy/marksweepspace/native_ms/global.rs | 4 +--- src/scheduler/gc_work.rs | 14 ++++++++------ 6 files changed, 17 insertions(+), 21 deletions(-) diff --git a/docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs b/docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs index 29be4f6184..f80ff56f9a 100644 --- a/docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs +++ b/docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs @@ -56,9 +56,7 @@ impl ProcessEdgesWork for MyGCProcessEdges { } 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) { diff --git a/docs/userguide/src/tutorial/mygc/ss/exercise_solution.md b/docs/userguide/src/tutorial/mygc/ss/exercise_solution.md index 4f4717ed77..cb751aa082 100644 --- a/docs/userguide/src/tutorial/mygc/ss/exercise_solution.md +++ b/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) { diff --git a/src/plan/generational/gc_work.rs b/src/plan/generational/gc_work.rs index a5c38e84ea..28080ff72d 100644 --- a/src/plan/generational/gc_work.rs +++ b/src/plan/generational/gc_work.rs @@ -36,9 +36,8 @@ impl + PlanTraceObject> 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 @@ -46,6 +45,9 @@ impl + PlanTraceObject> ProcessEdg } fn process_edge(&mut self, slot: EdgeOf) { 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); diff --git a/src/policy/marksweepspace/malloc_ms/global.rs b/src/policy/marksweepspace/malloc_ms/global.rs index 7454fbb287..8d42b74f0d 100644 --- a/src/policy/marksweepspace/malloc_ms/global.rs +++ b/src/policy/marksweepspace/malloc_ms/global.rs @@ -400,9 +400,7 @@ impl MallocSpace { queue: &mut Q, object: ObjectReference, ) -> ObjectReference { - if object.is_null() { - return object; - } + debug_assert!(!object.is_null()); assert!( self.in_space(object), diff --git a/src/policy/marksweepspace/native_ms/global.rs b/src/policy/marksweepspace/native_ms/global.rs index 8d8eae7d0e..a533fcde33 100644 --- a/src/policy/marksweepspace/native_ms/global.rs +++ b/src/policy/marksweepspace/native_ms/global.rs @@ -241,9 +241,7 @@ impl MarkSweepSpace { 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.", diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs index 7d0add88a4..f41de8c547 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -663,6 +663,9 @@ pub trait ProcessEdgesWork: /// trace the object and store back the new object reference if necessary. fn process_edge(&mut self, slot: EdgeOf) { let object = slot.load(); + if object.is_null() { + return; + } let new_object = self.trace_object(object); if Self::OVERWRITE_REFERENCE { slot.store(new_object); @@ -721,9 +724,7 @@ impl ProcessEdgesWork for SFTProcessEdges { 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 type parameter let worker = GCWorkerMutRef::new(self.worker()); @@ -997,9 +998,7 @@ impl + Plan, 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,6 +1007,9 @@ impl + Plan, const KIND: TraceKin fn process_edge(&mut self, slot: EdgeOf) { let object = slot.load(); + if object.is_null() { + return; + } let new_object = self.trace_object(object); if P::may_move_objects::() { slot.store(new_object); From 7b086731f19dee611564791afa5568b7ad417e29 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Thu, 30 Nov 2023 15:21:38 +0800 Subject: [PATCH 2/9] More non-null assertions in spaces. --- src/policy/copyspace.rs | 1 + src/policy/immix/immixspace.rs | 1 + src/policy/immortalspace.rs | 1 + src/policy/largeobjectspace.rs | 1 + src/policy/markcompactspace.rs | 1 + 5 files changed, 5 insertions(+) diff --git a/src/policy/copyspace.rs b/src/policy/copyspace.rs index 8d08ec1507..8193c7c63c 100644 --- a/src/policy/copyspace.rs +++ b/src/policy/copyspace.rs @@ -217,6 +217,7 @@ impl CopySpace { worker: &mut GCWorker, ) -> 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() { diff --git a/src/policy/immix/immixspace.rs b/src/policy/immix/immixspace.rs index 3809f7bd24..bf72394c01 100644 --- a/src/policy/immix/immixspace.rs +++ b/src/policy/immix/immixspace.rs @@ -184,6 +184,7 @@ impl crate::policy::gc_work::PolicyTraceObject for ImmixSpace copy: Option, worker: &mut GCWorker, ) -> 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 { diff --git a/src/policy/immortalspace.rs b/src/policy/immortalspace.rs index 5eeebd58c9..1e4d19eefa 100644 --- a/src/policy/immortalspace.rs +++ b/src/policy/immortalspace.rs @@ -187,6 +187,7 @@ impl ImmortalSpace { 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::(object), diff --git a/src/policy/largeobjectspace.rs b/src/policy/largeobjectspace.rs index ec6b2f7506..64a13c8f37 100644 --- a/src/policy/largeobjectspace.rs +++ b/src/policy/largeobjectspace.rs @@ -189,6 +189,7 @@ impl LargeObjectSpace { 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::(object), diff --git a/src/policy/markcompactspace.rs b/src/policy/markcompactspace.rs index 693218b492..e6d332919e 100644 --- a/src/policy/markcompactspace.rs +++ b/src/policy/markcompactspace.rs @@ -130,6 +130,7 @@ impl crate::policy::gc_work::PolicyTraceObject for MarkCompac _copy: Option, _worker: &mut GCWorker, ) -> ObjectReference { + debug_assert!(!object.is_null()); debug_assert!( KIND != TRACE_KIND_TRANSITIVE_PIN, "MarkCompact does not support transitive pin trace." From 01865db6a299cc35d18b061b38e5f5ae3a6c001c Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Thu, 30 Nov 2023 15:50:33 +0800 Subject: [PATCH 3/9] Update documentation Update the doc comments of `Edge::load` and `Edge::store` to mention tagged references. --- src/vm/edge_shape.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/vm/edge_shape.rs b/src/vm/edge_shape.rs index dd23efd393..269e02f7a1 100644 --- a/src/vm/edge_shape.rs +++ b/src/vm/edge_shape.rs @@ -41,9 +41,35 @@ use crate::util::{Address, ObjectReference}; /// 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. + /// + /// If the slot is not holding an object reference, it should return `ObjectReference::NULL`. + /// Specifically, + /// + /// - If the langauge has the concept of "null pointer" which does not point to any object in + /// the heap, this method should return `ObjectReference::NULL` regardless how a null + /// pointer is encoded in the VM. However, if the VM uses a special object in the heap to + /// represent a null value, such as the `None` object of `NoneType` in Python, this method + /// should still return the object reference to such `None` objects so that they are + /// properly traced, kept alive, and they have their references forwarded. + /// - If, in a VM, the data type a slot can hold is a union of references and non-reference + /// values, and the slot is currently holding a non-reference value, such as a small + /// integer, a floating-point number, or any special value such as `true`, `false` or `nil` + /// that do not point to any object, the slot is considered not holding an reference. This + /// method should return `ObjectReference::NULL` in such cases. + /// + /// 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. + /// + /// 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 fn store(&self, object: ObjectReference); /// Prefetch the edge so that a subsequent `load` will be faster. From 5d0c98b12f15fb19bd586186099e9fd51de4ba58 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Fri, 8 Dec 2023 12:08:13 +0800 Subject: [PATCH 4/9] Check return value of trace_object --- src/plan/generational/gc_work.rs | 6 +++++- src/scheduler/gc_work.rs | 6 ++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/plan/generational/gc_work.rs b/src/plan/generational/gc_work.rs index 28080ff72d..3a01b3bcd4 100644 --- a/src/plan/generational/gc_work.rs +++ b/src/plan/generational/gc_work.rs @@ -50,7 +50,11 @@ impl + PlanTraceObject> ProcessEdg } 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( diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs index f41de8c547..5bcbbfe40c 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -667,7 +667,8 @@ pub trait ProcessEdgesWork: 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); } } @@ -1011,7 +1012,8 @@ impl + Plan, const KIND: TraceKin return; } let new_object = self.trace_object(object); - if P::may_move_objects::() { + debug_assert!(!new_object.is_null()); + if P::may_move_objects::() && new_object != object { slot.store(new_object); } } From 0cf0c1d6a17ad33188983a701a8e206150aa9de5 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Mon, 18 Dec 2023 14:09:59 +0800 Subject: [PATCH 5/9] Formatting --- src/scheduler/gc_work.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs index 5bcbbfe40c..aa4c7c171c 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -1013,7 +1013,7 @@ impl + Plan, const KIND: TraceKin } let new_object = self.trace_object(object); debug_assert!(!new_object.is_null()); - if P::may_move_objects::() && new_object != object { + if P::may_move_objects::() && new_object != object { slot.store(new_object); } } From fcc6d858a4566bc6bf6e29b62123f4cfefc78c05 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Mon, 18 Dec 2023 14:42:06 +0800 Subject: [PATCH 6/9] Update doc comment --- src/vm/edge_shape.rs | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/vm/edge_shape.rs b/src/vm/edge_shape.rs index 269e02f7a1..dc5a399f3b 100644 --- a/src/vm/edge_shape.rs +++ b/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,28 +45,17 @@ 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, it should return `ObjectReference::NULL`. - /// Specifically, - /// - /// - If the langauge has the concept of "null pointer" which does not point to any object in - /// the heap, this method should return `ObjectReference::NULL` regardless how a null - /// pointer is encoded in the VM. However, if the VM uses a special object in the heap to - /// represent a null value, such as the `None` object of `NoneType` in Python, this method - /// should still return the object reference to such `None` objects so that they are - /// properly traced, kept alive, and they have their references forwarded. - /// - If, in a VM, the data type a slot can hold is a union of references and non-reference - /// values, and the slot is currently holding a non-reference value, such as a small - /// integer, a floating-point number, or any special value such as `true`, `false` or `nil` - /// that do not point to any object, the slot is considered not holding an reference. This - /// method should return `ObjectReference::NULL` in such cases. + /// 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 @@ -70,6 +64,12 @@ pub trait Edge: Copy + Send + Debug + PartialEq + Eq + Hash { /// 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. From bc340d8424ca90136a9fa946c7f8076bc3674e67 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Mon, 18 Dec 2023 15:42:13 +0800 Subject: [PATCH 7/9] Fix hyperlink format --- src/vm/edge_shape.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vm/edge_shape.rs b/src/vm/edge_shape.rs index dc5a399f3b..45e7141c25 100644 --- a/src/vm/edge_shape.rs +++ b/src/vm/edge_shape.rs @@ -63,13 +63,13 @@ pub trait Edge: Copy + Send + Debug + PartialEq + Eq + Hash { /// /// 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 + /// See: /// /// 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 + /// See: fn store(&self, object: ObjectReference); /// Prefetch the edge so that a subsequent `load` will be faster. From 5aa3392e9992f0e8b026b51dd1f3e022a0537d76 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Tue, 19 Dec 2023 16:18:26 +0800 Subject: [PATCH 8/9] Check if referent is cleared before forwarding --- src/util/reference_processor.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/util/reference_processor.rs b/src/util/reference_processor.rs index 9683048881..718235ff67 100644 --- a/src/util/reference_processor.rs +++ b/src/util/reference_processor.rs @@ -216,6 +216,7 @@ impl ReferenceProcessor { e: &mut E, referent: ObjectReference, ) -> ObjectReference { + debug_assert!(!referent.is_null()); e.trace_object(referent) } @@ -223,6 +224,7 @@ impl ReferenceProcessor { e: &mut E, object: ObjectReference, ) -> ObjectReference { + debug_assert!(!object.is_null()); e.trace_object(object) } @@ -230,6 +232,7 @@ impl ReferenceProcessor { e: &mut E, referent: ObjectReference, ) -> ObjectReference { + debug_assert!(!referent.is_null()); e.trace_object(referent) } @@ -285,9 +288,6 @@ impl ReferenceProcessor { reference: ObjectReference, ) -> ObjectReference { let old_referent = ::VMReferenceGlue::get_referent(reference); - let new_referent = ReferenceProcessor::get_forwarded_referent(trace, old_referent); - ::VMReferenceGlue::set_referent(reference, new_referent); - let new_reference = ReferenceProcessor::get_forwarded_reference(trace, reference); { use crate::vm::ObjectModel; trace!( @@ -295,13 +295,22 @@ impl ReferenceProcessor { reference, ::VMObjectModel::get_current_size(reference) ); + } + + if !::VMReferenceGlue::is_referent_cleared(old_referent) { + let new_referent = ReferenceProcessor::get_forwarded_referent(trace, old_referent); + ::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", From afb4beb25c358b57ef76249faddce63774dd6f8b Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Tue, 19 Dec 2023 16:34:29 +0800 Subject: [PATCH 9/9] Update doc comment and add assertion --- src/scheduler/gc_work.rs | 5 +---- src/vm/scanning.rs | 11 +++++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs index 3633f58f27..692873eb84 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -273,11 +273,8 @@ pub(crate) struct ProcessEdgesWorkTracer { impl ObjectTracer for ProcessEdgesWorkTracer { /// 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 diff --git a/src/vm/scanning.rs b/src/vm/scanning.rs index 44ae056190..21768f4337 100644 --- a/src/vm/scanning.rs +++ b/src/vm/scanning.rs @@ -26,11 +26,14 @@ impl EdgeVisitor 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; }