RFR (M): JDK-8035401: Fix visibility of G1ParScanThreadState members
Bengt Rutisson
bengt.rutisson at oracle.com
Thu Jun 26 11:43:17 UTC 2014
Hi Thomas,
Looks good. One question though. In g1ParScanThreadState.hpp you have:
131 inline HeapWord* allocate(GCAllocPurpose purpose, size_t word_sz);
132 inline HeapWord* allocate_slow(GCAllocPurpose purpose, size_t
word_sz);
133 inline void undo_allocation(GCAllocPurpose purpose, HeapWord*
obj, size_t word_sz);
But the methods are implemented in g1ParScanThreadState.cpp. Shouldn't
the implementation be placed in g1ParScanThreadState.inline.hpp?
Thanks,
Bengt
On 2014-05-27 12:39, Thomas Schatzl wrote:
> 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
>>>>>>
>>>>>>
>
More information about the hotspot-gc-dev
mailing list