Move some states from BasePlan to GlobalState #949

qinsoon · 2023-09-08T07:13:05Z

This PR moves some states from BasePlan to a new struct GlobalState. Before this change, the states can only be accessed through Plan. Whoever needs to access those states need to acquire or store a reference to Plan, which is unnecessary. With the changes in the PR, GlobalState is stored as Arc<GlobalState>. It can be directly accessed through the MMTK instance, and can also be stored in some components, such as allocators. This change reduces the dependency on Plan.

plan. Introduce AllocatorContext

wks · 2023-09-08T07:48:11Z

Nice work. This PR addressed some long-standing issues.

Issue #532: The last work item in that issue is moving GCRequester out of BasePlan. This PR will fix it.

Issue #587: That issue mentioned some fields that were accidentally added to BasePlan, but should be placed in MMTK or other global data structures. That includes initialized, options and gc_requester. This PR will fix those fields, too.

#587 (comment) also mentioned some previous static methods in JikesRVM MMTk. Those methods were in BasePlan, and this PR moves them to MMTK or GlobalState.

qinsoon · 2023-09-19T01:16:16Z

The refactoring seems to slow down the STW/GC time. I can't find an obvious reason that causes the slowdown. I will create separate PRs for a subset of changes, evaluate and merge them separately.

qinsoon · 2023-10-17T23:00:00Z

The PR does not seem to have any measurable performance difference. The following is the total time for 4 plans, SemiSpace, GenCopy, Immix and GenImmix. More details can be found in the link: plotty

SemiSpace

GenCopy

Immix

GenImmix

src/plan/global.rs

qinsoon · 2023-10-17T23:35:55Z

src/vm/active_plan.rs

-    // TODO: I don't know how this can be implemented when we have multiple MMTk instances.
-    // This function is used by space and phase to refer to the current plan.
-    // Possibly we should remove the use of this function, and remove this function?
-    fn global() -> &'static dyn Plan<VM = VM>;


I will update bindings for this change.

qinsoon · 2023-10-17T23:38:14Z

src/util/alloc/malloc_allocator.rs

-    /// [`Plan`] instance that this allocator instance is associated with.
-    plan: &'static dyn Plan<VM = VM>,
+    context: Arc<AllocatorContext<VM>>,
+    _pad: usize,


I am currently use a padding field to make sure the size of the allocator types stays the same as before. I plan to remove this field, and update the bindings.

qinsoon · 2023-10-17T23:42:09Z

@wks Can you review this PR? I haven't created binding PRs for it, but I would like to get some reviews for the PR first. Updating bindings should be straightforward after that.

wks

The mmtk.h headers need to be updated to remove the modify_check functions.

src/mmtk.rs

src/plan/global.rs

src/util/alloc/allocator.rs

src/util/alloc/free_list_allocator.rs

wks

I have finished this pass of reviewing. Generally speaking this is a nice PR that solves some long-standing structural problems.

src/policy/immix/defrag.rs

wks

The mmtk-core part looks good to me. Just ensure bindings are fixed as well.

qinsoon · 2023-10-19T23:20:47Z

This PR is not ready for merge. The binding PRs need to be ready, reviewed and approved before we can merge this PR.

qinsoon · 2023-10-20T04:27:55Z

binding-refs
OPENJDK_BINDING_REPO=qinsoon/mmtk-openjdk
OPENJDK_BINDING_REF=update-mmtk-core-pr-949
JIKESRVM_BINDING_REPO=qinsoon/mmtk-jikesrvm
JIKESRVM_BINDING_REF=update-mmtk-core-pr-949
V8_BINDING_REPO=qinsoon/mmtk-v8
V8_BINDING_REF=update-mmtk-core-pr-949
JULIA_BINDING_REPO=qinsoon/mmtk-julia
JULIA_BINDING_REF=update-mmtk-core-pr-949
RUBY_BINDING_REPO=qinsoon/mmtk-ruby
RUBY_BINDING_REF=update-mmtk-core-pr-949

Updates to mmtk/mmtk-core#949. --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>

Updates to mmtk/mmtk-core#949 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>

Updates to mmtk/mmtk-core#949. --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>

qinsoon added 4 commits September 7, 2023 07:01

Move global state from BasePlan to GlobalState

efe572d

Move more stuff out of BasePlan. Allocator no longer keeps a ref to

8768970

plan. Introduce AllocatorContext

Fix build and style

2e2e9aa

Remove ActivePlan::global

c4b3160

qinsoon added 4 commits September 12, 2023 01:58

Introduce notify_collection_required and refactor collection_required.

f14ad1a

Fix docs

a6fd28a

Some cleanup

d2c929d

Merge branch 'master' into refactor-global-state

5c13a6c

Revert changes about collection_required

280758a

qinsoon mentioned this pull request Sep 21, 2023

Remove cast_ref_to_mut related to Prepare and Release #852

Open

qinsoon mentioned this pull request Sep 29, 2023

Introduce ArcFlexMut for Space references #965

Draft

Merge branch 'master' into refactor-global-state

753b511

Minor cleanup

4bd24a6

qinsoon marked this pull request as ready for review October 17, 2023 23:25

qinsoon commented Oct 17, 2023

View reviewed changes

qinsoon requested a review from wks October 17, 2023 23:42

wks requested changes Oct 18, 2023

View reviewed changes

src/mmtk.rs Show resolved Hide resolved

src/mmtk.rs Show resolved Hide resolved

src/mmtk.rs Show resolved Hide resolved

src/plan/global.rs Show resolved Hide resolved

wks requested changes Oct 18, 2023

View reviewed changes

src/util/alloc/allocator.rs Outdated Show resolved Hide resolved

src/util/alloc/allocator.rs Outdated Show resolved Hide resolved

src/util/alloc/free_list_allocator.rs Outdated Show resolved Hide resolved

wks requested changes Oct 18, 2023

View reviewed changes

src/policy/immix/defrag.rs Outdated Show resolved Hide resolved

qinsoon added 3 commits October 19, 2023 00:16

Remove mmtk_modify_check from header files

607ef6f

Use store instead of swap. Rename PlanStatsForDefrag to StatsForDefrag.

068d5a9

Remove the _pad field in allocators

d5f5ac9

wks approved these changes Oct 19, 2023

View reviewed changes

This was referenced Oct 20, 2023

Update to MMTk core PR #949 mmtk/mmtk-openjdk#240

Merged

Update to MMTk core PR #949 mmtk/mmtk-ruby#36

Merged

This was referenced Oct 20, 2023

Update to MMTk core PR #949 mmtk/mmtk-julia#109

Merged

Update to MMTk core PR #949 mmtk/mmtk-jikesrvm#152

Merged

Update to MMTk core PR #949 mmtk/mmtk-v8#81

Merged

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

qinsoon added this pull request to the merge queue Oct 24, 2023

Merged via the queue into mmtk:master with commit 57af17f Oct 24, 2023
25 of 26 checks passed

qinsoon deleted the refactor-global-state branch October 24, 2023 02:12

mmtkgc-bot added a commit to mmtk/mmtk-v8 that referenced this pull request Oct 24, 2023

Update to MMTk core PR #949 (#81)

1dc0803

Updates to mmtk/mmtk-core#949. --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>

mmtkgc-bot added a commit to mmtk/mmtk-ruby that referenced this pull request Oct 24, 2023

Update to MMTk core PR #949 (#36)

6992549

Updates to mmtk/mmtk-core#949 --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>

mmtkgc-bot added a commit to mmtk/mmtk-openjdk that referenced this pull request Oct 24, 2023

Update to MMTk core PR #949 (#240)

60a8357

Updates to mmtk/mmtk-core#949. --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>

qinsoon added a commit to mmtk/mmtk-jikesrvm that referenced this pull request Oct 24, 2023

Update to MMTk core PR #949 (#152)

77da53e

Updates to mmtk/mmtk-core#949. --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>

mmtkgc-bot added a commit to mmtk/mmtk-julia that referenced this pull request Oct 24, 2023

Update to MMTk core PR #949 (#109)

bb00563

Updates to mmtk/mmtk-core#949. --------- Co-authored-by: mmtkgc-bot <mmtkgc.bot@gmail.com>

wks mentioned this pull request Nov 22, 2023

Refactor scheduler #532

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move some states from BasePlan to GlobalState #949

Move some states from BasePlan to GlobalState #949

qinsoon commented Sep 8, 2023

wks commented Sep 8, 2023

qinsoon commented Sep 19, 2023

qinsoon commented Oct 17, 2023

qinsoon Oct 17, 2023

qinsoon Oct 17, 2023

qinsoon commented Oct 17, 2023

wks left a comment

wks left a comment

wks left a comment

qinsoon commented Oct 19, 2023

qinsoon commented Oct 20, 2023

Move some states from BasePlan to GlobalState #949

Move some states from BasePlan to GlobalState #949

Conversation

qinsoon commented Sep 8, 2023

wks commented Sep 8, 2023

qinsoon commented Sep 19, 2023

qinsoon commented Oct 17, 2023

SemiSpace

GenCopy

Immix

GenImmix

qinsoon Oct 17, 2023

Choose a reason for hiding this comment

qinsoon Oct 17, 2023

Choose a reason for hiding this comment

qinsoon commented Oct 17, 2023

wks left a comment

Choose a reason for hiding this comment

wks left a comment

Choose a reason for hiding this comment

wks left a comment

Choose a reason for hiding this comment

qinsoon commented Oct 19, 2023

qinsoon commented Oct 20, 2023