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

Thomas Schatzl thomas.schatzl at oracle.com
Thu Mar 29 08:33:21 UTC 2018

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, gcareyoung and > > duringim 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.0to1 (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 initiateconcmarkifpossible() is set at the beginning of a - initiateconcmarkifpossible instead of initiateconcmarkifpossible() as other cases are using a variable name.

Fixed.

======================= src/hotspot/share/gc/g1/g1ConcurrentMark.cpp 1899 if (!G1CollectedHeap::heap()->collectorstate()- >markorrebuildinprogress()) { - 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

More information about the hotspot-gc-dev mailing list