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

[Vladimir Kempik]

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

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


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

> 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/20131003/dc1bb24d/attachment.htm>