RFR: 8255765: Shenandoah: Isolate concurrent, degenerated and full GC

[Aleksey Shipilev]

On Wed, 6 Jan 2021 16:45:03 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

> The purpose of this patch is to isolate concurrent, degenerated and full gc implementation, so that, makes each GC implementation more straightforward and clean, and improves readability.
> 
> Current implementation emphasis code sharing, e.g. degenerated GC reuses concurrent GC's ops. It was not a problem in the beginning, when they actually behave similarly. Since concurrent GC moved root processing into concurrent phases, code started to diverge, we started to put bandages to make the shared ops work for both concurrent and degenerated GC, that made code hard to read and error prone.
> 
> The patch breaks up GCs into 3 (mainly just concurrent and degenerated GC, as full GC already standalone) easy to identify and understand classes (ShenandoahConcurrentGC, ShenandoahDegeneratedGC and ShenandoahMarkCompactGC), subclasses of ShenandoahGC class.
> 
> The three GCs still keep vmop/entry/op paradigm, but encapsulate GC control flow inside their own classes, as ShenandoahMarkCompact GC already does. So that, concurrent and degenerated GC no longer share ops and op implementations no longer need to consider other GC modes, which results in simplifying implementation and improving readability. Code sharing is achieved via helper methods provided by ShenandoahHeap.
> 
> Test:
>  - [x] hotspot_gc_shenandoah
>  - [x] nightly tests

Some minor nits from the initial review. Please make sure `tier1` and `tier2` with Shenandoah still pass.

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.hpp line 53:

> 51:   ShenandoahDegenPoint degen_point() const;
> 52: 
> 53:   // Cancel on going concurrent GC

"ongoing"?

src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp line 28:

> 26: 
> 27: #include "gc/shared/collectorCounters.hpp"
> 28: 

Do we really need these newlines?

src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp line 44:

> 42: #include "gc/shenandoah/shenandoahWorkerPolicy.hpp"
> 43: #include "gc/shenandoah/shenandoahVMOperations.hpp"
> 44: 

...and this newline?

src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.hpp line 54:

> 52:   // Prepare STW evacuation
> 53:   void op_prepare_evacuation();
> 54:   //

Empty comment. Actually, do we even need comments here? I.e. does `ShenandoahConcurrentGC` have them?

src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.hpp line 25:

> 23:  */
> 24: 
> 25: #ifndef SHARE_GC_SHENANDOAH_SHENANDOAHDEGENDGC_HPP

The include guard should match the file name, maybe? `SHARE_GC_SHENANDOAH_SHENANDOAHDEGENERATEDGC_HPP`?

src/hotspot/share/gc/shenandoah/shenandoahGC.cpp line 28:

> 26: 
> 27: #include "gc/shared/workgroup.hpp"
> 28: 

Excess newlines?

src/hotspot/share/gc/shenandoah/shenandoahGC.hpp line 43:

> 41:  *      |                             | upgrade from degenerated GC |
> 42:  *  Full GC---------------------------v---------------------------->|
> 43:  */

This diagram is confusing to me.

  ("normal" mode) ----> Concurrent GC ----> (finish)
                             |
                             | <upgrade>                   
                             v
  ("passive" mode) ---> Degenerated GC ---> (finish)
                             |
                             | <upgrade>                   
                             v
                          Full GC --------> (finish)

src/hotspot/share/gc/shenandoah/shenandoahGC.hpp line 32:

> 30: 
> 31: /*
> 32:  * Base class of three Shenandoah GC modes

Is it a "mode", though? There is `ShenandoahGCMode`, which means a different thing. Maybe "flavor"?

src/hotspot/share/gc/shenandoah/shenandoahGC.hpp line 66:

> 64: };
> 65: 
> 66: #endif

No newline at the end of the file.

src/hotspot/share/gc/shenandoah/shenandoahGC.hpp line 45:

> 43:  */
> 44: 
> 45: class ShenandoahHeap;

Cannot see why this forward declaration is needed.

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 128:

> 126:   friend class ShenandoahConcurrentGC;
> 127:   friend class ShenandoahDegenGC;
> 128:   friend class ShenandoahMarkCompact;

At some point, it would make sense to rename `ShenandoahMarkCompact` to `ShenandoahFullGC`?

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 369:

> 367: 
> 368: public:
> 369:   void notify_gc_progress()    { _progress_last_gc.set();}

Align the closing brace with the next line?

src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.cpp line 196:

> 194: 
> 195:   if (!heap->unload_classes()) {
> 196:     _cld_roots.cld_do(&clds_cl, worker_id);

This change seems unrelated?

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

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1964