RFR(M): 6407976: GC worker number should be unsigned

[Stefan Karlsson]

On 2015-04-14 21:51, Jesper Wilhelmsson wrote:
> Stefan Karlsson skrev den 14/4/15 14:05:
>>
>>
>> On 2015-04-09 14:30, Evgeniya Stepanova wrote:
>>> Hi,
>>>
>>> Please review fix for  JDK-6407976.
>>> In some source files GC worker threads numbers are being stored as
>>> signed values and same values in other classes are being stored as 
>>> unsigned
>>> values. Fix changed GC worker thread number to be uint.
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-6407976
>>> webrev: http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/
>>
>> http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.patch 
>>
>>
>>   void G1CollectedHeap::reset_taskqueue_stats() {
>> -  const int n = workers()->total_workers();
>> -  for (int i = 0; i < n; ++i) {
>> +  const uint n = workers()->total_workers();
>> +  for (int i = 0; i < (int) n; ++i) {
>>       task_queue(i)->stats.reset();
>>     }
>>   }
>>
>> Why do you have this (int) cast here? Didn't you change the 
>> task_queue function
>> with:
>> -  RefToScanQueue *task_queue(int i) const;
>> +  RefToScanQueue *task_queue(uint i) const;
>>
>
> That (int) does not exist in the diffs or frame view of the webrev, 
> only in the patch. In the rest of the webrev 'i' in the loop is uint.
>
> Jane, please make sure your webrev contains the latest version of your 
> change. For some reason old versions of some files seems to be mixed 
> with new ones in this webrev.

I wonder if this is yet another case of this bug:
https://bugs.openjdk.java.net/browse/CODETOOLS-7901115 - webrev -r 
generates broken patch files

>
>>
>> http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/src/share/vm/gc_implementation/shared/adaptiveSizePolicy.cpp.patch 
>>
>>
>> You should keep the parameter alignments:
>>
>> -int AdaptiveSizePolicy::calc_active_workers(uintx total_workers,
>> +uint AdaptiveSizePolicy::calc_active_workers(uintx total_workers,
>>                                               uintx active_workers,
>>                                               uintx 
>> application_workers) {
>>
>> -int AdaptiveSizePolicy::calc_active_conc_workers(uintx total_workers,
>> +uint AdaptiveSizePolicy::calc_active_conc_workers(uintx total_workers,
>>                                                    uintx active_workers,
>>                                                    uintx 
>> application_workers) {
>>
>>
>> This goes for the other files as well.
>>
>> http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/src/share/vm/runtime/vm_version.cpp.udiff.html 
>>
>>
>> As Igor mentioned, it would be good to only use uint:
>>
>> -int Abstract_VM_Version::_parallel_worker_threads = 0;
>> +unsigned int Abstract_VM_Version::_parallel_worker_threads = 0;
>
> Looking at the rest of vm_version.cpp we can see that unsigned int is 
> consistently used throughout the file. I don't know why, but maybe 
> there is a reason. We should probably check with runtime before 
> introducing uint. In any case I don't think this change should 
> introduce uint in just a single place. If we want this file to use 
> uint that should be changed throughout the file in a separate change.

OK. Let's use unsigned int then.

Thanks,
StefanK

>
> Thanks,
> /Jesper
>
>>
>> Thanks,
>> StefanK
>>
>>>
>>> tested by running all hotspot tests on all platforms
>>>
>>>
>>> Thanks,
>>> Jane
>>>
>>