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