RFR (M): 8199742: Clean up state flags in G1CollectorState

[Stefan Johansson]

Hi Thomas,

Thanks for this much needed cleanup :)

On 2018-03-26 17:06, Thomas Schatzl wrote:
> Hi all,
>
>    I would like to request reviews for this change that cleans up the
> flags in G1CollectorState, applying uniform naming, removing members
> that were basically temporary variables for a single method, and remove
> redundant flags (the four flags in the "XXX" block were basically a
> single one).
>
> While this is just a step towards my goal of removing the flags as much
> as possible and merging them into enums, I think this is a very
> worthwhile step on that way. Further it blocks a few more changes of
> mine :P
>
> Naming guidelines:
>    - flags indicating a particular GC were named "in_XXX_gc"
>    - durations spanning multiple garbage collections, i.e. phases have
> the "phase" postfix.
>
> Other than that there were general updates for the names to hopefully
> make them reflect reality a bit closer.
>
> Main changes apart from naming were
>
> - in g1CollectionSet.cpp where I made  add_young_region_common() to not
> update statistics. This avoids the need to make the full gc also a
> young gc (at least as far as flags are concerned); to fix this properly
> we would need to know the previous phase we were in when coming into
> the full gc. I just do not think it is worth adding a new state here
> (and the code has been wrong previously too, unconditionally adding
> these statistics to the young-only phase).
>
> I made a note in the follow-up change JDK-8080226.
>
> - the changes in G1Policy::record_collection_pause_end() were mostly
> due to removing the last_gc_was_young() member which was basically a
> reminder just for this method that the current gc was a young-only gc.
>
> There is still _initiate_conc_mark_if_possible flag that indicates that
> a state change to initial mark should occur asap, but I think that
> should be moved to e.g. g1policy in a next change.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8199742
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8199742/webrev
The change looks good. Just some minor things, gc_are_young and 
during_im are used as local variables at some places. I think we could 
change those to reflect the new better names as well.

Thanks,
Stefan

> Testing:
> hs-tier 1-5
>
> These changes are based on the rebuild remembered sets concurrently
> changes. As it may be a bit of work to get them working (I already
> pushed to first four), here is a webrev that updates latest jdk/hs to
> that level at http://cr.openjdk.java.net/~tschatzl/8199742/baseline/web
> rev/
>
> Thanks,
>    Thomas
>