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

[Thomas Schatzl]

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).

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
> >
> >
>