request for review (m) - 7121618
bengt.rutisson at oracle.com
Tue Dec 20 23:02:32 PST 2011
Sorry, I missed this email for some reason. (Thanks for pointing me to it!)
Let's leave the flags as uintx. Thanks for fixing ParGCStridesPerThread.
Apologies again for my previous confusing email.
On 2011-12-20 16:24, Jon Masamitsu wrote:
> Thanks for reviewing this. I'll fix the other work() methods.
> My apologies for not being more diligent on that score.
> I don't know the answer to the question of using uint as a flag
> type. I know there is some machinery behind the flag types.
> For example there is a get_uintx() and no get_uint() and
> I don't know it is just a lack of an implementation for uint or
> something else. If you don't mind, I'd like to avoid stepping into
> that question with this set of changes.
> Inline comment below.
> On 12/20/11 00:04, Bengt Rutisson wrote:
>> 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:
>> 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
>> Line 758: "virtual void work(uint i);" for ParNewRefProcTaskProxy
>> Line 242: "void work(uint i);" for ParNewGenTask
>> Line 71: "virtual void work(uint i) = 0;" for AbstractGangTask
>> 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
>> 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.
> Yes I'll change ParGCStridesPerThread to uintx
> Thanks again.
>> 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.
More information about the hotspot-gc-dev