Edge trait #606

wks · 2022-06-02T07:04:26Z

We make the representation of edges VM-specific. The MMTk core only
depends on the abstract Edge trait. This allows VM bindings to custom
the behaviour of loading/storing of edges, and support features like
compressed OOPs.

Some example Edge implementations are in vmbindings/dummyvm/src/edges.rs. There is a test case vmbindings/dummyvm/src/tests/edges_test.rs to show what to expect.

This PR involves API change. Migration guide is in the commit message.

Binding PRs:

OpenJDK: Edge trait mmtk-openjdk#160
JikesRVM: Edge trait mmtk-jikesrvm#121
V8: Use the new API with Edge trait mmtk-v8#66
Ruby: Use the new API with Edge trait mmtk-ruby#9

Closes: #573

We make the representation of edges VM-specific. The MMTk core only depends on the abstract Edge trait. This allows VM bindings to custom the behaviour of loading/storing of edges, and support features like compressed OOPs. Binding migration advices: - The type of edges is now defined by the VM, and is no longer limited to Address. To transition from the old API, the VM can start by defining its vm-specific edge type as an alias of Address, like: type OpenJDKEdge = Address; and gradually transition to mmtk::vm::edge_shape::SimpleEdge, or define a more appropriate type for the VM. - The VMBinding trait now requires a VMEdge member type. Implement it with the VM-specific edge type, like: type VMEdge = OpenJDKEdge; - EdgeVisitor and RootsWorkFactory now become EdgeVisitor<T> and RootsWorkFactory<T>. If an old API function passes an EdgeVisitor or RootsWorkFactory object, add the VM's edge type as T. For example, change fn scan_object<EV: EdgeVisitor>(...) { ... } to fn scan_object<EV: EdgeVisitor<OpenJDKEdge>>(...) { ... }

qinsoon

LGTM

qinsoon · 2022-08-25T23:01:32Z

src/vm/edge_shape.rs

+/// For backword compatibility.
+/// We let Address implement Edge so existing bindings that use `Address` to represent an edge can
+/// continue to work.
+impl Edge for Address {


Maybe it is worth documenting that we suggest using SimpleEdge instead of Address, and why.

Yes. I'll add more comments about why we should move away from using Address directly as an edge.

qinsoon · 2022-08-25T23:11:44Z

vmbindings/dummyvm/src/edges.rs

+
+/// This edge presents the object reference itself to mmtk-core.
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
+pub struct ValueEdge {


Will we use a type like this when a binding wants to do node enqueuing? If so, we probably should just call it Node or at least in the comment, state this is also known as node.

I thought this ValueEdge could help node-enqueuing, but I later found it doesn't help. I already removed all comments that mentioned "edge enqueuing" in this PR, but I want to keep this example, just to demonstrate that the developers can think out of the box when implementing Edge. I'll add some comments to clarify the purpose of this example.

qinsoon · 2022-08-25T23:19:50Z

I suggest we also get a review from @wenyuzhao on this PR.

qinsoon · 2022-08-26T02:31:17Z

vmbindings/dummyvm/src/edges.rs

+    }
+
+    fn store(&self, _object: ObjectReference) {
+        // No-op.  Value edges are immutable.


I don't think we can call this an edge. We call store() to update the edge, but this implementation cannot update. The store() impl should be at least unreachable!() or panic!(), as the impl cannot do what it is asked. However, that will cause MMTk crash, as we will call store. I think we should just remove this type -- it is confusing.

OK. I think I'll just remove the ValueEdge type because it is confusing.

src/vm/edge_shape.rs

wenyuzhao · 2022-08-26T03:50:57Z

src/vm/mod.rs

@@ -45,6 +46,9 @@ where
    type VMActivePlan: ActivePlan<Self>;
    type VMReferenceGlue: ReferenceGlue<Self>;

+    /// The type of edges in this VM.
+    type VMEdge: edge_shape::Edge;


Is it possible to do a runtime switch between different VMEdges? For example, OpenJDK may need to support compressed pointers as an optional run-time feature, and cannot be activated at build-time.

Also probably this can be moved to VMObjectModel?

If the "feature" is a Cargo feature, we can define it like this:

enum OpenJDKEdge { Uncompressed(*mut AtomicUsize), #[cfg(features = "compressed_oop")] Compressed(*mut AtomicU32), }

But if we want it enabled/disabled by a command-line flag, the easiest way (may affect performance, but need to be confirmed with tests) is simply not use the OpenJDKEdge::Compressed case. As an optimisation, maybe we can parameterise OpenJDK to OpenJDK<EdgeType> so that depending on the command line options, we can instantiate MMTk with different (parameterised) VMBinding implementation.

Maybe we could check the performance impact of having enum OpenJDKEdge (but never using the other case). We do hope that is minimal.

Also probably this can be moved to VMObjectModel?

I don't think ObjectModel is the right place to put it. I think it is fine to put the type in VMBinding (as it is now). If we plan to move it, I think Scanning fits well. All related changes are in the Scanning trait. This is an indication that the type is related with Scanning.

Also probably this can be moved to VMObjectModel?

In theory, we can. But then instead of VM::VMEdge, we will need to write <<VM as VMBinding>::VMObjectModel as ObjectModel<VM>>::VMEdge everywhere.

A refactoring may be possible to let VMBinding require VMObjectModel, instead of including a VMObjectModel as a member. I have seen similar problems when I tried to refactor ReferenceProcessor. I tried to add a function in VMCollection that depends on a type in VMReferenceGlue, and I have to navigate from VMCollection to VMBinding, and from there to VMReferenceGlue. Sometimes the compiler is not smart enough to figure out the VM type passed to the function to VMCollection is the same VM as the VM in VMReferenceGlue<VM>.

I'll elaborate the refactoring in a different issue.

Using enum OpenJDKEdge will (1) introduce an extra branch for each edge load and store (2) For a mark-queue Vec<Edge>, there will be one word (100%) overhead for each entry. Probably I'll parameterize OpenJDK.

In theory, we can. But then instead of VM::VMEdge, we will need to write <::VMObjectModel as ObjectModel>::VMEdge everywhere.

I see. Yes I agree it should stay in VMBinding.

I think I don't really need another edge type for compressed pointers. The size of an edge address is always one word, no matter whether it's a *mut AtomicU32 or *mut AtomicUsize . The only thing I need to replace is the handler of the load/store operations.

If space overhead is a problem, the OpenJDK binding can implement Edge with a tagged pointer. It saves space, but still needs a test on bits.

Other bindings may not be able to use this trick. ~~Ruby already uses tagged pointers~~ (Sorry. It doesn't matter whether the VM uses tagged pointers. It's about the binding differentiating kinds of edges using tag bits.), and Julia has offsetted edges where the offset has to be recorded.

More comments on why bindings should use `SimpleEdge` instead of `Address`. Add `#[repr(transparent)]` to `SimpleEdge`.

wks · 2022-08-26T07:40:15Z

I updated the PR.

I replaced the ValueEdge example with TaggedEdge, because we do have some VMs using tagged pointers.
I added comments about why we should prefer SimpleEdge over Address.
I added #[repr(transparent)] to SimpleEdge.

wks mentioned this pull request Jun 2, 2022

Edge trait mmtk/mmtk-openjdk#160

Merged

wks force-pushed the edge-shape branch 3 times, most recently from 5003b66 to af2575f Compare August 5, 2022 09:56

wks force-pushed the edge-shape branch from af2575f to f85efdd Compare August 16, 2022 12:13

wks force-pushed the edge-shape branch 2 times, most recently from cd0618f to c540854 Compare August 25, 2022 07:53

wks force-pushed the edge-shape branch from c540854 to 24b7e47 Compare August 25, 2022 09:11

This was referenced Aug 25, 2022

Edge trait mmtk/mmtk-jikesrvm#121

Merged

Use the new API with Edge trait mmtk/mmtk-v8#66

Merged

wks marked this pull request as ready for review August 25, 2022 15:42

wks mentioned this pull request Aug 25, 2022

Use the new API with Edge trait mmtk/mmtk-ruby#9

Merged

wks requested a review from qinsoon August 25, 2022 15:50

qinsoon approved these changes Aug 25, 2022

View reviewed changes

qinsoon requested a review from wenyuzhao August 25, 2022 23:19

qinsoon reviewed Aug 26, 2022

View reviewed changes

wenyuzhao reviewed Aug 26, 2022

View reviewed changes

wks added 2 commits August 26, 2022 14:08

Comments and representation.

55ef856

More comments on why bindings should use `SimpleEdge` instead of `Address`. Add `#[repr(transparent)]` to `SimpleEdge`.

Replace ValueEdge example with TaggedEdge

58ac993

Fix test for offset change.

9af3a93

wks mentioned this pull request Aug 28, 2022

Remove the unpleasant <VM as VMBinding>::Foo::bar code #652

Draft

wks merged commit 76131c4 into mmtk:master Aug 29, 2022

wks mentioned this pull request Oct 24, 2022

Should we rename the Edge trait? #687

Closed

wks mentioned this pull request Jul 19, 2024

Require ObjectReference to be inside an object #1170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edge trait #606

Edge trait #606

wks commented Jun 2, 2022 •

edited

Loading

qinsoon left a comment

qinsoon Aug 25, 2022

wks Aug 26, 2022

qinsoon Aug 25, 2022

wks Aug 26, 2022

qinsoon commented Aug 25, 2022

qinsoon Aug 26, 2022

wks Aug 26, 2022

wenyuzhao Aug 26, 2022 •

edited

Loading

wenyuzhao Aug 26, 2022

wks Aug 26, 2022

qinsoon Aug 26, 2022 •

edited

Loading

qinsoon Aug 26, 2022

wks Aug 26, 2022

wenyuzhao Aug 26, 2022 •

edited

Loading

wenyuzhao Aug 26, 2022

wenyuzhao Aug 26, 2022

wks Aug 26, 2022 •

edited

Loading

wks commented Aug 26, 2022

Edge trait #606

Edge trait #606

Conversation

wks commented Jun 2, 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

qinsoon commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyuzhao Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinsoon Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyuzhao Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wks Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

wks commented Aug 26, 2022

wks commented Jun 2, 2022 •

edited

Loading

wenyuzhao Aug 26, 2022 •

edited

Loading

qinsoon Aug 26, 2022 •

edited

Loading

wenyuzhao Aug 26, 2022 •

edited

Loading

wks Aug 26, 2022 •

edited

Loading