request for review (m) - 7121618

[Bengt Rutisson]

Jon,

On 2011-12-20 19:34, Jon Masamitsu wrote:
> I've updated the webrev for review comments
> (thanks, Bengt).
>
> http://cr.openjdk.java.net/~jmasa/7121618/webrev.01/

Thanks for applying my comments. Looks good.

Do you have any input on the other questions?

A question regarding uint and uintx in flag definitions in globals.hpp. 
There are no flags using "only" uint. All int flags use uintx. This 
seems reasonable since we should be using the best int for the platform 
we are running on. However, now that we internally use uint for the 
number of GC workers it looks kind of strange that the flag for setting 
it is uintx. This causes some extra casting in the code. For example: 
g1\concurrentMark.cpp line 489: 
"_max_task_num(MAX2((uint)ParallelGCThreads, 1U)),". In theory I guess 
we could also get issues with a 64 bit value being cast down to a 32 bit 
value. Seems unlikely that anybody wants > 4 billion worker threads 
though...

Is it Hotspot policy to use uintx for the flags or would it be ok to 
change ParallelGCThreads to be an uint instead? That would make more 
sense to me since that is our internal representation. I guess that 
would propagate to other flags that set number of threads such as 
ConcGCThreads.

parCardTableModRefBS.cpp
A little bit too picky maybe, but line 59 looks like this: "uint 
n_strides = n_threads * ParGCStridesPerThread;". It is the only use of 
ParGCStridesPerThread. For some reason ParGCStridesPerThread is declared 
an intx in globals.hpp. Would you be ok with including a change of 
ParGCStridesPerThread to uintx in your fix? Makes me a little nervous 
that we mix signed and unsigned types even though it seems to work out here.

Thanks,
Bengt

>
> On 12/19/11 11:33, Jon Masamitsu wrote:
>> This changes the type used for the number of GC threads to unsigned 
>> int from
>> mostly size_t and changes corresponding variables that take the 
>> number of GC
>> threads also to unsigned int.  Some variables names were also changed in
>> the process (principally "int i" in the work() method where "i" 
>> refers to
>> the thread to "uint worker_id").
>>
>> There are many changes but all are straightforward type or
>> name changes.
>>
>> 7121618: Change type of number of GC workers to unsigned int.
>>
>> http://cr.openjdk.java.net/~jmasa/7121618/webrev.00/
>>
>> Thanks.