RFR: Allow young collection to suspend marking in old generation [v14]

[Roman Kennke]

On Sat, 27 Feb 2021 01:42:18 GMT, earthling-amzn <github.com+71722661+earthling-amzn at openjdk.org> wrote:

>> **This is a work in progress.**
>> 
>> ## Summary of changes
>> The goal of these changes is to allow young cycles to suspend marking in the old generation. When the young cycle is complete, the concurrent old marking will resume.
>> * Evaluation of heuristics was pulled into a new `ShenandoahRegulatorThread`. Taking this evaluation out of line from the control thread allows the regulator to suspend (using the cancellation mechanism) old generation marking and start a young generation cycle.
>> * Task queues for marking have been moved from `ShenandoahMarkingContext` into `ShenandoahGeneration`. This allows the marking state for the old generation to persist across young generation cycles. The associated `is_complete` state has also been moved to `ShenandoahGeneration`.
>> * Old generation marking is bootstrapped by a complete young generation cycle. In this scenario, the mark closures for a young cycle are given reference to the young generation mark queues _and_ the old generation mark queues. Rather than ignore old references as is done for a normal young cycle, they are enqueued in the old generation mark queues. When the young cycle completes, the old generation marking continues using the task queues primed by the preceding young cycle.
>> * There is a new flag: `ShenandoahAllowOldMarkingPreemption` (defaults to true). Disabling this option will cause the regulator to schedule  `young` or `global` collects (according to heuristics), but will _not_ schedule `old` collects.
>> * The `global` generation is used to support satb and incremental-update modes. The `global` generation is also used for degenerated and implicit/explicit gc requests. Degenerated cycles are not working on this branch and the root cause is understood.
>> * The `global` generation is also used for `FullGC`, but this is also broken. The `FullGC` doesn't update the remembered set during compaction. We reckon there is a non-trivial amount of work to fix this.
>> * The `MARKING` gc state has been split into `YOUNG_MARKING` and `OLD_MARKING`.
>> * Immediate garbage collection is broken in generational mode. The update references phase is used to repair the remembered set, so when this phase is skipped the remembered set scan runs into trouble. A fix for this is in progress.
>> * The remembered set is scanned on a safepoint during initial mark. Work to make this concurrent is in progress.
>
> earthling-amzn has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Encode active generation and old generation marking status
>  - Do not unset preemption request flag prematurely
>  - Disable reference processing generational mode

That is a huge change! Good work!
First-pass review follows.

(I need to remember to scan for 'HEY!' in addition to 'TODO' ;-) )

src/hotspot/cpu/aarch64/gc/shenandoah/c1/shenandoahBarrierSetC1_aarch64.cpp line 32:

> 30: #include "gc/shenandoah/shenandoahBarrierSetAssembler.hpp"
> 31: #include "gc/shenandoah/c1/shenandoahBarrierSetC1.hpp"
> 32: #include "gc/shenandoah/mode/shenandoahMode.hpp"

The include is unused, isn't it?

src/hotspot/cpu/aarch64/gc/shenandoah/shenandoahBarrierSetAssembler_aarch64.cpp line 64:

> 62:         __ tbz(rscratch1, ShenandoahHeap::HAS_FORWARDED_BITPOS, done);
> 63:       } else {
> 64:         __ mov(rscratch2, ShenandoahHeap::HAS_FORWARDED | ShenandoahHeap::OLD_MARKING);

In the x86 variant you preserve YOUNG_MARKING there.

src/hotspot/share/gc/shenandoah/mode/shenandoahGenerationalMode.cpp line 46:

> 44:   // HEY! Disabled while M7 work is in progress.
> 45:   FLAG_SET_ERGO(ShenandoahUnloadClassesFrequency, 0);
> 46:   FLAG_SET_ERGO(RegisterReferences, false);

This doesn't work in release build, unfortunately.

src/hotspot/share/gc/shenandoah/mode/shenandoahMode.hpp line 28:

> 26: #define SHARE_GC_SHENANDOAH_MODE_SHENANDOAHMODE_HPP
> 27: 
> 28: #include "runtime/java.hpp"

What is that include needed for?

src/hotspot/share/gc/shenandoah/shenandoahArguments.cpp line 159:

> 157:   }
> 158: 
> 159:   if (strcmp(ShenandoahGCMode, "generational") == 0) {

I guess this could be moved into ShenandoahGenerationalMode::initialize_flags()?

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 193:

> 191: }
> 192: #endif
> 193: 

This is probably better placed in a more general place? I can imagine it would be used in other places too? (Maybe in ShenandoahRegionAffiliation itself?)

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2001, 2018, Oracle and/or its affiliates. All rights reserved.

That copyright seems wrong.

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 189:

> 187:   _cancelled_gc.set(CANCELLABLE);
> 188:   if (clear_oom_handler) {
> 189:     _oom_evac_handler.clear();

This change seems a bit suspicious to me. Why do we need to clear the OOM evac handler here?

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 323:

> 321:     // we have to keep the fwdptr initialized and pointing to our (stale) copy.
> 322: 
> 323: 

Stray spacing changes here...

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp line 342:

> 340:   if (_affiliation == FREE) {
> 341:     //assert(_live_data == 0, "Setting non-zero live data (%zu) on FREE region", s);
> 342:   }

Is this commented-out assert relevant? If not, maybe remove it?

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp line 423:

> 421: }
> 422: 
> 423: void ShenandoahHeapRegion::oop_iterate_objects(OopIterateClosure* blk, bool fill_dead_objects, bool reregister_coalesced_objects) {

I wonder if this is better done as two separate methods: the normal/old oop_iterate_objects() and a new one that also fills dead objects?

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp line 524:

> 522:   printf("SHR::recycle(), setting region (%llx, %llx, %llx) to FREE\n",
> 523:          (unsigned long long) bottom(), (unsigned long long) top(), (unsigned long long) end());
> 524:   fflush(stdout);

In Hotspot, it's simpler to use tty->print_cr("...") for this, or even better use logging e.g. log_trace(gc)("...") or so.

src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.cpp line 1:

> 1: #include "precompiled.hpp"

It's lacking a copyright header here.

src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2020, Red Hat, Inc. All rights reserved.

And this copyright header has the wrong year in it :-)

src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.hpp line 35:

> 33: #include "gc/shenandoah/shenandoahMarkingContext.hpp"
> 34: #include "gc/shenandoah/shenandoahMarkClosures.hpp"
> 35: 

Instead of including everything, we can put forward declaration for any type that is only used as pointer in the header.

src/hotspot/share/gc/shenandoah/shenandoahOldGC.cpp line 1:

> 1: #include "precompiled.hpp"

Missing copyright header here.

src/hotspot/share/gc/shenandoah/shenandoahOldGC.hpp line 1:

> 1: #ifndef SHARE_GC_SHENANDOAH_SHENANDOAHOLDGC_HPP

And here too.

src/hotspot/share/gc/shenandoah/shenandoahOldGC.hpp line 7:

> 5: #include "gc/shenandoah/shenandoahConcurrentMark.hpp"
> 6: #include "gc/shenandoah/shenandoahConcurrentGC.hpp"
> 7: #include "gc/shenandoah/shenandoahHeap.hpp"

I see no use of ShenandoahHeap or ShenandoahConcurrentMark in this header. I believe those two includes could be removed.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 1:

> 1: #include "precompiled.hpp"

Missing header.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 1:

> 1: 

Missing header

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 6:

> 4: 
> 5: #include "gc/shenandoah/shenandoahGeneration.hpp"
> 6: 

You should add a forward declaration of ShenandoahHeapRegion and ShenandoahHeapRegionClosure.

src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.hpp line 103:

> 101:                                                                                        \
> 102:   f(init_update_refs_gross,                         "Pause Init Update Refs (G)")      \
> 103:   f(init_update_refs,                               "Pause Init Update Refs (N)")      \

Looks like unrelated spacing changes

src/hotspot/share/gc/shenandoah/shenandoahRegulatorThread.cpp line 2:

> 1: /*
> 2:  * Copyright (c), Amazon.com, Inc. and/or its affiliates. All rights reserved.

You should add the copyright year (2021) here.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 28:

> 26: // During development of this new feature, we want the option to test
> 27: // with and without, and to compare performance before and after.
> 28: #define FAST_REMEMBERED_SET_SCANNING

You'll still want the normal #ifndef SHARE_GC_SHENANDOAH_SHENANDOAHSCANREMEMBERED_HPP protection, don't you?

-------------

Changes requested by rkennke (Reviewer).

PR: https://git.openjdk.java.net/shenandoah/pull/19