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

[Evgeniya Stepanova]

The webrev for the diff could be found at
http://cr.openjdk.java.net/~eistepan/6407976/webrev.01.diff/

But changes are visible only in patch files, since there are only 
white-spaces
changes.

Thanks,
Jane
On 17.04.2015 16:53, Evgeniya Stepanova wrote:
> Hi,
>
> Please look at the new webrev:
> http://cr.openjdk.java.net/~eistepan/6407976/webrev.01
>
> Thank you!
> Jane
> On 15.04.2015 11:31, Evgeniya Stepanova wrote:
>> Hi Igor, Stefan, Jesper
>>
>> Thank you very much for the reviews!
>> On 15.04.2015 10:57, Stefan Karlsson wrote:
>>> 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
>>>
>> That's really strange. I'll double check patches next time to prevent 
>> this happened again
>>>>
>>>>>
>>>>> 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.
>> Thank you, will fix
>>
>>>>>
>>>>> 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.
>>
>> I also think it should be done with the separate rfr for all 
>> sources.  So I'll keep it "as is" for now.
>>
>> Thanks once again,
>> Jane
>>>
>>> Thanks,
>>> StefanK
>>>
>>>>
>>>> Thanks,
>>>> /Jesper
>>>>
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>>>
>>>>>> tested by running all hotspot tests on all platforms
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Jane
>>>>>>
>>>>>
>>>
>>
>> -- 
>> /Evgeniya Stepanova/
>
> -- 
> /Evgeniya Stepanova/

-- 
/Evgeniya Stepanova/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150420/6a88e595/attachment.htm>