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

Optimize object forwarding for single threaded GC #1283

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

k-sareen
Copy link
Collaborator

No description provided.

@k-sareen k-sareen requested a review from wks March 17, 2025 01:12
@wks
Copy link
Collaborator

wks commented Mar 17, 2025

1.74.1 failed to compile because icu4x started to require rust 1.81. Let's fix that separately.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

Two high-level comments:

Firstly, we can skip the BEING_FORWARDED state without affecting the callers in CopySpace and ImmixSpace. After calling attempt_to_forward, the caller always calls state_is_forwarded_or_being_forwarded to test if the return value is FORWARDING_NOT_TRIGGERED_YET or other states (BEING_FORWARDED or FORWARDED), but in fact it can never observe BEING_FORWARDED even we set the state to BEING_FORWARDED in attempt_to_forward. That's because if it is FORWARDING_NOT_TRIGGERED_YET, the (only) GC worker will immediately try to forward the object, and it will either successfully copy the object and change the state to FORWARDED, or decide not to copy the object and revert the state to FORWARDING_NOT_TRIGGERED_YET. Then a subsequent invocation of attempt_to_forward will either observe FORWARDED or FORWARDING_NOT_TRIGGERED_YET. So we can simply omit the store in attempt_to_forward.

Secondly, I think the "early exit" style is more readable than the current if-else style.

.compare_exchange_metadata::<VM, u8>(
if old_value == FORWARDING_NOT_TRIGGERED_YET {
unsafe {
VM::VMObjectModel::LOCAL_FORWARDING_BITS_SPEC.store::<VM, u8>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This store can be removed. The caller of attempt_to_forward only needs the old_value. As long as it is FORWARDING_NOT_TRIGGERED_YET, the caller will immediately try to forward the object.

debug_assert!(
forwarding_bits == FORWARDING_NOT_TRIGGERED_YET,
forwarding_bits != BEING_FORWARDED,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can just assert forwarding_bits == FORWARDED if we don't use BEING_FORWARDED.

@@ -21,22 +21,36 @@ const FORWARDING_POINTER_MASK: usize = 0xffff_fffc;
/// Attempt to become the worker thread who will forward the object.
/// The successful worker will set the object forwarding bits to BEING_FORWARDED, preventing other workers from forwarding the same object.
pub fn attempt_to_forward<VM: VMBinding>(object: ObjectReference) -> u8 {
loop {
if cfg!(not(feature = "single_worker")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having this if-else statement with not(feature = "single_worker") as the condition makes the code a bit harder to read. Since it is simpler to do it with a single worker, we can rewrite it in the early exit style. For example,

pub fn attempt_to_forward<VM: VMBinding>(object: ObjectReference) -> u8 {
    if cfg!(feature = "single_worker") {
        return get_forwarding_status::<VM>(object); // Assume we remove BEING_FORWARDED.
    }

    // old code here
}

@k-sareen
Copy link
Collaborator Author

Secondly, I think the "early exit" style is more readable than the current if-else style.

The problem is then I think the code for the multi-threaded object forwarding will still be in the generated binary leading to code bloat. I don't know if the Rust/LLVM compiler will remove that code if the if statement is always true. Might need to actually look at the assembly code to confirm this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants