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

Expose is_emergency_collection to VM bindings #997

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

wks
Copy link
Collaborator

@wks wks commented Oct 24, 2023

is_emergency_collection is useful during weak reference processing. JVMs can choose not to retain referents of SoftReference during emergency GC.

The is_emergency_collection function was moved from Plan to GlobalState in 57af17f. After that, the MMTK:state field is private and inaccessible to VM bindings. VM bindings that depend on the to-be-deprecated built-in ReferenceProcessor still work because it is part of the mmtk crate, and can mmtk.state.is_emergency_collection. However, VM bindings that do weak reference processing on the VM side using the Scanning::process_weak_refs API can no longer call that function. This makes this PR unable to merge. In the future, the OpenJDK binding will also need it when it off-loads weak reference processing to the binding side.

This PR adds a public API function MMTK::is_emergency_collection which can be called by the VM bindings.

@wks wks requested a review from qinsoon October 24, 2023 10:04
@k-sareen
Copy link
Collaborator

What about exposing GlobalState itself? Being global state, shouldn't the binding also be able to access it?

@wks
Copy link
Collaborator Author

wks commented Oct 24, 2023

What about exposing GlobalState itself? Being global state, shouldn't the binding also be able to access it?

Yes. That makes sense, too. Although not all states in the GlobalState struct is public, we can select what is public by annotating fields and methods with pub.

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
Copy link
Member

qinsoon commented Oct 24, 2023

What about exposing GlobalState itself? Being global state, shouldn't the binding also be able to access it?

Yes. That makes sense, too. Although not all states in the GlobalState struct is public, we can select what is public by annotating fields and methods with pub.

I agree. Being global doesn't mean they are public.

This PR looks good. Providing getter/setter is generally better than exposing public fields.

@wks wks enabled auto-merge October 24, 2023 11:37
@wks wks added this pull request to the merge queue Oct 24, 2023
Merged via the queue into mmtk:master with commit b4f4519 Oct 24, 2023
18 of 19 checks passed
@wks wks deleted the fix/expose-emergency branch October 24, 2023 12:46
fepicture added a commit to fepicture/mmtk-jikesrvm that referenced this pull request Oct 24, 2023
k-sareen pushed a commit to k-sareen/mmtk-core that referenced this pull request Apr 11, 2024
`is_emergency_collection` is useful during weak reference processing.
JVMs can choose not to retain referents of `SoftReference` during
emergency GC.
 
The `is_emergency_collection` function was moved from `Plan` to
`GlobalState` in
mmtk@57af17f.
After that, the `MMTK:state` field is private and inaccessible to VM
bindings. VM bindings that depend on the to-be-deprecated built-in
`ReferenceProcessor` still work because it is part of the `mmtk` crate,
and can `mmtk.state.is_emergency_collection`. However, VM bindings that
do weak reference processing on the VM side using the
`Scanning::process_weak_refs` API can no longer call that function. This
makes [this PR](mmtk/mmtk-jikesrvm#150) unable
to merge. In the future, the OpenJDK binding will also need it when it
off-loads weak reference processing to the binding side.

This PR adds a public API function `MMTK::is_emergency_collection` which
can be called by the VM bindings.
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.

3 participants