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

[Per Liden]

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
>