RFR (M): JDK-8035401: Fix visibility of G1ParScanThreadState members

[Jon Masamitsu]

On 4/22/14 1:36 PM, Thomas Schatzl wrote:
> Hi Jon,
>
> On Tue, 2014-04-22 at 13:07 -0700, Jon Masamitsu wrote:
>> Thomas,
>>
>> What I see in these changes are
>>
>> 1) no semantic changes
> No.
>
>> 2) some methods in .hpp files moved to .cpp files
> Yes, because they were only referenced by the cpp file, so I thought it
> would be good to move them there. They will be inlined as needed anyway
> (and I think for some of them they were never inlined due to their
> size).
>
> I will do some more runs with the inline's added again.
>
>> 3) creation of steal_and_trim_queue() with definition in
>> a .cpp file (I may have missed additional such new
>> methods)
> There are none except queue_is_empty(), see below.
>
>> 4) change in visibility as the CR says
> That's the main change.
>
>> 5) no performance regressions as stated in your RFR
> No. Checked the results for the usual benchmarks (specjvm2008,
> specjbb05/2013) again right now, and there are no significant
> differences in the scores (on x64 and sparc), and for specjbb05/2013 the
> average gc pause time, and the object copy time (assuming that this is
> the part that will be affected most) stay the same as in the baseline.
>
>> If that's what constitutes the change, looks good.
> Thanks.
>
>> Reviewed.
>>
>> If there is something more significant that I have
>> overlooked, please point me at it and I'll look again.
> There is not. Sorry, I should have pointed out the changes in more
> detail instead of you making guesses.
>
> Additional minor changes:
>
> - G1ParScanThreadState accesses members directly instead of using
> getters (e.g. _refs instead of refs()).
>
> - fixed some newlines in method declarations, removing newlines
>
> - removed refs() to avoid direct access from outside, and adding a new
> method queue_is_empty() (only used in asserts as refs()->is_empty(), and
> I did not want to expose refs() just for the asserts).

All looks good.

Reviewed.

Jon

>
> Thanks,
>    Thomas
>
>> On 4/18/14 6:29 AM, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     can I have reviews for this change? After moving G1ParScanThreadState,
>>> this change cleans up visibility, making a whole lot of stuff private.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8035401
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8035401/webrev/
>>>
>>> Testing:
>>> perf testing indicated no changes, jprt
>>>
>>> Thanks,
>>>     Thomas
>>>
>>>
>