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

[Thomas Schatzl]

Hi,

  thanks for your review :)

On Wed, 2018-03-28 at 23:27 -0700, sangheon.kim wrote:
> Hi Thomas,
> 
> On 03/27/2018 06:25 AM, Thomas Schatzl wrote:
> > Hi Stefan,
> > 
> > On Tue, 2018-03-27 at 14:58 +0200, Stefan Johansson wrote:
> > > 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 [...]
> > > > 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.
> > 
> > I grepped through the sources for the old names and changed the
> > uses to
> > something better (I hope).
> > 
> > http://cr.openjdk.java.net/~tschatzl/8199742/webrev.0_to_1 (diff)
> > http://cr.openjdk.java.net/~tschatzl/8199742/webrev.1 (full)
> 
> webrev.1 looks good to me.
> I have some minor comments.
> 
> =======================
> src/hotspot/share/gc/g1/g1CollectorState.hpp
> 41   // If initiate_conc_mark_if_possible() is set at the beginning
> of a
> - _initiate_conc_mark_if_possible instead of 
> initiate_conc_mark_if_possible() as other cases are using a variable
> name.

Fixed.

> 
> =======================
> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
> 1899   if 
> (!G1CollectedHeap::heap()->collector_state()-
> >mark_or_rebuild_in_progress()) 
> {
> - Pre-existing issue: _g1h instead of G1CollectedHeap::heap(). This 
> should be commented on JDK-8151171 but I missed. :) Probably need an 

I added this tiny change to JDK-8151171 before pushing. Nice catch.

> extra CR for G1CollectedHeap::heap() cleanup as other classes also
> would use it even though the class has a member of G1CollectedHeap.
> e.g. g1CollectionSet.cpp:260

I know that other classes are guilty of the same issue.

Thanks,
  Thomas