Skip to content

Support for object pinning #703

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

Merged
merged 17 commits into from
Dec 6, 2022
Merged

Conversation

udesou
Copy link
Contributor

@udesou udesou commented Nov 21, 2022

This is a draft PR to support object pinning.
I have added three methods to the SFT: pin_object, unpin_object, and is_object_pinned.
In policies where pinning should not be supported I've created a panic message. For policies where objects do not move, all methods simply return false and do nothing. I'm not 100% if objects can be pinned in mark compact, so I left these methods as unimplemented() for now.
The PR also adds a PinningBitSpec to the object model, and check to the trace_object_with_opportunistic_copy function before attempting to forward objects or clear the forwarding status.

@udesou udesou requested review from qinsoon and wenyuzhao November 21, 2022 03:55
Copy link
Member

@wenyuzhao wenyuzhao left a comment

Choose a reason for hiding this comment

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

LGTM. Just minor questions

… true for is_object_pinned for non-movable spaces
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

I think we would need to discuss about the new API methods. Currently there are pin, unpin and is_pinned. The problem is that for a non-moving space, is_pinned will return true (which means the object is pinned) even if every call to pin returns false (which means we fail to pin the object). If necessary, we can define semantics like this. But generally, it is a bit confusing. I suggest we have some discussion first.

@qinsoon
Copy link
Member

qinsoon commented Dec 1, 2022

@udesou Can you fix the tests? Then we can get this merged, and #706 will not include changes about pinning.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

LGTM

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Dec 5, 2022
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.

I just discovered a small problem. Variable names with an underscore in front of it, such as _object, indicates that they are unused. The compiler will not give the "unused variables" warning for those variables. We should remove those underscores since we started using them.

@udesou
Copy link
Contributor Author

udesou commented Dec 6, 2022

I just discovered a small problem. Variable names with an underscore in front of it, such as _object, indicates that they are unused. The compiler will not give the "unused variables" warning for those variables. We should remove those underscores since we started using them.

Where exactly do you mean they should be renamed? Is it in code like:

    #[inline(always)]
    fn is_pinned(&self, _object: ObjectReference) -> bool {
        #[cfg(feature = "object_pinning")]
        return self.is_object_pinned(_object);

        #[cfg(not(feature = "object_pinning"))]
        false
    }

If so, I was just using the same idea as

    fn initialize_object_metadata(&self, _object: ObjectReference, _alloc: bool) {
        #[cfg(feature = "global_alloc_bit")]
        crate::util::alloc_bit::set_alloc_bit::<VM>(_object);
    }

@qinsoon
Copy link
Member

qinsoon commented Dec 6, 2022

I just discovered a small problem. Variable names with an underscore in front of it, such as _object, indicates that they are unused. The compiler will not give the "unused variables" warning for those variables. We should remove those underscores since we started using them.

Where exactly do you mean they should be renamed? Is it in code like:

    #[inline(always)]
    fn is_pinned(&self, _object: ObjectReference) -> bool {
        #[cfg(feature = "object_pinning")]
        return self.is_object_pinned(_object);

        #[cfg(not(feature = "object_pinning"))]
        false
    }

If so, I was just using the same idea as

    fn initialize_object_metadata(&self, _object: ObjectReference, _alloc: bool) {
        #[cfg(feature = "global_alloc_bit")]
        crate::util::alloc_bit::set_alloc_bit::<VM>(_object);
    }

The issue is how we tell Rust about arguments that are only used under some features and are unused in other cases. What we currently do for most of the cases are: leaving the argument with the underscore _ prefix, and use it if you need to.

You could argue that the best approach is to separate the implementations, such as

     #[cfg(feature = "object_pinning")]
     fn is_pinned(&self, object: ObjectReference) -> bool {
         self.is_object_pinned(object)
     }
     #[cfg(not(feature = "object_pinning"))]
     fn is_pinned(&self, object: ObjectReference) -> bool {
         false
     }

However, this may not be desirable in some cases when there are code shared.

Anyway, I think this is a minor styling issue. What has been done in this PR is consistent with our current code base, and I don't feel it is wrong -- it is just one approach to deal with it.

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.

OK. I didn't realise that the Rust compiler is not considering it as "used" when only used in some configuration.

LGTM now.

@qinsoon qinsoon merged commit 6fe9bbc into mmtk:master Dec 6, 2022
@wks wks mentioned this pull request Jan 31, 2023
14 tasks
wenyuzhao pushed a commit to wenyuzhao/mmtk-core that referenced this pull request Mar 20, 2023
This is a draft PR to support object pinning. 
I have added three methods to the SFT: `pin_object`, `unpin_object`, and `is_object_pinned`. 
In policies where pinning should not be supported I've created a panic message. For policies where objects do not move, all methods simply return false and do nothing. 
The PR also adds a `PinningBitSpec` to the object model, and check to the `trace_object_with_opportunistic_copy` function before attempting to forward objects or clear the forwarding status.
@qinsoon qinsoon mentioned this pull request May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants