RFR: 8143041: Unify G1CollectorPolicy::PauseKind and G1YCType

[Thomas Schatzl]

On Mon, 8 Mar 2021 10:26:59 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Please review this refactoring change to unify G1CollectorPolicy::PauseKind and G1YCType enums under the same file. Additionally, make the distinction between collector types and phases clearer.

Looks good otherwise.

src/hotspot/share/gc/g1/g1GCTypes.hpp line 30:

> 28: #include "utilities/debug.hpp"
> 29: 
> 30: enum G1YCPhase {

I kind of think a better name would be `G1YoungPhase` - the `C` in that name is an abbreviation for "collection", and to me, `YCPhase` indicates the phases in the collection which this is not.
Better names are certainly possible.
Please also add some small documentation, like "The phases the collection cycle can be in".

src/hotspot/share/gc/g1/g1GCTypes.hpp line 38:

> 36: };
> 37: 
> 38: enum G1GCType {

Not sure if it might be better to name this `G1PauseType` since `Cleanup` and `Remark` aren't really collections. But maybe it's fine as is (or `G1GCPauseType` if you prefer).

src/hotspot/share/gc/g1/g1GCTypes.hpp line 87:

> 85:     }
> 86:   }
> 87: 

Extra newline that can be deleted.

src/hotspot/share/gc/g1/g1GCTypes.hpp line 2:

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

This should probably be `2012, 2021,` - I do not see a reason to remove the former date.

src/hotspot/share/gc/g1/g1GCTypes.hpp line 54:

> 52: 
> 53:   static bool is_young_only_pause(G1GCType type) {
> 54:     assert(type != FullGC, "must be");

These three asserts could probably be moved into a helper method as the exact same code is used in `is_mixed_pause()` too. Your call.

src/hotspot/share/gc/g1/g1GCTypes.hpp line 53:

> 51:  public:
> 52: 
> 53:   static bool is_young_only_pause(G1GCType type) {

`is_young_pause_young_only()`? Just to match the method. Feel free to ignore.

src/hotspot/share/gc/g1/g1GCTypes.hpp line 63:

> 61:   }
> 62: 
> 63:   static bool is_mixed_pause(G1GCType type) {

`is_young_pause_mixed()`? Just to match the method. Feel free to ignore.

src/hotspot/share/gc/g1/g1GCTypes.hpp line 70:

> 68:   }
> 69: 
> 70:   static bool is_last_young_pause(G1GCType type) {

`is_young_pause_last_young()`. I also think this method is missing the three asserts of the other methods (pre-existing).

src/hotspot/share/gc/g1/g1GCTypes.hpp line 74:

> 72:   }
> 73: 
> 74:   static bool is_concurrent_start_pause(G1GCType type) {

`is_young_pause_concurrent_start()`. Same as above, it should have the three asserts.

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

Changes requested by tschatzl (Reviewer).

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