Request for review: 8016302: Change type of the number of GC workers to unsigned int (2)

[Vladimir Kempik]

Hello.

Here is updated webrev

http://cr.openjdk.java.net/~vkempik/8016302/webrev.03/



Thanks, Vladimir.
On 03.10.2013 22:41, Jon Masamitsu wrote:
>
> On 10/3/13 8:37 AM, Vladimir Kempik wrote:
>> Hello
>>
>> On 03.10.2013 18:57, Jon Masamitsu wrote:
>>> http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/concurrentG1Refine.cpp.udiff.html
>>>
>>>     for (int i = _n_threads - 1; i >= 0; i--) {
>>>
>>> Did you consider using "uint i" instead of "int i"?  Maybe
>>> adding an assert
>>>
>>> assert(_n_threads > 0, ...)
>>>
>>> In order to avoid the cast
>>>
>>> +    ConcurrentG1RefineThread* t = new ConcurrentG1RefineThread(this, next, worker_id_offset, (uint)i);
>>>
>> But then   i >= 0 will always be true. The loop will look much uglier 
>> if I transform it to check against MAX_UINT
>
> OK.
>
>>
>>> http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/concurrentMark.cpp.udiff.html
>>>
>>> I used to seeing all parameters on the same line or each parameter
>>> on its own line.  Instead of
>>>
>>> +    G1NoteEndOfConcMarkClosure g1_note_end(_g1h, &local_cleanup_list,
>>>                                              &old_proxy_set,
>>>                                              &humongous_proxy_set,
>>>                                              &hrrs_cleanup_task);
>>>
>>>
>>> this style.
>>>
>>> +    G1NoteEndOfConcMarkClosure g1_note_end(_g1h,
>>>                                              &local_cleanup_list,
>>>                                              &old_proxy_set,
>>>                                              &humongous_proxy_set,
>>>                                              &hrrs_cleanup_task);
>>>
>>> I know, not really your code originally but since you touched it, I 
>>> thought I
>>> would mention it.
>>>
>>> http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/dirtyCardQueue.cpp.udiff.html
>>>
>>
>> I will update webrev to include these changes.
>>
> Thanks.
>
>>
>>> Is this cast necessary?
>>>
>>> +                                  (uint) worker_i);
>>>
>>
>> worker_i is defined as size_t in that function, it's almost same, but 
>> not always (on every platform we have), and that code is compiled 
>> with  -WError, so I've decided to make it clear for compiler and not 
>> produce any warnings.
>
> Ok.
>
> Jon
>>
>>> Rest looks good.
>>>
>>> Jon
>>>
>>>
>>>
>>> On 10/3/13 3:32 AM, Vladimir Kempik wrote:
>>>> Hello
>>>>
>>>> Can I have two reviewers to looks at it please ?
>>>>
>>>> Thanks, Vladimir.
>>>> On 26.09.2013 11:41, Per Liden wrote:
>>>>> Looks good!
>>>>>
>>>>> cheers,
>>>>> /Per
>>>>>
>>>>> On 2013-09-25 12:47, Vladimir Kempik wrote:
>>>>>> Hello
>>>>>>
>>>>>> Thanks for comments, updated webrev - 
>>>>>> http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/
>>>>>>
>>>>>>
>>>>>> On 24.09.2013 17:22, Thomas Schatzl wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Thu, 2013-09-19 at 16:22 +0200, Vladimir Kempik wrote:
>>>>>>>> Hello
>>>>>>>>
>>>>>>>> Thanks for comments, here is updated webrev -
>>>>>>>> http://cr.openjdk.java.net/~vkempik/8016302/webrev.01/
>>>>>>>>
>>>>>>> - the comments in DirtyCardQueueSet::mut_process_buffer() should be
>>>>>>> adapted that we use uints now. They talk of worker_i of -1.
>>>>>>>
>>>>>>> E.g.
>>>>>>>
>>>>>>> 130  // If worker_i is not -1 then the thread has already claimed
>>>>>>> 131  // a par_id. We make note of it using the already_claimed 
>>>>>>> value
>>>>>>>
>>>>>>> - in G1GCPhaseTimes::print_stats() we need to use UINT32_FORMAT 
>>>>>>> instead
>>>>>>> of %d as the printf() specifier for uints.
>>>>>>>
>>>>>>> - same in ScanRSClosure::printCard().
>>>>>>>
>>>>>>> Thanks,
>>>>>>>    Thomas
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131004/9a92afa1/attachment.htm>