Implement mem balancer #724

qinsoon · 2023-01-03T01:31:16Z

This PR implements mem balancer (https://dl.acm.org/doi/pdf/10.1145/3563323).

Changes:

Introduce a trait GenerationalPlan: Plan, and move methods that are specific to generational plans to the new trait from the old trait Plan.
Change the return type of Plan::generational() to return an option of &dyn GenerationalPlan. We can use this to know whether a plan is generational or not. If it is, we can further invoke methods about the generational plan.
Add GCTriggerPolicy::on_gc_release() which is called in the Release work packet. A GCTriggerPolicy can use this to get stats before we reclaim memory (e.g. to get promoted size).
Add an implementation of mem balancer to replace the current simple resize policy.
Add some logging to help debug GC triggering.

GCTriggerPolicy.

for poll

the heap size cannot grow.

descriptor. Deal with multiply overflow.

end_of_gc

qinsoon · 2023-01-04T23:25:58Z

@MarisaKirisame This is the PR that implements mem balancer. I was trying to implement based on my understanding of the paper. Let me know if there is any issue. Any comment is welcome. The changes that may be interesting to you is in src/util/heap/gc_trigger.rs.

k-sareen · 2023-01-05T02:32:39Z

Is it possible to implement the fix for nursery GCs we discussed in #723 in this PR? Or will we have a separate PR for that.

qinsoon · 2023-01-05T03:01:12Z

Is it possible to implement the fix for nursery GCs we discussed in #723 in this PR? Or will we have a separate PR for that.

Yeah, I think that is a different issue.

wks · 2023-01-05T03:16:20Z

src/util/heap/gc_trigger.rs

@@ -139,27 +147,203 @@ pub struct MemBalancerTrigger {
    max_heap_pages: usize,
    /// The current heap size
    current_heap_pages: AtomicUsize,
+    /// Statistics
+    stats: Atomic<MemBalancerStats>,


The Atomic type is used on a complex struct. The underlying implementation uses a global hash map to assign a spin lock for each hash value of address. (See: https://docs.rs/atomic/latest/src/atomic/fallback.rs.html#101 ) This is not really lock-free, and is not efficient, either.

Given that this struct is only used at GC start/release/end when no two threads access that struct concurrently, I suggest using AtomicRefCell and it’s borrow_mut method (you can implement access_stats with them). It panics if two threads attempt to borrow it mutably at the same time, but given the use pattern, this shouldn’t happen unless there is a bug.

GC work packet stats are implemented using AtomicRefCell, too.

Alternatively you can always use Mutex.

Done. I changed that to AtomicRefCell.

MarisaKirisame · 2023-01-05T21:29:30Z

it look good. there are tricks here and there (e.g. you could try to add a constant 1MB to all heap size), but no doubt you guys will find them when doing some performance turning.

src/util/options.rs

wks

I observed an phenomenon that some methods of Plan are only relevant to generational plans. It includes:

The old generational() that returns &Gen<Self::VM> without Option
is_current_gc_nursery: Only called by the prepare method of generational plans and the newly added GCTrigger.
get_mature_physical_pages_available: Only called by Gen
Newly added get_mature_reserved_pages: Only called by GCTrigger in generational_mem_stats_on_gc_release.

In Plan, they are implemented as stubs. is_current_gc_nursery returns false; other methods panic if the plan is not generational.

With this PR, the new generational() method returns an Option<&Gen>. I think we can refactor the code and move those generational-specific methods to Gen. Existing call sites should already have access to &Gen instance. If not, the Plan::generational() method can return an Option for the caller to match against. (See my comments on the code below.)

However, I am not sure how to implement get_mature_physical_pages_available because it needs to know which space is the "mature space", and that is per-plan knowledge. Intuitively, there should be another trait, such as

trait GenPlan: Plan {
    fn is_current_gc_nursery(&self) -> bool;
    fn get_mature_physical_pages_available(&self) -> usize;
    fn get_mature_reserved_pages(&self) -> usize;
}

Concrete plans, such as GenCopy and GenImmix shall implement GenPlan in addition to Plan.

I am trying to refactor locally, but haven't managed to get the downcast operation working.

What do you think? If it is too complicated, we can defer this refactoring to another PR.

src/plan/global.rs

src/util/heap/gc_trigger.rs

wks · 2023-01-10T06:31:28Z

src/scheduler/gc_work.rs

@@ -108,6 +108,8 @@ impl<C: GCWorkContext> Release<C> {
 impl<C: GCWorkContext + 'static> GCWork<C::VM> for Release<C> {
    fn do_work(&mut self, worker: &mut GCWorker<C::VM>, mmtk: &'static MMTK<C::VM>) {
        trace!("Release Global");
+        self.plan.base().gc_trigger.policy.on_gc_release(mmtk);


Not a problem for now. This is added before the VMCollection::vm_release() below, but plan_mut.release happens after VMCollection::vm_release(). It's a bit weird this way, but it doesn't matter if the VM doesn't implement vm_release.

VMCollection::vm_release() was created for VMs to do what our current RefEnqueue does to handle weak references. Existing bindings (except mmtk-openjdk in the lxr branch) do not use it, but with my new weak-ref API, the openjdk binding should use it instead of relying on mmtk-core's RefEnqueue work packet. I am considering moving this <C::VM as VMBinding>::VMCollection::vm_release(); statement to a dedicated work packets, because ref-enqueuing work does not depend on the Plan instance or the release of the Plan, and can therefore be parallelised.

If we can run some code when we open the Release bucket, that would be a good timing to run this on_gc_release() method.

src/util/heap/gc_trigger.rs

qinsoon · 2023-01-12T04:10:30Z

@wks I have done some refactoring about methods that are in Plan but should be specific to generational plans. See 5aa7023 for the changes. In short, Plan::generational() now returns Option<&dyn GenerationalPlan> so we do not need to do downcast. Let me know if there could be any issue on this change.

wks

Nice work.

Two problems remain.

One is the naming of the Gen struct and the gen() method. It is a bit confusing to have both trait GenerationalPlan and struct Gen. Similar is true for generational() and gen(). I think we can rename Gen to CommonGenPlan to indicate that it plays a similar role as CommonPlan, i.e. CommonGenPlan is the common parts of most generational plans, whereas CommonPlan is the common parts of most plans.

The other is the is_nursery_gc function. This function works for both generational plans and non-generational plans. It is used in two places.

Finalization processor. It still uses is_nursery_gc as a hint so that it only needs to look at newly registered finalizable objects since the last GC. It is still helpful.
The ProcessModBuf and ProcessRegionModBuf work packets. Those work packets are only used by generational plans, so they can be sure that the current plan is generational. I think they can skip the map_or part, and panic if the plan is not generational.

src/plan/generational/gc_work.rs

src/plan/generational/global.rs

src/plan/generational/gc_work.rs

Generational work packets directly use is_current_gc_nursery() from CommonGenPlan.

wks

It looks good to me now.

This PR implements mem balancer (https://dl.acm.org/doi/pdf/10.1145/3563323). Changes: * Introduce a trait `GenerationalPlan: Plan`, and move methods that are specific to generational plans to the new trait from the old `trait Plan`. * Change the return type of `Plan::generational()` to return an option of `&dyn GenerationalPlan`. We can use this to know whether a plan is generational or not. If it is, we can further invoke methods about the generational plan. * Add `GCTriggerPolicy::on_gc_release()` which is called in the `Release` work packet. A `GCTriggerPolicy` can use this to get stats before we reclaim memory (e.g. to get promoted size). * Add an implementation of mem balancer to replace the current simple resize policy. * Add some logging to help debug GC triggering.

qinsoon added 22 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

Implement complete MemBalancer. Add on_gc_release for GC trigger.

d5a9300

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.

Merge branch 'gc-trigger-option' into complete-mem-balancer

1ef4e85

Merge branch 'master' into complete-mem-balancer

0e23ce0

Minor fix

dd29f39

Add Plan.end_of_gc. Generational plans set next_heap_full_size in

8431b7a

end_of_gc

Fix typo

c34c6b4

Merge branch 'plan-end-of-gc' into complete-mem-balancer

9eb4dbf

Update mem balancer for generational plan

325e412

Merge branch 'master' into complete-mem-balancer

c3b0d6a

Tidy up

a55866b

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

qinsoon marked this pull request as ready for review January 4, 2023 23:21

qinsoon requested a review from wks January 4, 2023 23:21

wks reviewed Jan 5, 2023

View reviewed changes

qinsoon added 2 commits January 6, 2023 10:23

Merge branch 'master' into complete-mem-balancer

fe3879c

Use AtomicRefCell instead of Atomic for MemBalancerStats

21ac93a

k-sareen reviewed Jan 6, 2023

View reviewed changes

src/util/options.rs Outdated Show resolved Hide resolved

wks reviewed Jan 6, 2023

View reviewed changes

src/plan/global.rs Outdated Show resolved Hide resolved

src/plan/global.rs Outdated Show resolved Hide resolved

src/util/heap/gc_trigger.rs Outdated Show resolved Hide resolved

src/util/heap/gc_trigger.rs Outdated Show resolved Hide resolved

wks reviewed Jan 10, 2023

View reviewed changes

src/util/heap/gc_trigger.rs Outdated Show resolved Hide resolved

qinsoon mentioned this pull request Jan 11, 2023

Clarify contract/semantics of "Release" and other work buckets #717

Open

qinsoon added 4 commits January 12, 2023 14:57

Add trait GenerationalPlan

5aa7023

Fix typo in comments

b2f83c7

Use Option<f64> for MemBalancerStats previous values

1b88d90

Merge branch 'master' into complete-mem-balancer

02d2962

wks requested changes Jan 12, 2023

View reviewed changes

src/plan/generational/gc_work.rs Outdated Show resolved Hide resolved

src/plan/generational/global.rs Outdated Show resolved Hide resolved

src/plan/generational/gc_work.rs Outdated Show resolved Hide resolved

src/plan/generational/gc_work.rs Outdated Show resolved Hide resolved

Rename GenerationalPlan::gen to common_gen. Rename Gen to CommonGenPlan.

c72eb5e

Generational work packets directly use is_current_gc_nursery() from CommonGenPlan.

wks approved these changes Jan 13, 2023

View reviewed changes

qinsoon merged commit 541020b into mmtk:master Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement mem balancer #724

Implement mem balancer #724

qinsoon commented Jan 3, 2023 •

edited

Loading

qinsoon commented Jan 4, 2023

k-sareen commented Jan 5, 2023

qinsoon commented Jan 5, 2023

wks Jan 5, 2023

qinsoon Jan 5, 2023

MarisaKirisame commented Jan 5, 2023

wks left a comment

wks Jan 10, 2023 •

edited

Loading

qinsoon Jan 12, 2023

qinsoon commented Jan 12, 2023

wks left a comment •

edited

Loading

wks left a comment

Implement mem balancer #724

Implement mem balancer #724

Conversation

qinsoon commented Jan 3, 2023 • edited Loading

qinsoon commented Jan 4, 2023

k-sareen commented Jan 5, 2023

qinsoon commented Jan 5, 2023

wks Jan 5, 2023

Choose a reason for hiding this comment

qinsoon Jan 5, 2023

Choose a reason for hiding this comment

MarisaKirisame commented Jan 5, 2023

wks left a comment

Choose a reason for hiding this comment

wks Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

qinsoon Jan 12, 2023

Choose a reason for hiding this comment

qinsoon commented Jan 12, 2023

wks left a comment • edited Loading

Choose a reason for hiding this comment

wks left a comment

Choose a reason for hiding this comment

qinsoon commented Jan 3, 2023 •

edited

Loading

wks Jan 10, 2023 •

edited

Loading

wks left a comment •

edited

Loading