Skip to content

Fix write barrier parameter type #1130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 30, 2024
Merged

Fix write barrier parameter type #1130

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc008e6
Fix write barrier parameter type.
wks Apr 28, 2024
5135cdd
Fix mock tests
wks Apr 29, 2024
e5e9d95
Change comment
wks Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/memory_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,23 @@ pub fn post_alloc<VM: VMBinding>(
/// * `src`: The modified source object.
/// * `slot`: The location of the field to be modified.
/// * `target`: The target for the write operation.
///
/// # Deprecated
///
/// This form of write barrier has multiple issues.
///
/// - It is only able to write non-null object references into the slot. But dynamic language
/// VMs may write non-reference values, such as tagged small integers, special values such as
/// `null`, `undefined`, `true`, `false`, etc. into a field that previous contains an object
/// reference.
/// - It relies on `slot.store` to write `target` into the slot, but `slot.store` is designed for
/// forwarding references when an object is moved by GC, and is supposed to preserve tagged
/// type information, the offset (if it is an interior pointer), etc. A write barrier is
/// associated to an assignment operation, which usually updates such information instead.
///
/// VM bindings should use `object_reference_write_pre` and `object_reference_write_post` instead.
/// Those functions leaves the actual write of the slot to the VM binding.
#[deprecated = "Use `object_reference_write_pre` and `object_reference_write_post` instead"]
Copy link
Member

@qinsoon qinsoon Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall we have any discussion about removing the subsuming barrier. If I missed any previous discussion, just let me know.

If I understand you right, there are a few issues with this method right now:

  1. target: ObjectReference cannot represent all the cases. If MMTk does the write for the binding, target needs to be able to represent any value the runtime may hold. Option<ObjectReference> is not sufficient either. We may want something like target: Value.
  2. slot: VM::VMEdge cannot handle writing values that are not references. Though Edge (we want to rename it to Slot) is introduced for updating slots for copying GC, I think it is a valid case of reusing this trait to update slots in write barriers. We could extend the trait to allow it to store any value.

Is there any fundamental reason that we should remove the subsuming barrier instead of trying to correct it?

The reason why write_pre/post work is that we currently do not use the slot argument, and we do not care the target value if it is not a reference (so Option<ObjectReference> is sufficient).

Copy link
Collaborator Author

@wks wks Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't remove the subsuming barrier, but we need to redesign it. That's why I said "This form of write barrier has multiple issues", not subsuming barriers in general. We will replace this object_reference_write with another function, maybe with the same name, but different parameters.

There are ways to allow the VM customize the write operation of the subsuming barrier. One way is letting the caller givt it a callback that actually writes to the field so that it can write the field in whatever way it likes. The subsuming barrier effectively executes the pre and post barrier between the callback.

Copy link
Member

@qinsoon qinsoon Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. The attribute deprecated mostly means the function will be removed in the future, thus the use of this function is not encouraged now. Clearly this is different from this case. I think either we make a practical change to this function so it still works, or we mark it as deprecated but make it very clear that this function or the subsuming barrier will not be removed but will be redesigned, we suggest not using it before the redesign.

Copy link
Collaborator Author

@wks wks Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the comment and emphasized that we would redesign that function and replace the current one.

pub fn object_reference_write<VM: VMBinding>(
mutator: &mut Mutator<VM>,
src: ObjectReference,
Expand All @@ -252,12 +269,14 @@ pub fn object_reference_write<VM: VMBinding>(
/// * `mutator`: The mutator for the current thread.
/// * `src`: The modified source object.
/// * `slot`: The location of the field to be modified.
/// * `target`: The target for the write operation.
/// * `target`: The target for the write operation. `None` if the slot did not hold an object
/// reference before the write operation. For example, the slot may be holding a `null`
/// reference, a small integer, or special values such as `true`, `false`, `undefined`, etc.
pub fn object_reference_write_pre<VM: VMBinding>(
mutator: &mut Mutator<VM>,
src: ObjectReference,
slot: VM::VMEdge,
target: ObjectReference,
target: Option<ObjectReference>,
) {
mutator
.barrier()
Expand All @@ -278,12 +297,14 @@ pub fn object_reference_write_pre<VM: VMBinding>(
/// * `mutator`: The mutator for the current thread.
/// * `src`: The modified source object.
/// * `slot`: The location of the field to be modified.
/// * `target`: The target for the write operation.
/// * `target`: The target for the write operation. `None` if the slot no longer hold an object
/// reference after the write operation. This may happen when writing a `null` reference, a small
/// integers, or a special value such as`true`, `false`, `undefined`, etc., into the slot.
pub fn object_reference_write_post<VM: VMBinding>(
mutator: &mut Mutator<VM>,
src: ObjectReference,
slot: VM::VMEdge,
target: ObjectReference,
target: Option<ObjectReference>,
) {
mutator
.barrier()
Expand Down
16 changes: 8 additions & 8 deletions src/plan/barriers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,17 @@ pub trait Barrier<VM: VMBinding>: 'static + Send + Downcast {
slot: VM::VMEdge,
target: ObjectReference,
) {
self.object_reference_write_pre(src, slot, target);
self.object_reference_write_pre(src, slot, Some(target));
slot.store(target);
self.object_reference_write_post(src, slot, target);
self.object_reference_write_post(src, slot, Some(target));
}

/// Full pre-barrier for object reference write
fn object_reference_write_pre(
&mut self,
_src: ObjectReference,
_slot: VM::VMEdge,
_target: ObjectReference,
_target: Option<ObjectReference>,
) {
}

Expand All @@ -71,7 +71,7 @@ pub trait Barrier<VM: VMBinding>: 'static + Send + Downcast {
&mut self,
_src: ObjectReference,
_slot: VM::VMEdge,
_target: ObjectReference,
_target: Option<ObjectReference>,
) {
}

Expand All @@ -81,7 +81,7 @@ pub trait Barrier<VM: VMBinding>: 'static + Send + Downcast {
&mut self,
_src: ObjectReference,
_slot: VM::VMEdge,
_target: ObjectReference,
_target: Option<ObjectReference>,
) {
}

Expand Down Expand Up @@ -147,7 +147,7 @@ pub trait BarrierSemantics: 'static + Send {
&mut self,
src: ObjectReference,
slot: <Self::VM as VMBinding>::VMEdge,
target: ObjectReference,
target: Option<ObjectReference>,
);

/// Slow-path call for mempry slice copy operations. For example, array-copy operations.
Expand Down Expand Up @@ -217,7 +217,7 @@ impl<S: BarrierSemantics> Barrier<S::VM> for ObjectBarrier<S> {
&mut self,
src: ObjectReference,
slot: <S::VM as VMBinding>::VMEdge,
target: ObjectReference,
target: Option<ObjectReference>,
) {
if self.object_is_unlogged(src) {
self.object_reference_write_slow(src, slot, target);
Expand All @@ -228,7 +228,7 @@ impl<S: BarrierSemantics> Barrier<S::VM> for ObjectBarrier<S> {
&mut self,
src: ObjectReference,
slot: <S::VM as VMBinding>::VMEdge,
target: ObjectReference,
target: Option<ObjectReference>,
) {
if self.log_object(src) {
self.semantics
Expand Down
2 changes: 1 addition & 1 deletion src/plan/generational/barrier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<VM: VMBinding, P: GenerationalPlanExt<VM> + PlanTraceObject<VM>> BarrierSem
&mut self,
src: ObjectReference,
_slot: VM::VMEdge,
_target: ObjectReference,
_target: Option<ObjectReference>,
) {
// enqueue the object
self.modbuf.push(src);
Expand Down
11 changes: 6 additions & 5 deletions src/vm/tests/mock_tests/mock_test_barrier_slow_path_assertion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ fn test_assertion_barrier_invalid_ref() {
fixture.mutator_mut().barrier.object_reference_write_slow(
invalid_objref,
edge,
objref,
Some(objref),
);
});
},
Expand All @@ -51,10 +51,11 @@ fn test_assertion_barrier_valid_ref() {
let edge = Address::from_ref(&slot);

// Invoke barrier slowpath with the valid object ref
fixture
.mutator_mut()
.barrier
.object_reference_write_slow(objref, edge, objref);
fixture.mutator_mut().barrier.object_reference_write_slow(
objref,
edge,
Some(objref),
);
});
},
no_cleanup,
Expand Down