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

[Jesper Wilhelmsson]

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.

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

Thanks,
/Jesper

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