RFR (M/L): JDK-8035400: Move G1ParScanThreadState into its own files

[Mikael Gerdin]

Hi,

On Thursday 26 June 2014 13.31.08 Bengt Rutisson wrote:
> Hi Thomas,
> 
> I think this looks good now.

+1
Looks good
/Mikael

> 
> Thanks,
> Bengt
> 
> On 2014-05-27 12:39, Thomas Schatzl wrote:
> > 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