Adding support for red/black roots #706

udesou · 2022-12-01T05:38:25Z

This is a draft PR to properly add support for processing red object roots (the root object or any object in its transitive closure must not move), and black object roots (where only the root object must not move, and any object beyond it may move).

Currently, the code is very messy, and I am pretty sure it is not generic enough in multiple places, so I'd appreciate your feedback on how to do that properly.

A short list of high-level changes that I've made:

introduced a ClosureImmovable work bucket that runs before Closure; this is necessary to make sure objects found during this closure are processed within the bucket, with the guarantee that no object processed there should move.
introduced TRACE_KIND_IMMOVABLE. This means that any object traced with this kind will not move, and objects that are "found" by the trace should be put in the ClosureImmovable work bucket.
split the work in schedule_collection into two: in defrag we first process red roots with TRACE_KIND_IMMOVABLE then process the remaining roots with TRACE_KIND_DEFRAG. Note that I create an API function in VMScanning called scan_vm_immovable_roots, in which the binding can scan red roots, when in_defrag is false, these roots are processed with TRACE_KIND_FAST where nothing moves.
created a ImmovableObjectsClosure that creates work into WorkBucketStage::ClosureImmovable instead of WorkBucketStage::Closure.

The rest is essentially duplicating code to enforce the separation between the two closures.

I guess it would be interesting to discuss and agree on the implementation first, then we should discuss naming conventions (red/black, movable/immovable, etc), and definitely involve others once the code is more mature.

… true for is_object_pinned for non-movable spaces

…mtk-core into feature/pinning-objects

qinsoon

These are my general suggestions:

focus on removing duplicate code
use a field in work packets to store the bucket that new work should be pushed to
have a switch (runtime option or feature) to enable/disable immovable closure
let a binding to create work for immovable vs movable roots by telling RootsWorkFactory which kind of roots they are.

qinsoon · 2022-12-01T06:48:16Z

src/plan/immix/global.rs

+                VM,
+                ImmixGCWorkContext<VM, TRACE_KIND_IMMOVABLE>,
+            >(scheduler, self);
+            schedule_remaining_work::<VM, ImmixGCWorkContext<VM, TRACE_KIND_DEFRAG>>(


You could have a runtime option or a feature. When it is turned on, you will schedule work for the immovable trace. In that case, you can just check the option/feature in schedule_common_work and schedule the work if enabled.

qinsoon · 2022-12-01T06:53:51Z

src/plan/immix/global.rs

+                }
+            }
+            // Scan immovable roots with immovable trace
+            mmtk.scheduler.work_buckets[WorkBucketStage::Prepare].add(ScanVMImmovableRoots::<


Do you still need this? I notice that in normal StopMutators, you also scheduled ScanVMImmovableRoots. One of the two is redundant I guess.

Alternatively, you could change RootsWorkFactory::create_process_edge_roots_work() to let it take an extra boolean to say whether the roots are movable or not. In that case, the binding does not need to scan stacks twice, one for immovable roots and one for normal roots. They can just do one stack scanning, and create different work packet for different kinds of roots (I assume this is more natural and efficient for the bindings?).
This change will also help remove quite a few duplicate code in this PR, such as ScanMutators, ScanVMImmovableRoots, and ProcessEdgesWorkImmovableRootsWorkFactory.

qinsoon · 2022-12-01T06:58:36Z

src/plan/tracing.rs

+        let buf = self.buffer.take();
+        if !buf.is_empty() {
+            self.worker.add_work(
+                WorkBucketStage::ClosureImmovable,


I guess the only difference of this type and a normal ObjectsClosure is which bucket new work will be added. You can just add a field closure_bucket to ObjectsClosure, and schedule work to the closure_bucket.

qinsoon · 2022-12-01T07:03:20Z

src/scheduler/gc_work.rs

+            // Executing these work packets now can remarkably reduce the global synchronization time.
+            self.worker().do_work(work_packet);
+        } else {
+            if KIND == TRACE_KIND_IMMOVABLE {


Similar to ObjectsClosure, you can just have a field to store which bucket new work should go to.

qinsoon · 2022-12-01T07:15:04Z

src/scheduler/gc_work.rs

+            // We create an instance of E to use its `trace_object` method and its object queue.
+            // let mut process_edges_work = Self::E::new(vec![], false, mmtk);
+            let mut process_edges_work =
+                <PlanProcessEdges<E::VM, P, TRACE_KIND_IMMOVABLE>>::new(vec![], false, mmtk);


Is this fix to the current do_work_common()?

mmtk-core/src/scheduler/gc_work.rs

Line 632 in 117000f

let mut process_edges_work = Self::E::new(vec![], false, mmtk);

qinsoon · 2022-12-01T07:16:33Z

src/scheduler/gc_work.rs

+        let mut scan_later = vec![];
+        {
+            if KIND == TRACE_KIND_IMMOVABLE {
+                let mut closure = ImmovableObjectsClosure::<Self::E>::new(worker);


Similarly, if you have a field to tell you which bucket to use, then you can just pass that bucket variable between work packets, and use it when you push new work.

qinsoon · 2022-12-01T07:17:15Z

src/scheduler/gc_work.rs

+                    if KIND == TRACE_KIND_IMMOVABLE {
+                        memory_manager::add_work_packet(
+                            mmtk,
+                            WorkBucketStage::ClosureImmovable,


Same here. You just need a variable to tell you which bucket to use.

…as forwarding bit

This reverts commit 38d2f6e.

This reverts commit 66e2709.

qinsoon · 2022-12-07T02:50:26Z

src/scheduler/gc_work.rs

-            .trace_object::<VectorObjectQueue, KIND>(&mut self.base.nodes, object, worker)
+        if self.is_immovable_trace() {
+            self.plan
+                .trace_object::<VectorObjectQueue, TRACE_KIND_FAST>(


This still uses Immix specific TraceKind. Currently RootsWorkFactory carries ProcessEdgesWork, which defines a trace kind. But this code 'overwrites' the trace kind for immix for immovable trace. This sounds hacky.

Ideally the Immix policy should define TRACE_KIND_IMMOVABLE, and pass that to work packets in schedule_collection. When RootsWorkFactory generates work packets, it should pass that trace kind to immovable work.

Maybe we can make RootsWorkFactory to take two type parameters, one for ProcessEdgesWork for normal trace, and the other for immovable trace. When RootsWorkFactory generates work, it chooses which to use, depending on it is movable or not.

Better check with @wks on his suggestion as well.

Yes. The key to the solution is the RootsWorkFactory. Note that it doesn't specify what kind of work packet it creates, and it does not force using only one impl of ProcessEdgesWork, either.

Like Yi suggested, we can create another impl of RootsWorkFactory, such as RedBlackRootsWorkFactory<E1, E2>. When create_process_node_roots_work is called, if is_immovable == true (should really be is_transitively_immovable if I understand correctly, because it should transitively pin objects. Ruby's conservative stack roots are immovable, too, but it doesn't transitively pin objects.), create E2, otherwise create E1.

qinsoon · 2022-12-07T02:54:49Z

src/scheduler/gc_work.rs

@@ -435,7 +450,12 @@ pub trait ProcessEdgesWork:
            // Executing these work packets now can remarkably reduce the global synchronization time.
            self.worker().do_work(work_packet);
        } else {
-            self.mmtk.scheduler.work_buckets[WorkBucketStage::Closure].add(work_packet);
+            if self.is_immovable_trace() {


This pattern keeps repeating. The problems are: 1. code duplication, 2. it leaks the abstraction, as for those work packets, they do not really need to know about immovable or not -- they just need to know what bucket they should use.

if is_immvable { ... WorkBucketStage::ClosureImmovable ... } else { ... WorkBucketStage::Closure ... }

I would suggest just storing a field bucket for those packet types.

Yes. I have had the same feeling for our current ProcessEdgesWork and ScanObjects work packets for a long time. They should add work buckets to their current stages (could be Closure, {Soft,Weak,Final,Phantom}Closure, a stage after SecondRoot, or my newly added VMRefClosure and VMRefForwarding stage).

Adding the "red/black roots" feature just made the issue more obvious, because ClosureImmovable and Closure is so different that we can no longer throw a work packet into Closure and expect them to be executed at any stage.

wks · 2022-12-01T06:18:46Z

src/memory_manager.rs

@@ -750,6 +750,47 @@ pub fn add_finalizer<VM: VMBinding>(
    mmtk.finalizable_processor.lock().unwrap().add(object);
 }

+/// Pin an object. MMTk will make sure that the object does not move
+/// during GC. Note that action cannot happen in some plans, eg, semispace.


Yes. I recommend adding a field to PlanConstraints to indicate whether a plan supports pinning. For example: supports_pinning: bool or allows_pinning: bool. Any plan that has moves_object = false should also have supports_pinning = true. By doing this, we can add debug_assert! or assert! here and panic early.

wks · 2022-12-07T03:44:58Z

src/scheduler/gc_work.rs

@@ -435,7 +450,12 @@ pub trait ProcessEdgesWork:
            // Executing these work packets now can remarkably reduce the global synchronization time.
            self.worker().do_work(work_packet);
        } else {
-            self.mmtk.scheduler.work_buckets[WorkBucketStage::Closure].add(work_packet);
+            if self.is_immovable_trace() {


Yes. I have had the same feeling for our current ProcessEdgesWork and ScanObjects work packets for a long time. They should add work buckets to their current stages (could be Closure, {Soft,Weak,Final,Phantom}Closure, a stage after SecondRoot, or my newly added VMRefClosure and VMRefForwarding stage).

Adding the "red/black roots" feature just made the issue more obvious, because ClosureImmovable and Closure is so different that we can no longer throw a work packet into Closure and expect them to be executed at any stage.

wks · 2022-12-07T04:07:08Z

src/scheduler/gc_work.rs

-            .trace_object::<VectorObjectQueue, KIND>(&mut self.base.nodes, object, worker)
+        if self.is_immovable_trace() {
+            self.plan
+                .trace_object::<VectorObjectQueue, TRACE_KIND_FAST>(


Yes. The key to the solution is the RootsWorkFactory. Note that it doesn't specify what kind of work packet it creates, and it does not force using only one impl of ProcessEdgesWork, either.

Like Yi suggested, we can create another impl of RootsWorkFactory, such as RedBlackRootsWorkFactory<E1, E2>. When create_process_node_roots_work is called, if is_immovable == true (should really be is_transitively_immovable if I understand correctly, because it should transitively pin objects. Ruby's conservative stack roots are immovable, too, but it doesn't transitively pin objects.), create E2, otherwise create E1.

wks · 2022-12-07T04:10:40Z

src/vm/scanning.rs

@@ -68,7 +68,7 @@ pub trait RootsWorkFactory<ES: Edge>: Clone + Send + 'static {
    ///
    /// Arguments:
    /// * `nodes`: A vector of references to objects pointed by root edges.
-    fn create_process_node_roots_work(&mut self, nodes: Vec<ObjectReference>);
+    fn create_process_node_roots_work(&mut self, nodes: Vec<ObjectReference>, is_immovable: bool);


Alternatively, we can add one extra function:

#[cfg(feature = "object_pinning")] fn create_process_red_roots_work(&mut self, nodes: Vec<ObjectReference>);

After some thinking, I think "red roots" really are a different kind of roots. It should be handled differently compared to regular roots. This also lets us make it optional so only those VMs that care about object pinning can call that.

"Red roots" may needs some explanation. We can choose another method name, too, if that makes it easier to understand.

An alternative is adding another function to Scanning, such as Scanning::scan_vm_specific_red_roots. But that may imply scanning stacks twice. So I think it is better to extend the RootsWorkFactory trait instead.

…ving

k-sareen · 2023-10-26T06:04:55Z

Superseded by #897

udesou added 8 commits November 21, 2022 14:43

Support for object pinning

d00e4c6

Adding pinning metadata to immix space side metadata specs; Returning…

00a33f2

… true for is_object_pinned for non-movable spaces

Proper semantics for pin/unpin; Fixing checks in immixspace.

a062970

Applying discussed semantics for pinning/unpinning

dd0a28b

Merge branch 'mmtk:master' into feature/pinning-objects

b56af7d

Updating to latest master

b574a75

Adding support for tracing 'red roots'

66e2709

Minor fix

38d2f6e

udesou requested review from wks and qinsoon December 1, 2022 05:38

qinsoon mentioned this pull request Dec 1, 2022

Support for object pinning #703

Merged

udesou added 4 commits December 1, 2022 16:49

Fixing object model for DummyVM

b86b321

Merge branch 'master' into feature/pinning-objects

8e99228

Fixing clippy issues

c013513

Merge branch 'feature/pinning-objects' of https://github.com/udesou/m…

2ded32d

…mtk-core into feature/pinning-objects

qinsoon reviewed Dec 1, 2022

View reviewed changes

udesou and others added 11 commits December 2, 2022 00:13

Converting object pinning into a feature

235f83a

Renaming object-pinning => object_pinning

13accdb

Merge branch 'master' into feature/pinning-objects

b73d892

Removing assertion that was useful if pin bin happens to be the same …

57f5dcb

…as forwarding bit

Revert "Minor fix"

0dedd14

This reverts commit 38d2f6e.

Revert "Adding support for tracing 'red roots'"

a915c22

This reverts commit 66e2709.

Merge branch 'feature/pinning-objects' into feature/object-moving

72bc39d

Refactoring support for red roots

3fe039d

Merge branch 'master' into feature/object-moving

c1feb58

Adding pinning methods to native ms space

86ffdcf

Merge branch 'master' into feature/object-moving

23090bb

qinsoon reviewed Dec 7, 2022

View reviewed changes

wks reviewed Dec 7, 2022

View reviewed changes

wks mentioned this pull request Jan 31, 2023

Support copying GC mmtk/mmtk-ruby#18

Closed

14 tasks

wks mentioned this pull request Jun 15, 2023

Fix VO bits for Immix #849

Merged

5 tasks

qinsoon and others added 2 commits June 26, 2023 05:49

Merge branch 'master' into feature/object-moving

45209c0

Merge remote-tracking branch 'upstream/master' into feature/object-mo…

dc0582b

…ving

k-sareen closed this Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for red/black roots #706

Adding support for red/black roots #706

udesou commented Dec 1, 2022 •

edited

Loading

qinsoon left a comment

qinsoon Dec 1, 2022

qinsoon Dec 1, 2022

qinsoon Dec 1, 2022

qinsoon Dec 1, 2022

qinsoon Dec 1, 2022

qinsoon Dec 1, 2022

qinsoon Dec 1, 2022

qinsoon Dec 1, 2022

qinsoon Dec 7, 2022

wks Dec 7, 2022

qinsoon Dec 7, 2022

wks Dec 7, 2022

wks Dec 1, 2022

wks Dec 7, 2022

wks Dec 7, 2022

wks Dec 7, 2022

k-sareen commented Oct 26, 2023

Adding support for red/black roots #706

Adding support for red/black roots #706

Conversation

udesou commented Dec 1, 2022 • edited Loading

qinsoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-sareen commented Oct 26, 2023

udesou commented Dec 1, 2022 •

edited

Loading