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

[Bengt Rutisson]

Hi Thomas,

I think this looks good now.

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
>