[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR (M): JDK-8035401: Fix visibility of G1ParScanThreadState members
From:       Bengt Rutisson <bengt.rutisson () oracle ! com>
Date:       2014-04-30 11:35:58
Message-ID: 5360E01E.7020906 () oracle ! com
[Download RAW message or body]


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?

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

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.

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:

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

[prev in list] [next in list] [prev in thread] [next in thread]