Choose Non-moving Policy based on features #1308

qinsoon · 2025-04-23T06:28:37Z

This PR uses an Immix space as the default non moving space, and adds two features to use mark sweep or immortal as the non moving space.

Mutator now includes 2 Immix allocators (one for Immix as the default space, and the other for Immix as the non moving space).
Call prepare/release/end_of_gc for the non moving space.
Add common_prepare_func and common_release_func for mutators.

commit 4472263 Author: Yi Lin <[email protected]> Date: Thu Apr 17 04:32:13 2025 +0000 Remove an outdated assertion commit abbde8b Author: Yi Lin <[email protected]> Date: Thu Apr 17 03:39:41 2025 +0000 Remove ALLOC_TABLE from local specs. Fix build for 1.74.1 commit 1c64dcb Author: Yi Lin <[email protected]> Date: Thu Apr 17 01:21:45 2025 +0000 Make chunk map as global side metadata commit b7b5988 Author: Yi Lin <[email protected]> Date: Tue Apr 15 04:33:21 2025 +0000 Refactor ChunkState to encode space index

commit 530d80c Author: Yi Lin <[email protected]> Date: Tue Apr 22 04:03:27 2025 +0000 Fix commit 836cac5 Author: Yi Lin <[email protected]> Date: Thu Apr 17 04:27:11 2025 +0000 Allow configuring each Immix space to be non moving

qinsoon · 2025-05-06T06:13:37Z

binding-refs
OPENJDK_BINDING_REPO=qinsoon/mmtk-openjdk
OPENJDK_BINDING_REF=update-mmtk-core-1308
JULIA_BINDING_REPO=qinsoon/mmtk-julia
JULIA_BINDING_REF=update-mmtk-core-1308
JIKESRVM_BINDING_REPO=qinsoon/mmtk-jikesrvm
JIKESRVM_BINDING_REF=update-mmtk-core-1308

…mtk/mmtk-core#1308

qinsoon · 2025-05-08T00:46:27Z

Cargo.toml

 # Group:marksweepallocation
 # default is native allocator with lazy sweeping
 eager_sweeping = []
 # Use library malloc as the freelist allocator for mark sweep. This will makes mark sweep slower. As malloc may return addresses outside our
 # normal heap range, we will have to use chunk-based SFT table. Turning on this feature will use a different SFT map implementation on 64bits,
 # and will affect all the plans in the build. Please be aware of the consequence, and this is only meant to be experimental use.
 malloc_mark_sweep = []
+
+# Group:nonmovingspace
+immortal_as_nonmoving = []


This is a bit awkward. Ideally, we should have 3 features immix_as_nonmoving, immortal_as_nonmoving and marksweep_as_nonmoving, and we use immix_as_nonmoving as a default feature. But we need to ensure that for every build, exactly one feature from those 3 features is enabled. This also applies to the mark sweep features above, which should be 3 features lazy_sweeping, eager_sweeping and malloc_mark_sweep, and we need to choose exactly one.

So basically we need mutually exclusive features, and we need to make sure the features are mandatory that you need to use one of them. We cannot do this with cargo. In our test script, we try to achieve this when we cover all the builds with different features, but our test script only supports optional mutually exclusive features. So for now, we assume the default (without any feature to control it), and make the other two features optional and mutually exclusive.

I would like to do this refactoring in a separate PR.

We can do something like this: https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features

Default features can be disabled (see https://doc.rust-lang.org/cargo/reference/features.html#the-default-feature). mmtk-core currently has one default feature: builtin_env_logger, and VM bindings that use their own logging frameworks (such as OpenJDK) could disable default features and wire Rust logs into their own.

If the user disables default features, there will be neither immix_as_nonmoving, immortal_as_nonmoving nor marksweep_as_nonmoving. So we need to consider the case that there is no non-moving space.

Actually I think the default should be "no non-moving space". Not all VMs need that. Neither OpenJDK, JikesRVM nor Ruby need it. We can make all of them optional, and use the trick described in https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features to reject multiple non-moving spaces.

wks

I refactored the code a bit to make NonMovingSpace a trait. By doing so, we greatly reduced the amount of cfg attributes used.

https://github.com/qinsoon/mmtk-core/compare/nonmoving-space-options...wks:mmtk-core:feature/nonmoving-space-options-trait?expand=1

~~If you think that is appropriate, you can cherry-pick it into this PR.~~
Please tell me if it is appropriate, as I am still adding comments and making other changes.

wks · 2025-05-08T07:58:45Z

OK. I have pushed more changes to

https://github.com/qinsoon/mmtk-core/compare/nonmoving-space-options...wks:mmtk-core:feature/nonmoving-space-options-trait?expand=1

@qinsoon You can merge it into your nonmoving-space-options branch to make it part of this PR if you think it is appropriate.

I am also trying to further remove the cfg in create_allocator_mapping (see here), but without success. I basically wanted to use the "visitor pattern" to remove the cascading if-else. But the ChosenNonMovingSpace type still has a <VM> type parameter, while the create_allocator_mapping doesn't have that. I think there is a deeper problem that create_allocator_mapping should really visit each space in the Plan and let each space reserve its desired allocator. But that requires more refactoring, and may be better done in a separate PR.

wks · 2025-05-08T08:08:51Z

src/plan/global.rs

+#[cfg(feature = "immortal_as_nonmoving")]
+pub type NonMovingSpace<VM> = crate::policy::immortalspace::ImmortalSpace<VM>;
+
+#[cfg(not(any(feature = "immortal_as_nonmoving", feature = "marksweep_as_nonmoving")))]
+pub type NonMovingSpace<VM> = crate::policy::immix::ImmixSpace<VM>;
+
+#[cfg(feature = "marksweep_as_nonmoving")]
+pub type NonMovingSpace<VM> = crate::policy::marksweepspace::native_ms::MarkSweepSpace<VM>;


If you think it is not worth introducing the trait NonMovingSpace as I suggested in #1308 (comment), we can still simplify this part of the code with cfg_if::cfg_if!. The else branch can save you from having to type not(any(feature = "immortal_as_nonmoving", feature = "marksweep_as_nonmoving")).

See my comments in #1308 (comment). I would like to introduce a feature immix_as_nonmoving as a default feature. That would solve most of the issues here with annoying cfg (we also have the same issue with the features like malloc_jemalloc, and eager_sweep).

wks · 2025-05-08T08:09:11Z

src/plan/global.rs

        &self.nonmoving
    }
+
+    fn new_nonmoving_space(args: &mut CreateSpecificPlanArgs<VM>) -> NonMovingSpace<VM> {


And we can use cfg_if in these functions, too.

wks · 2025-05-08T08:10:50Z

src/plan/mutator_context.rs

+        } else {
+            panic!("No policy selected for nonmoving space")


Actually the first branch cfg!(not(any(feature = "immortal_as_nonmoving", feature = "marksweep_as_nonmoving"))) covers all other cases, so this else is unreachable. But I suggest we move reserved.add_immix_allocator() into this else branch and removt the first if cfg!(not(any(..., ...))) branch.

src/plan/mutator_context.rs

qinsoon · 2025-05-08T09:28:41Z

I refactored the code a bit to make NonMovingSpace a trait. By doing so, we greatly reduced the amount of cfg attributes used.

https://github.com/qinsoon/mmtk-core/compare/nonmoving-space-options...wks:mmtk-core:feature/nonmoving-space-options-trait?expand=1

~~If you think that is appropriate, you can cherry-pick it into this PR.~~ Please tell me if it is appropriate, as I am still adding comments and making other changes.

I considered introducing a struct NonMovingSpace, but I gave up the idea. I think it is inconsistent with the design. We have policies which are GC algorithms, and we use those policies for certain spaces, such as non moving, or nursery. It would be strange to have something like NonMovingSpace, which confuses GC algorithms and its actual use.

Besides, the methods in the proposed ChosenNonMovingSpace are just methods that should have been in the Space. I think we could just add methods like prepare, release, end_of_gc, etc, to Space. Some policies may need extra parameters like the defrag stats for Immix, we can invoke a separate method in the ImmixSpace to set the parameter, and make prepare/release/end_of_gc have the same parameters for all the spaces.

qinsoon · 2025-05-09T01:26:53Z

I have made changes according to the comments about the cfg_if.

qinsoon · 2025-05-09T01:28:22Z

OK. I have pushed more changes to

https://github.com/qinsoon/mmtk-core/compare/nonmoving-space-options...wks:mmtk-core:feature/nonmoving-space-options-trait?expand=1

@qinsoon You can merge it into your nonmoving-space-options branch to make it part of this PR if you think it is appropriate.

I am also trying to further remove the cfg in create_allocator_mapping (see here), but without success. I basically wanted to use the "visitor pattern" to remove the cascading if-else. But the ChosenNonMovingSpace type still has a <VM> type parameter, while the create_allocator_mapping doesn't have that. I think there is a deeper problem that create_allocator_mapping should really visit each space in the Plan and let each space reserve its desired allocator. But that requires more refactoring, and may be better done in a separate PR.

I created a draft PR that moves prepare/release/end_of_gc to trait Space. With this PR, we should not need an extra NonMovingSpace trait to unify the signature of prepare/release/end_of_gc.

wks

Two more things to change:

Some #[cfg] can be re-written with cfg_if. I admit that without visitor pattern or generalizing the prepare/release/end_of_gc, chained if-else may be the best thing we can get for now.
If we use the default value of needs_prepare_mutator, we can omit that line because ..PlanConstraints::default() will fill in default values.

wks · 2025-05-09T05:19:20Z

src/plan/global.rs

+    fn prepare_nonmoving_space(&mut self, _full_heap: bool) {
+        #[cfg(feature = "immortal_as_nonmoving")]
+        self.nonmoving.prepare();
+        #[cfg(not(any(feature = "immortal_as_nonmoving", feature = "marksweep_as_nonmoving")))]


This is a candidate for cfg_if!, too.

wks · 2025-05-09T05:19:26Z

src/plan/global.rs

+    fn release_nonmoving_space(&mut self, _full_heap: bool) {
+        #[cfg(feature = "immortal_as_nonmoving")]
+        self.nonmoving.release();
+        #[cfg(not(any(feature = "immortal_as_nonmoving", feature = "marksweep_as_nonmoving")))]


This is a candidate for cfg_if!, too.

wks · 2025-05-09T05:19:30Z

src/plan/global.rs

+        // Only mark sweep and immix need end of GC.
+        #[cfg(feature = "marksweep_as_nonmoving")]
+        self.nonmoving.end_of_gc();
+        #[cfg(not(any(feature = "immortal_as_nonmoving", feature = "marksweep_as_nonmoving")))]


This is a candidate for cfg_if!, too.

wks · 2025-05-09T05:20:16Z

src/plan/immix/global.rs

@@ -42,7 +42,7 @@ pub const IMMIX_CONSTRAINTS: PlanConstraints = PlanConstraints {
    moves_objects: !cfg!(feature = "immix_non_moving"),
    // Max immix object size is half of a block.
    max_non_los_default_alloc_bytes: crate::policy::immix::MAX_IMMIX_OBJECT_SIZE,
-    needs_prepare_mutator: false,
+    needs_prepare_mutator: false || PlanConstraints::default().needs_prepare_mutator,


false || x is just x. But we can just delete this line because the line ..PlanConstraints::default() will include all the default values, including this one.

wks · 2025-05-09T05:21:23Z

src/plan/markcompact/global.rs

@@ -42,7 +42,7 @@ pub const MARKCOMPACT_CONSTRAINTS: PlanConstraints = PlanConstraints {
    needs_forward_after_liveness: true,
    max_non_los_default_alloc_bytes:
        crate::plan::plan_constraints::MAX_NON_LOS_ALLOC_BYTES_COPYING_PLAN,
-    needs_prepare_mutator: false,
+    needs_prepare_mutator: false || PlanConstraints::default().needs_prepare_mutator,


wks · 2025-05-09T05:55:15Z

src/plan/semispace/global.rs

@@ -40,7 +40,7 @@ pub const SS_CONSTRAINTS: PlanConstraints = PlanConstraints {
    moves_objects: true,
    max_non_los_default_alloc_bytes:
        crate::plan::plan_constraints::MAX_NON_LOS_ALLOC_BYTES_COPYING_PLAN,
-    needs_prepare_mutator: false,
+    needs_prepare_mutator: false || PlanConstraints::default().needs_prepare_mutator,


Same is here.

wks · 2025-05-09T05:55:23Z

src/plan/generational/mod.rs

@@ -50,7 +50,7 @@ pub const GEN_CONSTRAINTS: PlanConstraints = PlanConstraints {
    may_trace_duplicate_edges: ACTIVE_BARRIER.equals(BarrierSelector::ObjectBarrier),
    max_non_los_default_alloc_bytes:
        crate::plan::plan_constraints::MAX_NON_LOS_ALLOC_BYTES_COPYING_PLAN,
-    needs_prepare_mutator: false,
+    needs_prepare_mutator: false || PlanConstraints::default().needs_prepare_mutator,


wks · 2025-05-09T05:55:34Z

src/plan/pageprotect/global.rs

@@ -31,7 +31,7 @@ pub struct PageProtect<VM: VMBinding> {
 /// The plan constraints for the page protect plan.
 pub const CONSTRAINTS: PlanConstraints = PlanConstraints {
    moves_objects: false,
-    needs_prepare_mutator: false,
+    needs_prepare_mutator: false || PlanConstraints::default().needs_prepare_mutator,


wks · 2025-05-09T06:01:56Z

src/plan/mutator_context.rs

+        .unwrap()
+        .release();
+    }
+    #[cfg(not(any(feature = "immortal_as_nonmoving", feature = "marksweep_as_nonmoving")))]


We can use cfg_if!. Although "immortal_as_nonmoving" needs no action in this function. I still think it is more explicit and more readable to have something like:

cfg_if::cfg_if! { if #[cfg(feature = "immortal_as_nonmoving")] { // We don't need to do anything for ImmortalSpace. } else if #[cfg(feature = "mark_sweep_as_nonmoving")] { // ... } else { // ... } }

At least we won't miss any cases if we write this way.

wks

LGTM.

There is an opportunity to remove some unsafe, but we can leave it to another PR.

wks · 2025-05-09T07:47:02Z

src/plan/mutator_context.rs

+    ///
+    /// # Safety
+    /// The semantic needs to match the allocator type.
+    pub unsafe fn allocator_impl_for_semantic<T: Allocator<VM>>(


This function is labelled as unsafe, but it is only used in non-performance-critical paths, i.e. common_prepare_func and common_release_func which are only called once per mutator per GC. We could make it return Option<&T> or &dyn Allocator instead to make it safe, although that would require a unwrap() or downcasting before using.

But the source of the unsafety is in Allocators::get_allocator_mut. Currently that is used in Mutator::alloc which includes the Rust-implemented non-inlined fast path. I think any VM that cares about the performance should implement the bump-pointer fast path in the VM binding, and I don't think it is worth using unsafe here. But let's leave it for another PR.

qinsoon added 8 commits April 23, 2025 00:15

Squashed commit of the following:

e885f63

commit 530d80c Author: Yi Lin <[email protected]> Date: Tue Apr 22 04:03:27 2025 +0000 Fix commit 836cac5 Author: Yi Lin <[email protected]> Date: Thu Apr 17 04:27:11 2025 +0000 Allow configuring each Immix space to be non moving

Use Immix as non moving space

de4d5f8

Choose nonmoving policy based on the feature

f45d87e

Merge branch 'master' into nonmoving-space-options

d15c475

Make sure prepare, release, end_of_gc is properly called

89b934e

Work around an issue about trace kind

d98137f

Fix doc

9ef1cb7

qinsoon added 2 commits May 6, 2025 18:47

Update immixspace.rs

98e9ed5

Immix needs post_scan. Fix style check

0763fb7

qinsoon force-pushed the nonmoving-space-options branch from 654ec4d to 0763fb7 Compare May 7, 2025 07:10

udesou added a commit to mmtk/mmtk-julia that referenced this pull request May 8, 2025

Using mmtk-core branch that has changes from mmtk/mmtk-core#1089 and m…

594e80a

…mtk/mmtk-core#1308

qinsoon commented May 8, 2025

View reviewed changes

Fix the leftover FIXMEs

c541b2f

qinsoon added the PR-extended-testing Run extended tests for the pull request label May 8, 2025

qinsoon requested a review from wks May 8, 2025 04:39

qinsoon marked this pull request as ready for review May 8, 2025 04:39

wks reviewed May 8, 2025

View reviewed changes

Address reviews about cfg_if

aa5b6ee

This was referenced May 9, 2025

Move prepare/release/end_of_gc to trait Space #1315

Closed

Arguments of prepare, release and end_of_gc of each space #1316

Open

wks requested changes May 9, 2025

View reviewed changes

qinsoon added 2 commits May 9, 2025 06:59

Use cfg_if where we can. Remove unnecessary needs_mutator_prepare.

57624ad

Minor refactoring

bdf69fd

Merge branch 'master' into nonmoving-space-options

cf5c91d

wks approved these changes May 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Choose Non-moving Policy based on features #1308

Choose Non-moving Policy based on features #1308

qinsoon commented Apr 23, 2025 •

edited

Loading

qinsoon commented May 6, 2025 •

edited

Loading

qinsoon May 8, 2025

k-sareen May 8, 2025

wks May 8, 2025

wks left a comment •

edited

Loading

wks commented May 8, 2025

wks May 8, 2025

qinsoon May 8, 2025

wks May 8, 2025

wks May 8, 2025

qinsoon commented May 8, 2025

qinsoon commented May 9, 2025

qinsoon commented May 9, 2025

wks left a comment

wks May 9, 2025

wks May 9, 2025

wks May 9, 2025

wks May 9, 2025

wks May 9, 2025

wks May 9, 2025

wks May 9, 2025

wks May 9, 2025

wks May 9, 2025

wks left a comment

wks May 9, 2025

Choose Non-moving Policy based on features #1308

Are you sure you want to change the base?

Choose Non-moving Policy based on features #1308

Conversation

qinsoon commented Apr 23, 2025 • edited Loading

qinsoon commented May 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wks left a comment • edited Loading

Choose a reason for hiding this comment

wks commented May 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinsoon commented May 8, 2025

qinsoon commented May 9, 2025

qinsoon commented May 9, 2025

wks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinsoon commented Apr 23, 2025 •

edited

Loading

qinsoon commented May 6, 2025 •

edited

Loading

wks left a comment •

edited

Loading