Implement Edge::update_for_forwarding #1037
Conversation
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.
Update the doc comments of `Edge::load` and `Edge::store` to mention tagged references.
| return; | ||
| } | ||
| let new_object = updater(object); | ||
| if new_object.is_null() { |
There was a problem hiding this comment.
Can you do new_object.is_null() && object == new_object? That would fix the case where we overwrite references for objects that were not moved.
There was a problem hiding this comment.
I am expecting the updater (usually implemented by trace_object) to do that test inside (if necessary at all) so that we don't need to check object == new_object here. One example is the nursery (for GenCopy and GenImmix). It always moves the object, so it doesn't need to check. It actually does debug_assert!(!self.plan.is_object_in_nursery(new_object)); which implies new_object cannot be equal to object.
There was a problem hiding this comment.
Hm? I don't follow. trace_object can't store the new object (if any) into the slot. It will only return an object reference
There was a problem hiding this comment.
That's right. trace_object doesn't do the storing. However, the contract of the updater closure is, if it returns ObjectReference::NULL, then Edge::update_for_forwarding will not do the storing.
process_edge implements the updater closure. After executing let new_object = trace_object(object), it should check if new_object == object and, if they are equal, return ObjectReference::NULL from the updater closure. Currently ProcessEdgesWork::process_edge is not doing this because I don't want to change the semantics of the existing ProcessEdgesWork::process_edge. We may give it a try by adding if new_object == object { return ObjectReference::NULL; } else { return new_object; } there. IIRC, @qinsoon once tried doing this check after trace_object, and it is not always profitable. We may give it another try, but at this moment, I want to make sure the closure itself doesn't introduce extra cost.
There was a problem hiding this comment.
I believe Yi's work made it an Option<ObjectReference>. That has more overhead since it doesn't condense into a usize (given ObjectReference::NULL exists). I don't think we should retain the current semantics since we know the current semantics are broken. There is no need to store the same object again into the slot
There was a problem hiding this comment.
Related issues:
- Only update reference if the object is actually moved? #574
- Remove TransitiveClosure param from Space::trace_object and Scanning::scan_object #559 (comment)
My previous experiment showed that the performance impact is negligible. So we have reasons to replace the constant OVERWRITE_REFERENCE and the invocation P::may_move_objects::<KIND>() with an actual new_object == object check. But we'll probably do it in a separate PR to address #574 specifically.
There was a problem hiding this comment.
Sure. Alternatively, we can make a PR for fixing that right now (since it should be simple to fix) and then merge that first, given this PR is a "design" PR and I'm not sure if we've decided what's the best way to review them yet
There was a problem hiding this comment.
Sure. Alternatively, we can make a PR for fixing that right now (since it should be simple to fix) and then merge that first
Yes. That's a good idea.
given this PR is a "design" PR and I'm not sure if we've decided what's the best way to review them yet
I think this can be an "MEP", too, but this may not be as controversal as removing ObjectReference::NULL. Since I already made this PR, I think I will test the performance with the OpenJDK binding and, if it performas well, I'll write up an "MEP" for this, too, and welcome everyone to discuss. Actually I still have some design questions about this. One is whether we need a way to update slots atomically. The LXR branch sometimes does "compare exchange" on slots.
This PR implements the single-method updating operation described in #1033