[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR (M/L): JDK-8035400: Move G1ParScanThreadState into its own files
From:       Thomas Schatzl <thomas.schatzl () oracle ! com>
Date:       2014-05-27 10:39:21
Message-ID: 1401187161.2682.13.camel () cirrus
[Download RAW message or body]

Hi Bengt,

  thanks for the review and sorry for the long delay...

On Wed, 2014-05-07 at 13:18 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> On 2014-04-18 15:52, Thomas Schatzl wrote:
> > Hi all,
> >
> >    can I have reviews for the above change? It moves G1ParScanThreadState
> > into G1ParScanThreadState*pp files.
> >
> > The only changes are limited to:
> >   - adding a "#pragma warning( disable:4355 ) // 'this' : used in base
> > member initializer list" to shut visual C up about the problem (which
> > should be cleaned up at some point - I found an issue that slipped
> > through because of that, JDK-8040977)
> 
> As I commented in the review of JDK-8040977 I would prefer to make the 
> change to not pass this as a parameter to the constructor. That would 
> also remove the need for disabling the warning. Maybe in that case base 
> this review on top of the fix for JDK-8040977 rather than the other way 
> around?

I do not see an advantage either way. Since this would require me make
significant changes to all patches, I would prefer keeping the order
this way if you do not mind.

In the latest JDK-8040977 I removed the need for the pragma as
requested.

> >   - added necessary include file references; I hope the AIX guys can
> > compile that change to avoid troubles. It compiles fine with all Oracle
> > supported archs.
> 
> You also moved the definition of the destructor of G1ParScanThreadState 
> from the hpp file to the cpp file. Makes sense, but was not strictly 
> needed for this change, right?

Fixed that. This has been an oversight when separating out the changes.

> > There will be another CR for fixing up visibility and cleaning up stuff
> > a little.
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8035400
> >
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8035400/webrev/
> 
> It is a bit hard to review moved code. But except for the comment 
> regarding JDK-8040977 above I think it looks good.
> 
> I think you can clean up the includes a bit more if you have time. Seems 
> like these includes in g1CollectedHeap.cpp are for example not needed 
> anymore:
> 
> #include "oops/oop.inline.hpp"
> #include "oops/oop.pcgc.inline.hpp"

I tried to clean up the includes a little more. However you cannot move
these particular includes because they are still needed for evacuation
failure handling.

I also rebased the change on the current hotspot jdk9 gc repo.

Diff webrev at
http://cr.openjdk.java.net/~tschatzl/8035400/webrev.1_to_2/

Complete webrev at
http://cr.openjdk.java.net/~tschatzl/8035400/webrev.2/

Thanks,
  Thomas

[prev in list] [next in list] [prev in thread] [next in thread]