Skip to content
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.

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

Only update reference if the object is actually moved? #574

Closed
qinsoon opened this issue Apr 27, 2022 · 3 comments · Fixed by #1032
Closed

Only update reference if the object is actually moved? #574

qinsoon opened this issue Apr 27, 2022 · 3 comments · Fixed by #1032
Labels
A-work-queue Area: Work queue C-enhancement Category: Enhancement P-normal Priority: Normal.

Comments

@qinsoon
Copy link
Member

qinsoon commented Apr 27, 2022

In the line in ProcessEdgesWork,

let new_object = self.trace_object(object);
if Self::OVERWRITE_REFERENCE {
unsafe { slot.store(new_object) };
}

we check a constant OVERWRITE_REFERENCE and write back the reference returned by trace_object(). OVERWRITE_REFERENCE
is defined per ProcessEdgesWork, which is currently per plan. In a copying plan, OVERWRITE_REFERENCE is true, and we always update references.

However, this means there are a few cases where we update references but we do not have to:

  • an object is in a non-copying space: e.g. objects in immortal or large object space in a copying plan
  • an object is in a copying space but in a particular trace, the policy is not moving objects: e.g. objects in an immix space in a non-defrag GC (non-moving), or objects in the mature immix space in generational immix for a non-defrag GC (nursery is still copying, but objects in mature space is non-moving for that GC).

We may want to check if this introduces any measurable overhead.

There could be different solutions if this brings any overhead:

  • Per policy per trace work packet. So OVERWRITE_REFERENCE is defined per policy per trace.
  • Check if new_object is the same as object before write. (conditional vs memory write).
@qinsoon qinsoon added F-question Call For Participation: Unanswered question (need more information) C-enhancement Category: Enhancement A-work-queue Area: Work queue labels Apr 27, 2022
@qinsoon
Copy link
Member Author

qinsoon commented Apr 27, 2022

Alternative solutions:

  • Let policy's trace_object() returns whether they copied object or not (e.g. returns Option<ObjectReference>).
  • Pass the slot address to policy's trace_object(). If the policy copies objects, they are responsible to update the slot.

@wks
Copy link
Collaborator

wks commented Apr 27, 2022

One way to "let policy's trace_object() returns whether they copied object or not" is mentioned here: #559

Example:

pub struct TraceObjectResult {
    pub is_newly_marked: bool,  // true if the object is newly marked
    pub forwarded: Option<ObjectReference>,  // If an object is forwarded, return Some(newobj), otherwise None.
}

#[inline]
fn process_edge(&mut self, slot: Address) {
    let object = unsafe { slot.load::<ObjectReference>() };
    let TraceObjectResult(newly_marked, forwarded) = self.trace_object(object);
    if newly_marked {
        self.process_node(forwarded.unwrap_or(object));  // enqueue IFF newly marked
    }
    if let Some(new_object) = forwarded {
        unsafe { slot.store(new_object) };  // store IFF forwarded.
    }
}

But it is probably easier to try Option<ObjectReference> alone first to see if there is any impact on performance.

@qinsoon qinsoon removed the F-question Call For Participation: Unanswered question (need more information) label Apr 27, 2022
@wks
Copy link
Collaborator

wks commented May 18, 2022

As discussed in another issue, the impact is negligible if we return Option<ObjectReference> and update references exactly when the object is actually moved.

#559 (comment)

@udesou udesou added the P-normal Priority: Normal. label Nov 15, 2023
github-merge-queue bot pushed a commit that referenced this issue Dec 20, 2023
This PR does two things:

1. `ProcessEdgesWork::process_edge` will skip slots that are not holding
references (i.e. if `Edge::load()` returns `ObjectReference::NULL`).
2. `ProcessEdgesWork::process_edge` will skip the `Edge::store()` if the
object is not moved.

Doing (1) removes unnecessary invocations of `trace_object()` as well as
the subsequent `Edge::store()`. It also allows slots to hold
non-reference tagged values. In that case, the VM binding can return
`ObjectReference::NULL` in `Edge::load()` so that mmtk-core will simply
skip the slot, fixing #1031

Doing (2) removes unnecessary `Edge::store()` operations in the case
where the objects are not moved during `trace_object`. It reduces the
STW time in most cases, fixing
#574

Fixes: #1031
Fixes: #574
@wks wks closed this as completed in #1032 Dec 20, 2023
k-sareen pushed a commit to k-sareen/mmtk-core that referenced this issue Apr 11, 2024
This PR does two things:

1. `ProcessEdgesWork::process_edge` will skip slots that are not holding
references (i.e. if `Edge::load()` returns `ObjectReference::NULL`).
2. `ProcessEdgesWork::process_edge` will skip the `Edge::store()` if the
object is not moved.

Doing (1) removes unnecessary invocations of `trace_object()` as well as
the subsequent `Edge::store()`. It also allows slots to hold
non-reference tagged values. In that case, the VM binding can return
`ObjectReference::NULL` in `Edge::load()` so that mmtk-core will simply
skip the slot, fixing mmtk#1031

Doing (2) removes unnecessary `Edge::store()` operations in the case
where the objects are not moved during `trace_object`. It reduces the
STW time in most cases, fixing
mmtk#574

Fixes: mmtk#1031
Fixes: mmtk#574
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-work-queue Area: Work queue C-enhancement Category: Enhancement P-normal Priority: Normal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants