Request for review: 8016302: Change type of the number of GC workers to unsigned int (2)

[Vladimir Kempik]

Hello

Thanks for comments, here is updated webrev - 
http://cr.openjdk.java.net/~vkempik/8016302/webrev.01/

Vladimir.
On 19.09.2013 14:30, Per Liden wrote:
> Hi Vladimir,
>
> Some comments:
>
>> --- old/src/share/vm/gc_implementation/g1/concurrentMark.cpp    Wed 
>> Sep 18 19:01:08 2013
>> +++ new/src/share/vm/gc_implementation/g1/concurrentMark.cpp Wed Sep 
>> 18 19:01:06 2013
>> @@ -1803,7 +1803,7 @@
>>
>>  class G1NoteEndOfConcMarkClosure : public HeapRegionClosure {
>>    G1CollectedHeap* _g1;
>> -  int _worker_num;
>> +  uint _worker_num;
>>    size_t _max_live_bytes;
>>    uint _regions_claimed;
>>    size_t _freed_bytes;
>
> It looks like _worker_num can be removed as it's never actually used.
>
>> --- old/src/share/vm/gc_implementation/g1/dirtyCardQueue.cpp    Wed 
>> Sep 18 19:01:11 2013
>> +++ new/src/share/vm/gc_implementation/g1/dirtyCardQueue.cpp Wed Sep 
>> 18 19:01:09 2013
>> @@ -125,11 +125,11 @@
>>
>>    // We get the the number of any par_id that this thread
>>    // might have already claimed.
>> -  int worker_i = thread->get_claimed_par_id();
>> +  uint worker_i = thread->get_claimed_par_id();
>>
>>    // If worker_i is not -1 then the thread has already claimed
>>    // a par_id. We make note of it using the already_claimed value
>> -  if (worker_i != -1) {
>> +  if ((int)worker_i != -1) {
>>      already_claimed = true;
>>    } else {
>>
>> @@ -141,7 +141,7 @@
>>    }
>>
>>    bool b = false;
>> -  if (worker_i != -1) {
>> +  if ((int)worker_i != -1) {
>>      b = DirtyCardQueue::apply_closure_to_buffer(_closure, buf, 0,
>>                                                  _sz, true, worker_i);
>>      if (b) Atomic::inc(&_processed_buffers_mut);
>
> Casting to int and comparing with -1 looks a bit ugly. Maybe it's 
> worth making Thread::_claimed_par_id an uint and the functions to 
> operate on that also take uint. Then use UINT_MAX for the special case.
>
>> --- old/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp    Wed 
>> Sep 18 19:01:17 2013
>> +++ new/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Wed Sep 
>> 18 19:01:14 2013
>> @@ -1212,9 +1212,9 @@
>>  class RebuildRSOutOfRegionClosure: public HeapRegionClosure {
>>    G1CollectedHeap*   _g1h;
>>    UpdateRSOopClosure _cl;
>> -  int                _worker_i;
>> +  uint                _worker_i;
>>  public:
>
> Indentation.
>
>> @@ -122,8 +122,8 @@
>>              // RSet updating while within an evacuation pause.
>>              // In this case worker_i should be the id of a GC worker 
>> thread
>>              assert(SafepointSynchronize::is_at_safepoint(), "Should 
>> be at a safepoint");
>> -            assert(worker_i < (int) (ParallelGCThreads == 0 ? 1 : 
>> ParallelGCThreads),
>> -                   err_msg("incorrect worker id: "INT32_FORMAT, 
>> worker_i));
>> +            assert((int)worker_i < (int) (ParallelGCThreads == 0 ? 1 
>> : ParallelGCThreads),
>> +                   err_msg("incorrect worker id: "UINT32_FORMAT, 
>> worker_i));
>
> Looks like both (int) casts can be removed.
>
>> --- old/src/share/vm/gc_implementation/g1/g1RemSet.cpp    Wed Sep 18 
>> 19:01:37 2013
>> +++ new/src/share/vm/gc_implementation/g1/g1RemSet.cpp    Wed Sep 18 
>> 19:01:35 2013
>> @@ -112,7 +112,7 @@
>>    CardTableModRefBS *_ct_bs;
>>
>>    double _strong_code_root_scan_time_sec;
>> -  int    _worker_i;
>> +  uint    _worker_i;
>>    int    _block_size;
>>    bool   _try_claimed;
>
> Indentation.
>
>>      // is during RSet updating within an evacuation pause.
>>      // In this case worker_i should be the id of a GC worker thread.
>>      assert(SafepointSynchronize::is_at_safepoint(), "not during an 
>> evacuation pause");
>> -    assert(worker_i < (int) (ParallelGCThreads == 0 ? 1 : 
>> ParallelGCThreads), "should be a GC worker");
>> +    assert((int)worker_i < (int) (ParallelGCThreads == 0 ? 1 : 
>> ParallelGCThreads), "should be a GC worker");
>
> Again looks like both int casts can be removed.
>
>>    // We cache the value of 'oc' closure into the appropriate slot in 
>> the
>>    // _cset_rs_update_cl for this worker
>> -  assert(worker_i < (int)n_workers(), "sanity");
>> +  assert((int)worker_i < (int)n_workers(), "sanity");
>>    _cset_rs_update_cl[worker_i] = oc;
>
> Same here.
>
>
> cheers,
> /Per
>
>
> On 09/19/2013 12:00 PM, Vladimir Kempik wrote:
>> Hi all,
>>
>> Could I have a couple of reviews for this change?
>>
>> http://cr.openjdk.java.net/~vkempik/8016302/webrev.00/
>>
>> In 7121618 variables representing GC workers (worker id, worker id
>> offset) have been changed from int to unsigned int.
>>
>> Since then, code reintroduced the use of int's for this type of
>> variable; fixing this by aligning the code to use uints for ints.
>>
>> Thanks,
>> Vladimir
>>
>