This PR enables transitively pinning (TP) objects from particular roots for Immix/StickyImmix #897

udesou · 2023-08-10T05:15:08Z

A new bucket ImmovableClosure has been created to trace and transitively pin roots, i.e., roots in which no object in its transitive closure is allowed to move.
RootsWorkFactory has two new functions create_process_tp_edge_roots_work and create_process_tp_node_roots_work to create work to process these roots.
GCWorkContext expects an TPProcessEdges type, which performs the transitively pinning trace (and is unimplemented for unsupported plans).
create_process_node_roots_work creates work to process roots in the NodeRootsTrace bucket, which is executed after TPClosure, but any work derived from it is put into the regular Closure bucket, meaning that the binding shouldn't need to pin root nodes.
For sticky immix, currently we only support non-moving nursery collections (sticky_immix_non_moving_nursery), but the solution here is to use the TPClosure on the modified buffer constructed from the write barrier (since during nursery collection we do not know if these objects should be transitively pinned or not).

wks

There is a correctness issue regarding the processing of black (non-transitively pinning) roots. See comments.

src/scheduler/gc_work.rs

src/scheduler/work_bucket.rs

qinsoon · 2023-08-13T03:18:22Z

I think we should stop using red/black roots in the PR, and in future discussion just to avoid any confusion. As we discussed, we will call it TransitivelyPin, TPin, or TP.

…andle node roots that should be kept it place but their children may move

… nursery GC, allowing moving on full heap but disabling it for the nursery GC

wks · 2023-08-24T09:53:26Z

The GCWorkContext trait is just a collection of associated types. The concrete types that implement it should never be instantiated, and is usually implemented with structs that do not have any fields (except PhantomData). It doesn't make sense for its instances to implement Send and 'static. However, some work packets are parameterised with C: GCWorkContext, and Rust uses the type parameters to determine whether the work packet type automatically implement Send.

To workaround this, we can let the GCWorkContext trait require Send + 'static. Because none of its concrete implementations have actual fields, they should automatically implement Send + 'static, too. Then we can remove + Send + 'static from impl<C: GCWorkContext + Send + 'static> GCWork<C::VM> for SomeWorkPacketType<C>

~~You can cherry-pick this commit: wks@6f79aea~~

Cherry-pick this commit, instead: wks@1617f54 It fixed more cases and added comments.

qinsoon · 2023-09-06T05:40:33Z

binding-refs
OPENJDK_BINDING_REPO=udesou/mmtk-openjdk
OPENJDK_BINDING_REF=fix/upstream-api-change-pr897
JIKESRVM_BINDING_REPO=udesou/mmtk-jikesrvm
JIKESRVM_BINDING_REF=fix/upstream-api-change-pr897
V8_BINDING_REPO=udesou/mmtk-v8
V8_BINDING_REF=fix/upstream-api-change-pr897
JULIA_BINDING_REPO=udesou/mmtk-julia
JULIA_BINDING_REF=fix/upstream-api-change-pr897
RUBY_BINDING_REPO=wks/mmtk-ruby
RUBY_BINDING_REF=feature/rb-roots

qinsoon · 2023-09-07T00:54:19Z

The Julia tests failed, as the Julia PR did not include the change about its tests so it timed out. I don't know what happened for the Ruby tests.

udesou · 2023-09-07T01:28:28Z

The Julia tests failed, as the Julia PR did not include the change about its tests so it timed out. I don't know what happened for the Ruby tests.

@wks I'm also not familiar with that error. Do you have any idea why the ruby tests failed to run?

wks · 2023-09-07T03:40:39Z

@wks I'm also not familiar with that error. Do you have any idea why the ruby tests failed to run?

When running binding tests for mmtk-core, I used a Python script to replace the mmtk dependency with a local directory. See: https://github.com/mmtk/mmtk-core/blob/master/.github/scripts/replace-mmtk-dep.py

This script works by deleting the dependencies.mmtk.git node and adding a dependencies.mmtk.path node. That worked if the Cargo.toml is like what it is in the master branch of mmtk-ruby, i.e. the dependencies.mmtk node has exactly one child, namely git.

However, there is a problem. In my mmtk-ruby PR that changes mmtk-ruby for this PR, I modified Cargo.toml to also include a branch node (See https://github.com/wks/mmtk-ruby/blob/feature/rb-roots/mmtk/Cargo.toml):

[dependencies.mmtk]
features = ["is_mmtk_object", "object_pinning"]

# Uncomment one of the following lines to choose where to find mmtk-core.
#git = "https://github.com/mmtk/mmtk-core.git"  # Use mmtk-core from the official repository.
#path = "../../mmtk-core"  # Use mmtk-core from a local repository.
git = "https://github.com/udesou/mmtk-core"  # Use mmtk-core with rb-roots feature
branch = "feature/rb-roots"

By removing the git node and adding a path node, it will look like

branch = "feature/rb-roots"
path = "/home/wks/projects/mmtk-github/mmtk-core"

branch and path cannot co-exist. Therefore, Cargo complained that "key branch is ignored for dependency (mmtk)."

It is a bug in https://github.com/mmtk/mmtk-core/blob/master/.github/scripts/replace-mmtk-dep.py

udesou · 2023-09-07T03:52:27Z

@wks I'm also not familiar with that error. Do you have any idea why the ruby tests failed to run?

When running binding tests for mmtk-core, I used a Python script to replace the mmtk dependency with a local directory. See: https://github.com/mmtk/mmtk-core/blob/master/.github/scripts/replace-mmtk-dep.py

This script works by deleting the dependencies.mmtk.git node and adding a dependencies.mmtk.path node. That worked if the Cargo.toml is like what it is in the master branch of mmtk-ruby, i.e. the dependencies.mmtk node has exactly one child, namely git.

However, there is a problem. In my mmtk-ruby PR that changes mmtk-ruby for this PR, I modified Cargo.toml to also include a branch node (See https://github.com/wks/mmtk-ruby/blob/feature/rb-roots/mmtk/Cargo.toml):
[dependencies.mmtk]
features = ["is_mmtk_object", "object_pinning"]

# Uncomment one of the following lines to choose where to find mmtk-core.
#git = "https://github.com/mmtk/mmtk-core.git"  # Use mmtk-core from the official repository.
#path = "../../mmtk-core"  # Use mmtk-core from a local repository.
git = "https://github.com/udesou/mmtk-core"  # Use mmtk-core with rb-roots feature
branch = "feature/rb-roots"
By removing the git node and adding a path node, it will look like
branch = "feature/rb-roots"
path = "/home/wks/projects/mmtk-github/mmtk-core"
branch and path cannot co-exist. Therefore, Cargo complained that "key branch is ignored for dependency (mmtk)."

It is a bug in https://github.com/mmtk/mmtk-core/blob/master/.github/scripts/replace-mmtk-dep.py

I see. But the local directory would take the branch node into consideration when rewriting from git to path? i.e., is this just a matter of deleting the branchnode after the rewriting?

wks · 2023-09-07T04:03:21Z

I see. But the local directory would take the branch node into consideration when rewriting from git to path? i.e., is this just a matter of deleting the branchnode after the rewriting?

Yes. I made a PR to fix it. (#947) Now the script simply remove all of "git", "branch", "version" and "registry".

But it should be easier to just cherry-pick #947 into this PR and run the CI again.

Remove all keys that may specify the location of a dependency since they may conflict.

udesou · 2023-09-07T04:11:32Z

Done. Thank you @wks!

EDIT: Something still isn't right. Maybe a network error. I'll try rerunning the tests after the other ones finish.

qinsoon · 2023-09-07T04:20:40Z

EDIT: Something still isn't right.

warning: spurious network error (3 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
warning: spurious network error (2 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
warning: spurious network error (1 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)

Seems to be a network issue of the cargo registry. Probably just rerun it later.

qinsoon · 2023-09-10T23:25:53Z

I ran benchmarks for the PR with OpenJDK/Immix/2.5x MC min heap (plotty link). It measures the impact of the code changes and extra buckets from the PR, but it does not measure the transitively pinning or the pinning roots (as the OpenJDK binding does not use them). The results show there is no slowdown in this PR for introducing transitively pinning.

Mutator time

GC time

qinsoon

LGTM

Related PR: mmtk/mmtk-core#897 Co-authored-by: Eduardo Souza <[email protected]>

…#99) Merge after mmtk/mmtk-core#897 to reflect the API change. --------- Co-authored-by: Yi Lin <[email protected]>

This PR adds support for (partially) moving objects in Julia and should be merged with mmtk/julia#27 and mmtk/mmtk-core#897. - It adds support for pinning/unpinning objects and checking if an object is pinned (the implementation uses a pin bit). (`mmtk_pin_object`, `mmtk_unpin_object` and `mmtk_is_pinned`) - It adds support for providing transitively pinned (`tpinned`) roots . - It implements the `copy` function in `object_model.rs`. Note that arrays with inlined data must be treated specially, as their `a->data` pointer needs to be updated after copying. - It uses Julia's GC bits to store forwarding information and the object's header to store the forwarding pointer. - Currently, all stack roots are transitively pinned. Note that we also need to traverse the `tls->live_tasks` to make sure that any stack root from these tasks are transitively pinned. - `scan_julia_object` had to be adapted to cover a few corner cases: - when an array object contains a pointer to the owner of the data, `a->data` needs to be updated in case the owner moves. - the `using` field inside a `jl_module_t` may also be inlined inside the module, and if that's the case, we need to make sure that field is updated if the module moves. - when marking finalizers, traversing the list of malloced arrays, and the list of live tasks at the end of GC, we need to updated these lists with objects that have possibly been moved. - Added a few debug assertions to capture scanning of misaligned objects and roots. NB: I've only tested moving immix; sticky immix is still non-moving. --------- Co-authored-by: Luis Eduardo de Souza Amorim <[email protected]> Co-authored-by: Luis Eduardo de Souza Amorim <[email protected]> Co-authored-by: mmtkgc-bot <[email protected]>

This PR adds support for (partially) moving objects in Julia and should be merged with mmtk/julia#27 and mmtk/mmtk-core#897. - It adds support for pinning/unpinning objects and checking if an object is pinned (the implementation uses a pin bit). (`mmtk_pin_object`, `mmtk_unpin_object` and `mmtk_is_pinned`) - It adds support for providing transitively pinned (`tpinned`) roots . - It implements the `copy` function in `object_model.rs`. Note that arrays with inlined data must be treated specially, as their `a->data` pointer needs to be updated after copying. - It uses Julia's GC bits to store forwarding information and the object's header to store the forwarding pointer. - Currently, all stack roots are transitively pinned. Note that we also need to traverse the `tls->live_tasks` to make sure that any stack root from these tasks are transitively pinned. - `scan_julia_object` had to be adapted to cover a few corner cases: - when an array object contains a pointer to the owner of the data, `a->data` needs to be updated in case the owner moves. - the `using` field inside a `jl_module_t` may also be inlined inside the module, and if that's the case, we need to make sure that field is updated if the module moves. - when marking finalizers, traversing the list of malloced arrays, and the list of live tasks at the end of GC, we need to updated these lists with objects that have possibly been moved. - Added a few debug assertions to capture scanning of misaligned objects and roots. NB: I've only tested moving immix; sticky immix is still non-moving. --------- Co-authored-by: Luis Eduardo de Souza Amorim <[email protected]> Co-authored-by: Luis Eduardo de Souza Amorim <[email protected]> Co-authored-by: mmtkgc-bot <[email protected]> (cherry picked from commit 1622162) # Conflicts: # julia/mmtk_julia.c # mmtk/Cargo.lock # mmtk/Cargo.toml # mmtk/api/mmtk.h # mmtk/src/julia_scanning.rs # mmtk/src/lib.rs # mmtk/src/scanning.rs

udesou added 8 commits August 3, 2023 09:46

Supporting moving immix (wip)

51dcf50

Removing print to file statements

f4cb338

Merge branch 'master' into feature/rb-roots

6f691af

Fixing incompatibilities from other plans to support immovable trace

fa68296

Adding option for not moving during nursery GCs for sticky immix

b1db978

Minor.

a349cc7

Fixing formatting and tests

b1b31cf

Merge branch 'master' into feature/rb-roots

17f3356

udesou requested review from qinsoon and wks August 10, 2023 05:15

udesou added 4 commits August 10, 2023 05:30

Fixing sanity GC

2089e0a

Fix warning in tests

a37fb56

Applying cargo fmt

325d61b

Fixing docs code

b054fd2

wks mentioned this pull request Aug 12, 2023

Expose trace_object to the VM binding #710

Open

wks requested changes Aug 12, 2023

View reviewed changes

src/scheduler/gc_work.rs Outdated Show resolved Hide resolved

src/scheduler/gc_work.rs Outdated Show resolved Hide resolved

src/scheduler/work_bucket.rs Outdated Show resolved Hide resolved

udesou changed the title ~~This PR enables processing "red" and "black" roots for Immix/StickyImmix~~ This PR enables transitively pinning (TP) objects from particular roots for Immix/StickyImmix Aug 16, 2023

udesou added 2 commits August 16, 2023 02:00

Renaming immovable => transitive pin (TP); Adding NodeRootsTrace to h…

407047b

…andle node roots that should be kept it place but their children may move

Run cargo fmt

8921ed7

wks mentioned this pull request Aug 22, 2023

Rename is_pinned, pin_object and unpin_object #917

Open

udesou mentioned this pull request Aug 22, 2023

Changes to support moving immix mmtk/julia#27

Merged

Merge remote-tracking branch 'origin/master' into feature/rb-roots

5a8c8a6

udesou mentioned this pull request Aug 23, 2023

Supporting moving immix mmtk/mmtk-julia#93

Merged

udesou added 2 commits August 23, 2023 04:53

Turning off stress copying as default; fixing logic to prefer copy on…

48dde07

… nursery GC, allowing moving on full heap but disabling it for the nursery GC

Fixed naming of node roots => pinned roots, and tp => tpinned/tpinning

b414746

udesou requested a review from wks August 24, 2023 04:32

qinsoon marked this pull request as ready for review August 24, 2023 04:49

wks mentioned this pull request Aug 24, 2023

WIP: Rename functions related to object pinning. #920

Draft

udesou added 3 commits September 6, 2023 13:41

Merge branch 'master' into feature/rb-roots

119a752

Fixing bug when replacing MarkingProcessEdges in markcompact/global.rs

a91453f

Fixing import that failed cargo clippy

d3fa977

qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Sep 6, 2023

Fix dependency replacing script

f184064

Remove all keys that may specify the location of a dependency since they may conflict.

Merge branch 'master' into feature/rb-roots

2e80e92

qinsoon mentioned this pull request Sep 10, 2023

Do we need structs in plan::tracing and ProcessedgesWork to be public? #950

Open

qinsoon approved these changes Sep 10, 2023

View reviewed changes

udesou added this pull request to the merge queue Sep 11, 2023

Merged via the queue into mmtk:master with commit 61d20e2 Sep 11, 2023

udesou deleted the feature/rb-roots branch September 11, 2023 00:53

wks added a commit to mmtk/mmtk-ruby that referenced this pull request Sep 11, 2023

Upstream API change (#29)

94ae9c1

Related PR: mmtk/mmtk-core#897 Co-authored-by: Eduardo Souza <[email protected]>

wks mentioned this pull request Sep 11, 2023

Fix dependency replacing script #947

Closed

udesou added a commit to mmtk/mmtk-julia that referenced this pull request Sep 12, 2023

Updating code to reflect API change; Bumping rust-toolchain to 1.71.1 (…

b57f962

…#99) Merge after mmtk/mmtk-core#897 to reflect the API change. --------- Co-authored-by: Yi Lin <[email protected]>

This was referenced Oct 26, 2023

Feature: Do not move root objects in Immix #591

Closed

Do not move root objects #705

Closed

Adding support for red/black roots #706

Closed

wks mentioned this pull request Nov 3, 2023

Support copying GC mmtk/mmtk-ruby#18

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This PR enables transitively pinning (TP) objects from particular roots for Immix/StickyImmix #897

This PR enables transitively pinning (TP) objects from particular roots for Immix/StickyImmix #897

udesou commented Aug 10, 2023 •

edited

Loading

wks left a comment

qinsoon commented Aug 13, 2023

wks commented Aug 24, 2023 •

edited

Loading

qinsoon commented Sep 6, 2023

qinsoon commented Sep 7, 2023

udesou commented Sep 7, 2023

wks commented Sep 7, 2023

udesou commented Sep 7, 2023

wks commented Sep 7, 2023

udesou commented Sep 7, 2023 •

edited

Loading

qinsoon commented Sep 7, 2023 •

edited

Loading

qinsoon commented Sep 10, 2023 •

edited

Loading

qinsoon left a comment

This PR enables transitively pinning (TP) objects from particular roots for Immix/StickyImmix #897

This PR enables transitively pinning (TP) objects from particular roots for Immix/StickyImmix #897

Conversation

udesou commented Aug 10, 2023 • edited Loading

wks left a comment

Choose a reason for hiding this comment

qinsoon commented Aug 13, 2023

wks commented Aug 24, 2023 • edited Loading

qinsoon commented Sep 6, 2023

qinsoon commented Sep 7, 2023

udesou commented Sep 7, 2023

wks commented Sep 7, 2023

udesou commented Sep 7, 2023

wks commented Sep 7, 2023

udesou commented Sep 7, 2023 • edited Loading

qinsoon commented Sep 7, 2023 • edited Loading

qinsoon commented Sep 10, 2023 • edited Loading

qinsoon left a comment

Choose a reason for hiding this comment

udesou commented Aug 10, 2023 •

edited

Loading

wks commented Aug 24, 2023 •

edited

Loading

udesou commented Sep 7, 2023 •

edited

Loading

qinsoon commented Sep 7, 2023 •

edited

Loading

qinsoon commented Sep 10, 2023 •

edited

Loading