Request for review: 8016302: Change type of the number of GC workers to unsigned int (2)
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
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
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
Hi, On Thu, 2013-09-19 at 16:22 +0200, Vladimir Kempik wrote:
Hello
Thanks for comments, here is updated webrev - http://cr.openjdk.java.net/~vkempik/8016302/webrev.01/
- the comments in DirtyCardQueueSet::mut_process_buffer() should be adapted that we use uints now. They talk of worker_i of -1. E.g. 130 // If worker_i is not -1 then the thread has already claimed 131 // a par_id. We make note of it using the already_claimed value - in G1GCPhaseTimes::print_stats() we need to use UINT32_FORMAT instead of %d as the printf() specifier for uints. - same in ScanRSClosure::printCard(). Thanks, Thomas
Hello Thanks for comments, updated webrev - http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/ On 24.09.2013 17:22, Thomas Schatzl wrote:
Hi,
On Thu, 2013-09-19 at 16:22 +0200, Vladimir Kempik wrote:
Hello
Thanks for comments, here is updated webrev - http://cr.openjdk.java.net/~vkempik/8016302/webrev.01/
- the comments in DirtyCardQueueSet::mut_process_buffer() should be adapted that we use uints now. They talk of worker_i of -1.
E.g.
130 // If worker_i is not -1 then the thread has already claimed 131 // a par_id. We make note of it using the already_claimed value
- in G1GCPhaseTimes::print_stats() we need to use UINT32_FORMAT instead of %d as the printf() specifier for uints.
- same in ScanRSClosure::printCard().
Thanks, Thomas
Hi, On Wed, 2013-09-25 at 14:47 +0400, Vladimir Kempik wrote:
Hello
Thanks for comments, updated webrev - http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/
Looks good to me. You still need an hsx/openjdk Reviewer to have a look at this though. Thomas
Looks good! cheers, /Per On 2013-09-25 12:47, Vladimir Kempik wrote:
Hello
Thanks for comments, updated webrev - http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/
On 24.09.2013 17:22, Thomas Schatzl wrote:
Hi,
On Thu, 2013-09-19 at 16:22 +0200, Vladimir Kempik wrote:
Hello
Thanks for comments, here is updated webrev - http://cr.openjdk.java.net/~vkempik/8016302/webrev.01/
- the comments in DirtyCardQueueSet::mut_process_buffer() should be adapted that we use uints now. They talk of worker_i of -1.
E.g.
130 // If worker_i is not -1 then the thread has already claimed 131 // a par_id. We make note of it using the already_claimed value
- in G1GCPhaseTimes::print_stats() we need to use UINT32_FORMAT instead of %d as the printf() specifier for uints.
- same in ScanRSClosure::printCard().
Thanks, Thomas
Hello Can I have two reviewers to looks at it please ? Thanks, Vladimir. On 26.09.2013 11:41, Per Liden wrote:
Looks good!
cheers, /Per
On 2013-09-25 12:47, Vladimir Kempik wrote:
Hello
Thanks for comments, updated webrev - http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/
On 24.09.2013 17:22, Thomas Schatzl wrote:
Hi,
On Thu, 2013-09-19 at 16:22 +0200, Vladimir Kempik wrote:
Hello
Thanks for comments, here is updated webrev - http://cr.openjdk.java.net/~vkempik/8016302/webrev.01/
- the comments in DirtyCardQueueSet::mut_process_buffer() should be adapted that we use uints now. They talk of worker_i of -1.
E.g.
130 // If worker_i is not -1 then the thread has already claimed 131 // a par_id. We make note of it using the already_claimed value
- in G1GCPhaseTimes::print_stats() we need to use UINT32_FORMAT instead of %d as the printf() specifier for uints.
- same in ScanRSClosure::printCard().
Thanks, Thomas
Hi, On Thu, 2013-10-03 at 14:32 +0400, Vladimir Kempik wrote:
Hello
Can I have two reviewers to looks at it please ?
just fyi, you only need one more Reviewer with capital R. Per and me (I think I already okay'ed the latest changes? If not, here it is) qualify for the normal reviewer part. Thomas
participants (3)
-
Per Liden
-
Thomas Schatzl
-
Vladimir Kempik