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

[Evgeniya Stepanova]

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/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150417/d48b1a09/attachment.htm>