RFR (M/L): JDK-8035400: Move G1ParScanThreadState into its own files
thomas.schatzl at oracle.com
Tue May 27 10:39:21 UTC 2014
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
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
> > - 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
> #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
I also rebased the change on the current hotspot jdk9 gc repo.
Diff webrev at
Complete webrev at
More information about the hotspot-gc-dev