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

[Evgeniya Stepanova]

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