Allow different GC triggers #712

qinsoon · 2022-12-14T02:57:24Z

This PR adds GCTrigger and forwards calls related with GC triggering to GCTrigger. GCTrigger would allow different implementations, such as fixed heap size, dynamic resizing, and runtime delegation. This PR only implements FixedHeapSizeTrigger and a simplified version of MemBalancerTrigger. This PR closes #561, and makes it easier to implement #641.

Changes:

Adds util::heap::gc_trigger::GCTrigger. Plan::poll() is moved to GCTrigger. Any call to Plan::poll() is replaced with GCTrigger::poll().
Adds util::options::GCTriggerSelector and gc_trigger in Options. gc_trigger defaults to a fixed heap size of half of the physical memory.
Refactoring on arguments for creating plans and spaces.
- I needed to pass GCTrigger to plans and spaces so I think it would be better that I just take this chance to do the refactoring.
- We now have structs to organize the arguments and any plan and space will receive the same set of the arguments for their constructors. This greatly simplifies our code.

Performance:
There is very little difference for the refactoring in this PR. The following compares this PR with master, both using the same fixed heap size. It shows the refactoring and the changes have no measurable overhead.

Plotty link (not public visible)

https://squirrel.anu.edu.au/plotty/yilin/mmtk/#0|skunk-2022-12-14-Wed-054336&benchmark^build^invocation^iteration&bmtime^time^time.other^time.stw&|10&iteration^1^2|20&1^invocation|30&1&benchmark&build;jdk-mmtk-master|41&Histogram%20(with%20CI)^build^benchmark&

GCTriggerPolicy.

for poll

wks

This PR tidied up the creation of plans and spaces. This is a good thing, but it also exposed some potential structural problem in mmtk-core, which should be addressed later.

There are some minor problems in the parsing of options that should be fixed now.

wks · 2022-12-15T08:55:12Z

src/plan/global.rs

+    {
+        // We haven't finished creating the plan. No one is using the GC trigger. We cast the arc into a mutable reference.
+        // TODO: use Arc::get_mut_unchecked() when it is availble.
+        let gc_trigger: &mut GCTrigger<VM> = unsafe { &mut *(Arc::as_ptr(&gc_trigger) as *mut _) };


The use of unsafe is unfortunate, but I can't find a better solution, either, without changing too many places.

I think the root cause of this cyclic reference is that MMTk was designed in an object-oriented way, where objects hold references to things they depends on. In this case, Space needs to trigger GC in acquire, so Space needs to point to a GCTrigger which points to Plan.

It is more idiomatic in Rust to put actors at the centre. In this case, the actor is Mutator. Mutator requests for more memory via alloc, and alloc eventually reaches Space::acquire, and it may trigger GC. Because Mutator has a pointer to an MMTk instance, it has access to everything. So the plan can be obtained from the mutator instance (or the tls argument of Space::acquire).

In the case where it is the GC worker that allocates, it cannot trigger GC anyway, so we don't need those context in that case.

wks · 2022-12-15T11:34:51Z

src/plan/global.rs

+            vm_map: self.global_args.vm_map,
+            mmapper: self.global_args.mmapper,
+            heap: &mut self.global_args.heap,
+            constraints: self.constraints,
+            gc_trigger: self.global_args.gc_trigger.clone(),
+            scheduler: self.global_args.scheduler.clone(),
+            options: &self.global_args.options,


The fact that those globally shared resources are referred by deeply embedded sub-structs indicates that they are just "execution contexts" that are required by everything, and should be held by some higher-level structs such as MMTk or Mutator.

But let's tidy this up later. This PR does a good job by making them less messy.

wks · 2022-12-15T13:12:05Z

src/util/options.rs

+        if let Some(stripped) = s.strip_prefix("FixedHeapSize") {
+            // FixedHeapSize:size
+            if let Some(size_str) = stripped.strip_prefix(':') {
+                return Self::parse_size(size_str).map(Self::FixedHeapSize);
+            }
+        } else if s.starts_with("DynamicHeapSize") {
+            // DynamicHeapSize:min,max
+            if let (Some(colon), Some(comma)) = (s.find(':'), s.find(',')) {
+                let min = Self::parse_size(&s[(colon + 1)..comma])?;
+                let max = Self::parse_size(&s[(comma + 1)..])?;
+                if min > max {
+                    return Err(format!(
+                        "Min heap size {:?} is larger than the given max heap size {:?}",
+                        min, max
+                    ));
+                }
+                return Ok(Self::DynamicHeapSize(min, max));
+            }
+        } else if s.starts_with("Delegated") {
+            return Ok(Self::Delegated);
+        }


Regular expression should make this easier.

Done. Now I am using regex to deal with the parsing. I was concerned whether it is worth to add the regex crate dependency. Maybe it is worth it.

src/util/options.rs

wks · 2022-12-15T13:24:36Z

src/util/options.rs

+                Ok(num * Self::K)
+            } else if s.ends_with('m') {
+                Ok(num * Self::M)
+            } else if s.ends_with('g') {
+                Ok(num * Self::G)


Even if num is within the range of usize, this may still overflow. For example, "1073741824G" is equal to 2 to the power of 64, and that overflows usize. For 32-bit machines, "4G" already overflows usize. Use usize::checked_mul instead.

We can also use u64 or u128 when computing, and convert back to usize afterwards. This can simplify the parsing.

Done. I am parse the number and do multiplication in u64, and convert to usize at the end.

the heap size cannot grow.

descriptor. Deal with multiply overflow.

qinsoon · 2022-12-16T00:46:54Z

binding-refs
OPENJDK_BINDING_REPO=qinsoon/mmtk-openjdk
OPENJDK_BINDING_REF=support-dynamic-heap-resizing
JIKESRVM_BINDING_REPO=qinsoon/mmtk-jikesrvm
JIKESRVM_BINDING_REF=update-pr-712
V8_BINDING_REPO=qinsoon/mmtk-v8
V8_BINDING_REF=update-pr-712

wks · 2022-12-16T03:02:01Z

src/util/options.rs

+            let size = if s.ends_with('k') {
+                num * Self::K
+            } else if s.ends_with('m') {
+                num * Self::M
+            } else if s.ends_with('g') {
+                num * Self::G
+            } else if s.ends_with('t') {
+                num * Self::T


I still think we should still use checked_mul here. Because it is a user-facing interface, it is possible that the user will supply an unreasonably large number and cause an overflow.

The current Rust Reference says overflow will panic in debug mode, but doesn't specify whether it has undefined behaviour or wrapping behaviour. But it is better to be safe.

live for mem balancer

qinsoon · 2022-12-16T04:31:58Z

@wks I also had two commits in the PR for correctness since you first reviewed:
9aa0cba
90bd3e5

wks

LGTM

MarisaKirisame · 2022-12-17T02:22:01Z

subscribing myself to this PR. LMK if you guys have any question.

qinsoon · 2022-12-18T04:06:09Z

subscribing myself to this PR. LMK if you guys have any question.

Thanks a lot for offering help. This PR is mostly about the structural changes to MMTk to support different GC triggers. I will have another PR that implements mem balancer. I will tag you in that PR, and it will be great if you can help check and make sure that it is implemented properly.

This PR reverts a change about where to call `pr.clear_request()` in `Space.acquire()` in #712. The change caused issues like #734. This PR reverts the change. We now clear the reserved pages before GC (the same as before, and the same as Java MMTk), and additionally we inform the GC trigger about pending allocation pages. This closes #734.

This PR reverts a change about where to call `pr.clear_request()` in `Space.acquire()` in mmtk#712. The change caused issues like mmtk#734. This PR reverts the change. We now clear the reserved pages before GC (the same as before, and the same as Java MMTk), and additionally we inform the GC trigger about pending allocation pages. This closes mmtk#734.

This PR adds `GCTrigger` and forwards calls related with GC triggering to `GCTrigger`. `GCTrigger` would allow different implementations, such as fixed heap size, dynamic resizing, and runtime delegation. This PR only implements `FixedHeapSizeTrigger` and a simplified version of `MemBalancerTrigger`. This PR closes mmtk#561, and makes it easier to implement mmtk#641. Changes: * Adds `util::heap::gc_trigger::GCTrigger`. `Plan::poll()` is moved to `GCTrigger`. Any call to `Plan::poll()` is replaced with `GCTrigger::poll()`. * Adds `util::options::GCTriggerSelector` and `gc_trigger` in `Options`. `gc_trigger` defaults to a fixed heap size of half of the physical memory. * Refactoring on arguments for creating plans and spaces. * I needed to pass `GCTrigger` to plans and spaces so I think it would be better that I just take this chance to do the refactoring. * We now have structs to organize the arguments and any plan and space will receive the same set of the arguments for their constructors. This greatly simplifies our code.

This PR reverts a change about where to call `pr.clear_request()` in `Space.acquire()` in mmtk#712. The change caused issues like mmtk#734. This PR reverts the change. We now clear the reserved pages before GC (the same as before, and the same as Java MMTk), and additionally we inform the GC trigger about pending allocation pages. This closes mmtk#734.

qinsoon added 10 commits December 13, 2022 11:03

Add GCTriggerSelector

c31de17

Add an option for gc_trigger

1d5c692

Refactoring args for creating plans and spaces.

92ce2cf

Remove Plan::poll. Add struct GCTrigger. Rename old trait GCTrigger to

3f496eb

GCTriggerPolicy.

Call on_gc_start/end

e3d98b0

Rename GCTriggerPolicySelector to GCTriggerSelector. Add info logging

cbf398b

for poll

Fix build/test/style

05159f8

Fix doc

9053110

Some cleanup

1a4d38d

Fix tests on 32 bits

39c038f

qinsoon requested a review from wks December 14, 2022 21:50

qinsoon marked this pull request as ready for review December 14, 2022 21:50

wks requested changes Dec 15, 2022

View reviewed changes

qinsoon added 2 commits December 16, 2022 10:48

Add can_heap_size_grow for GCTriggerPolicy. We ony do emergency GC if

9aa0cba

the heap size cannot grow.

Use regex to parsing. Separate validation from parsing. Allow T as size

a0dd8f1

descriptor. Deal with multiply overflow.

Fix test in 32bits

fad2e77

wks requested changes Dec 16, 2022

View reviewed changes

qinsoon added 3 commits December 16, 2022 15:18

Keep pending allocation pages in reserved_pages. Use reserved_pages as

90bd3e5

live for mem balancer

Use checked_mul for size multiplication

b9506b3

Merge branch 'master' into gc-trigger-option

a62bc2e

wks approved these changes Dec 16, 2022

View reviewed changes

qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Dec 16, 2022

qinsoon merged commit 2300ce1 into mmtk:master Dec 18, 2022

qinsoon mentioned this pull request Jan 11, 2023

PageAccounting::clear_reserved() called after reset() in GenImmix #734

Closed

qinsoon mentioned this pull request Feb 9, 2023

Notify GC trigger about pending allocation #755

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow different GC triggers #712

Allow different GC triggers #712

qinsoon commented Dec 14, 2022 •

edited

Loading

wks left a comment

wks Dec 15, 2022

wks Dec 15, 2022

wks Dec 15, 2022

qinsoon Dec 16, 2022

wks Dec 15, 2022 •

edited

Loading

qinsoon Dec 16, 2022

qinsoon commented Dec 16, 2022

wks Dec 16, 2022

qinsoon Dec 16, 2022

qinsoon commented Dec 16, 2022

wks left a comment

MarisaKirisame commented Dec 17, 2022

qinsoon commented Dec 18, 2022

Allow different GC triggers #712

Allow different GC triggers #712

Conversation

qinsoon commented Dec 14, 2022 • edited Loading

wks left a comment

Choose a reason for hiding this comment

wks Dec 15, 2022

Choose a reason for hiding this comment

wks Dec 15, 2022

Choose a reason for hiding this comment

wks Dec 15, 2022

Choose a reason for hiding this comment

qinsoon Dec 16, 2022

Choose a reason for hiding this comment

wks Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

qinsoon Dec 16, 2022

Choose a reason for hiding this comment

qinsoon commented Dec 16, 2022

wks Dec 16, 2022

Choose a reason for hiding this comment

qinsoon Dec 16, 2022

Choose a reason for hiding this comment

qinsoon commented Dec 16, 2022

wks left a comment

Choose a reason for hiding this comment

MarisaKirisame commented Dec 17, 2022

qinsoon commented Dec 18, 2022

qinsoon commented Dec 14, 2022 •

edited

Loading

wks Dec 15, 2022 •

edited

Loading