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

Tue May 27 10:39:25 UTC 2014

Hi Bengt,

  thanks for the review.

On Wed, 2014-04-30 at 13:35 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> Over all this looks good to me too.
> 
> One question for g1ParScanThreadState.cpp. You have marked the 
> deal_with_refrence() methods as "inline" even though they are in the 
> same cpp file. Does that have any effect?

I moved them to the inline files.

> 
>   394
>   395 template <class T> inline void 
> G1ParScanThreadState::deal_with_reference(T* ref_to_scan) {
>   396   if (!has_partial_array_mask(ref_to_scan)) {
>   397     // Note: we can use "raw" versions of "region_containing" because
>   398     // "obj_to_scan" is definitely in the heap, and is not in a
>   399     // humongous region.
>   400     HeapRegion* r = _g1h->heap_region_containing_raw(ref_to_scan);
>   401     do_oop_evac(ref_to_scan, r);
>   402   } else {
>   403     do_oop_partial_array((oop*)ref_to_scan);
>   404   }
>   405 }
>   406
>   407 inline void G1ParScanThreadState::deal_with_reference(StarTask ref) {
>   408   assert(verify_task(ref), "sanity");
>   409   if (ref.is_narrow()) {
>   410     deal_with_reference((narrowOop*)ref);
>   411   } else {
>   412     deal_with_reference((oop*)ref);
>   413   }
>   414 }
> 
> Also, I think that you have to declare methods that should be inlined 
> before the place where they are being used on some platforms (Solaris). 
> In this case I think it means that they should be declared before 
> steal_and_trim_queue().

Moved them to the inline file.

> 
> Personally I also find the new deal_with_reference(StarTask ref) a 
> little confusing. With that method and the two methods generated by 
> deal_with_reference(T* ref_to_scan) I get kind of unsure which method 
> that will be executed by a call like:
> 
>   156   StarTask stolen_task;
>   157   while (task_queues->steal(queue_num(), hash_seed(), stolen_task)) {
>   158     assert(verify_task(stolen_task), "sanity");
>   159     deal_with_reference(stolen_task);
> 
> All three deal_with_reference() methods are potential matches. I assume 
> the compiler prefers the deal_with_reference(StarTask ref) but it makes 
> me unsure when I read the code.

Changed to dispatch_reference().

> 
> One minor nit:
> 
> g1ParScanThreadState.hpp
> You have changed the indentation of private/protected/public keywords to 
> have one space indentation. That's fine as I think that is the standard, 
> but since the whole file used no space indentation I would also have 
> been fine with leaving that. However now the last "public" keyword is 
> still having no space before it. Can you indent that too?
> 
> 218 public:

Fixed. Also removed superfluous newlines at the end of files.

Also re-checked again for performance regressions, none found.

Diff to last revision
http://cr.openjdk.java.net/~tschatzl/8035401/webrev.1_to_2/

Full diff:
http://cr.openjdk.java.net/~tschatzl/8035401/webrev.2/

(based on 8035400)

Thanks,
Thomas

> 
> Thanks,
> Bengt
> 
> 
> On 2014-04-22 23:31, Jon Masamitsu wrote:
> >
> > 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
> >>>>
> >>>>
> >>
> >
> 