request for review (m) - 7121618

[Bengt Rutisson]

Hi Jon,

Thanks for cleaning this up! I like it much better!

Some details:

I think some method declarations still have to change to use "worker_id" 
instead of "i" as their parameter:

concurrentMarkSweepGeneration.cpp
Line 3782:  void work(uint i); for CMSConcMarkingTask
Line 5081:  void work(uint i); for CMSParRemarkTask.
Line 5742  "virtual void work(uint i);" for CMSRefProcTaskProxy

parNewGeneration.cpp
Line 758: "virtual void work(uint i);" for ParNewRefProcTaskProxy

parNewGeneration.hpp
Line 242: "void work(uint i);" for ParNewGenTask

workgroup.hpp
Line 71: "virtual void work(uint i) = 0;" for AbstractGangTask

yieldingWorkgroup.hpp
Line 74: "virtual void work(uint i) = 0;" for FlexibleGangTask
Line 131 "virtual void work(uint i) = 0;" for YieldingFlexibleGangTask


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 2011-12-19 20: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.