Refactor finalize/reference processor #150

fepicture · 2023-09-26T01:25:26Z

edit v1: sync latest status, i have finish the reference processor and integrate together.
edit v2: update with binding PR link.

fix #145
mmtk-jikesrvm PR #150
jikesRVM PR mmtk/jikesrvm#16

Modifications to the mmtk-jikesrvm repository encompass three key aspects:

I have removed some old bind APIs, marking them as unimplement, or directly removing them.
I implemented the process_weak_ref and forward_weak_ref methods. These will be invoked in the work bucket and act as an entry point for the original process in jikesrvm.
Lastly, the CI heap configuration has been updated.

All tests for the current semispace have passed. I've rebased to the draft pull request. Changes: - I implemented process_weak_refs which is invoked by a workbucket to process weak references. Internally, it uses jtoc_call to reuse existing JikesRVM methods.

changes: - Refactor to use state machine style scanning - Remove unnecessary debugging code

fepicture · 2023-10-05T00:59:33Z

update v1: i will test in my repo action first

I just found that the ci is using GitHub ci runner, I am going to test it in my repo action.

@qinsoon thanks for your commit to update jikesrvm version.
the test suite heap usage has been tested on my local (64GiB RAM, i9-12900H), but I do not know why will fail :p

- Fix style-check - Upgrade heap size in weak-ref and normal test - Disable flaky test

wks

The code should function properly, although there may be some hidden problems that prevent some tests from passing. But JikesRVM is still one of our officially-supported VMs. We run binding tests for some PRs (will soon become all PRs) of mmtk-core, and JikesRVM is one of the VMs to run binding tests with. So I think it is not appropriate to merge this unstable change and disable some tests now.

I suggest we merge your change, but keep your change disabled by default. That is, the MMTK-JikesRVM will still use the reference processors in mmtk-core. But with a compile-time flag, MMTk-JikesRVM will use JikesRVM's reference/finalizer processors, instead.

You can use conditional compilation tricks to enable/disable the code.

For Rust code, you can use Cargo features. You can add a feature, such as binding_side_ref_proc in Cargo.toml. Inside Rust source files, you can use the attribute #[cfg(feature = "binding_side_ref_proc")] to selectively enable types, functions, blocks, statements, etc. You can also use if cfg!(feature = "binding_side_ref_proc") to guard the control flow.

For Java code, you can just write if SOME_CONSTANT { ... }, and JikesRVM's compiler is smart enough to eliminate dead code. I'll discuss more in the review comments in your PR in the jikesrvm repo.

For test scripts such as ci-test-assertions.sh, instead of commenting out failing tests and changing heap sizes, I suggest copy those scripts, save to different files (such as ci-test-assertions-weakref.sh), and edit the content there, including disabling failing tests and changing heap sizes. (p.s. You should change both -Xms and -Xmx so that on each line the -Xms and -Xmx are set to the same value.)

And you should create a compile script to compile jikesrvm plus mmtk-jikesrvm using your added conditional compilation flags.

.github/scripts/ci-test-assertions.sh

.github/scripts/ci-test-normal.sh

jikesrvm/rvm/src/org/jikesrvm/runtime/Entrypoints.java

mmtk/api/mmtk.h

mmtk/src/api.rs

mmtk/src/reference_glue.rs

mmtk/src/scanning.rs

* Add a new configuration setting. The test script has been updated to modify this configuration using `sed`. * Employ Rust's `cfg` attribute to conditionally compile specific modules/functions.

fepicture · 2023-10-23T00:05:30Z

hi @qinsoon， I have pushed a new commit to solve the comment

(basically, use conditional compile to keep the compatibility),

can u activate the GitHub action workflow for this new commit?
https://github.com/mmtk/mmtk-jikesrvm/actions/runs/6600465759

https://github.com/mmtk/mmtk-jikesrvm/actions/runs/6600465756

qinsoon · 2023-10-23T00:09:15Z

hi @qinsoon， I have pushed a new commit to solve the comment

(basically, use conditional compile to keep the compatibility),

can u activate the GitHub action workflow for this new commit? https://github.com/mmtk/mmtk-jikesrvm/actions/runs/6600465759

https://github.com/mmtk/mmtk-jikesrvm/actions/runs/6600465756

Yeah. I just did. I thought the workflows would be allowed as they were approved for the previous commit. I will keep an eye on the PR.

fepicture · 2023-10-23T00:10:07Z

Thank you 🙇

wks

Although not all tests passed, the current state of the code used conditional compilation and other techniques to keep the CI tests passing. In this sense, it looks good to me.

We should keep working on this feature to make it stable so that we can eventually use VM-side weak ref and finalization processing by default.

wks · 2023-10-24T07:46:43Z

A recent upstream change (mmtk/mmtk-core@57af17f) moved the method is_emergency_collection from Plan to GlobalState, but GlobalState is a private structure and inaccessible from the VM binding. That prevents this PR from working.

The built-in reference processors and finalization processors can access the MMTK::state field because they are parts of the mmtk crate. We should make change to mmtk-core and expose the is_emergency_collection function to the binding.

fepicture · 2023-10-24T07:49:29Z

A recent upstream change (mmtk/mmtk-core@57af17f) moved the method is_emergency_collection from Plan to GlobalState, but GlobalState is a private structure and inaccessible from the VM binding. That prevents this PR from working.

The built-in reference processors and finalization processors can access the MMTK::state field because they are parts of the mmtk crate. We should make change to mmtk-core and expose the is_emergency_collection function to the binding.

thanks I will fix later.

`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](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.

fepicture · 2023-10-25T01:11:16Z

hi, @qinsoon

I have pushed a new commit to update with mmtk-core PR(expose the API for check emergency collection )

Plz activate the GitHub action workflow.

wks · 2023-10-25T01:59:58Z

The "Dacapo Tests / test" failed with a known bug that existed before this PR was introduced. I'll rerun the test.

`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.

fepicture added 2 commits September 26, 2023 01:22

simple update with process_weak_refs

9ff74d6

fepicture changed the title ~~[wip] Refactor finalize processor~~ [wip] Refactor finalize/reference processor Sep 26, 2023

fepicture force-pushed the refactor_finalize_processor branch from 637771e to 92bac4f Compare October 1, 2023 08:52

Upgrade maximum heap size, then passed the ms test

fe6e302

changes: - Refactor to use state machine style scanning - Remove unnecessary debugging code

fepicture force-pushed the refactor_finalize_processor branch from 92bac4f to fe6e302 Compare October 1, 2023 08:53

update with ci script heap usage

45816c2

fepicture changed the title ~~[wip] Refactor finalize/reference processor~~ Refactor finalize/reference processor Oct 4, 2023

fepicture marked this pull request as ready for review October 4, 2023 05:46

qinsoon and others added 3 commits October 4, 2023 20:40

Update JikesRVM version

3f0385d

Try fix CI: upgrade maximum size of heap

5bdc658

Merge branch 'master' into refactor_finalize_processor

012dc17

Fix ci:

db36f01

- Fix style-check - Upgrade heap size in weak-ref and normal test - Disable flaky test

fepicture mentioned this pull request Oct 5, 2023

Refactor finalize/reference processor mmtk/jikesrvm#16

Merged

wks requested changes Oct 11, 2023

View reviewed changes

Update with conditional compilation:

ba0eb9f

* Add a new configuration setting. The test script has been updated to modify this configuration using `sed`. * Employ Rust's `cfg` attribute to conditionally compile specific modules/functions.

fepicture requested a review from wks October 23, 2023 01:17

wks approved these changes Oct 24, 2023

View reviewed changes

Merge branch 'master' into refactor_finalize_processor

f06cd4f

wks mentioned this pull request Oct 24, 2023

Expose is_emergency_collection to VM bindings mmtk/mmtk-core#997

Merged

update with mmtk/mmtk-core#997 PR

51633c7

Update jikesrvm repo and revision

c8b3df2

wks merged commit d9daca2 into mmtk:master Oct 25, 2023
3 checks passed

wks mentioned this pull request Oct 25, 2023

Use JikesRVM-specific weak reference processing and finalisation mechanism #137

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor finalize/reference processor #150

Refactor finalize/reference processor #150

fepicture commented Sep 26, 2023 •

edited

Loading

fepicture commented Oct 5, 2023 •

edited

Loading

wks left a comment

fepicture commented Oct 23, 2023

qinsoon commented Oct 23, 2023

fepicture commented Oct 23, 2023

wks left a comment

wks commented Oct 24, 2023 •

edited

Loading

fepicture commented Oct 24, 2023

fepicture commented Oct 25, 2023

wks commented Oct 25, 2023

Refactor finalize/reference processor #150

Refactor finalize/reference processor #150

Conversation

fepicture commented Sep 26, 2023 • edited Loading

fepicture commented Oct 5, 2023 • edited Loading

wks left a comment

Choose a reason for hiding this comment

fepicture commented Oct 23, 2023

qinsoon commented Oct 23, 2023

fepicture commented Oct 23, 2023

wks left a comment

Choose a reason for hiding this comment

wks commented Oct 24, 2023 • edited Loading

fepicture commented Oct 24, 2023

fepicture commented Oct 25, 2023

wks commented Oct 25, 2023

fepicture commented Sep 26, 2023 •

edited

Loading

fepicture commented Oct 5, 2023 •

edited

Loading

wks commented Oct 24, 2023 •

edited

Loading