RFR: 8080106: Refactor setup of parallel GC threads

[Stefan Karlsson]

On 2015-05-20 19:23, Kim Barrett wrote:
> On May 20, 2015, at 7:50 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>> Hi all,
>>
>> Here are the fixes I made after the code review comments from Jon and Kim:
>>
>> http://cr.openjdk.java.net/~stefank/8080110/webrev.01.delta/
>> http://cr.openjdk.java.net/~stefank/8080110/webrev.01/
> Looks good.
>
>> http://cr.openjdk.java.net/~stefank/8080112/webrev.01.delta/
>> http://cr.openjdk.java.net/~stefank/8080112/webrev.01/
> ------------------------------------------------------------------------------
> src/share/vm/gc/cms/parCardTableModRefBS.cpp
>    44   assert(n_threads <= (uint)ParallelGCThreads,
>
> Why cast ParallelGCThreads?  Its type is uintx; I don't see a need for
> a cast here.  Oh, I see.  n_threads used to be int and
> ParallelGCThreads was being cast to int to avoid signed/unsigned
> comparison warnings.  With n_threads being changed to uint, the cast
> was updated accordingly, rather than being eliminated as no longer
> needed.
>
> Sorry about missing this in the earlier review pass.

I'll remove the cast.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/cms/parCardTableModRefBS.cpp
>    45          err_msg("Error: n_threads: %u > ParallelGCThreads: %u", n_threads, (uint)ParallelGCThreads));
>
> Why the "Error:" prefix in the message; seems rather redundent, and
> not typical usage.  (I only found 9 similar occurrances of "Error:" in
> assert messages in hotspot).  I noticed the immediately preceding
> assert is one of them...

That was the reason why I added it.

>
> Also, I'd prefer use of UINTX_FORMAT, rather than casting
> ParallelGCThreads.

Fine. It's extremely annoying that ParallelGCThreads is defined as a 
uintx (64 bits on 64-bit platforms) when we use it as an uint, which can 
be 32 bits wide.

$ grep -r "int) *ParallelGCThreads" src
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp: 
(int) ParallelGCThreads,             // mt processing degree
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp: 
assert(thr_num < (int)ParallelGCThreads, "thr_num is out of bounds");
src/share/vm/gc_implementation/g1/collectionSetChooser.cpp:  uint 
n_threads = (uint) ParallelGCThreads;
src/share/vm/gc_implementation/g1/concurrentMark.cpp: 
_max_worker_id(MAX2((uint)ParallelGCThreads, 1U)),
src/share/vm/gc_implementation/g1/concurrentMark.cpp:    uint 
marking_thread_num = scale_parallel_threads((uint) ParallelGCThreads);
src/share/vm/gc_implementation/g1/concurrentMark.cpp: active_workers = 
(uint) ParallelGCThreads;
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp:  int n_queues = 
MAX2((int)ParallelGCThreads, 1);
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp: (int) 
ParallelGCThreads,
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp: 
MAX2((int)ParallelGCThreads, 1),
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp: 
MAX2((int)ParallelGCThreads, 1),
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp:  int n_queues = 
MAX2((int)ParallelGCThreads, 1);
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp: uint n_queues = 
MAX2((int)ParallelGCThreads, 1);
src/share/vm/gc_implementation/g1/g1OopClosures.cpp: assert(_worker_id < 
MAX2((uint)ParallelGCThreads, 1u),
src/share/vm/gc_implementation/g1/g1OopClosures.cpp: err_msg("The given 
worker id %u must be less than the number of threads %u", _worker_id, 
MAX2((uint)ParallelGCThreads, 1u)));
src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp:  return 
MAX2(DirtyCardQueueSet::num_par_ids() + 
ConcurrentG1Refine::thread_num(), (uint)ParallelGCThreads);
src/share/vm/gc_implementation/parallelScavenge/psCompactionManager.cpp: 
assert(index >= 0 && index < (int)ParallelGCThreads, "index out of range");
src/share/vm/gc_implementation/parallelScavenge/psCompactionManager.hpp: 
assert(index >= 0 && index <= (int)ParallelGCThreads,
src/share/vm/gc_implementation/parallelScavenge/psParallelCompact.cpp: 
(int) ParallelGCThreads, // mt processing degree
src/share/vm/gc_implementation/parallelScavenge/psParallelCompact.cpp: 
(int) ParallelGCThreads, // mt discovery degree
src/share/vm/gc_implementation/parallelScavenge/psPromotionManager.cpp: 
assert(index >= 0 && index < (int)ParallelGCThreads, "index out of range");
src/share/vm/gc_implementation/parallelScavenge/psPromotionManager.inline.hpp: 
assert(index >= 0 && index <= (int)ParallelGCThreads, "out of range 
manager_array access");
src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp: (int) 
ParallelGCThreads,    // mt processing degree
src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp: (int) 
ParallelGCThreads,    // mt discovery degree
src/share/vm/gc_implementation/parNew/parCardTableModRefBS.cpp: 
n_threads <= (int)ParallelGCThreads,
src/share/vm/gc_implementation/parNew/parCardTableModRefBS.cpp: 
n_threads == (int)ParallelGCThreads,
src/share/vm/gc_implementation/parNew/parNewGeneration.cpp: (int) 
ParallelGCThreads,    // mt processing degree
src/share/vm/gc_implementation/parNew/parNewGeneration.cpp: (int) 
ParallelGCThreads,    // mt discovery degreee

>
>> http://cr.openjdk.java.net/~stefank/8080113/webrev.01.delta/
>> http://cr.openjdk.java.net/~stefank/8080113/webrev.01/
> Looks good.

Thanks,
StefanK

>
>> Entire patch:
>> http://cr.openjdk.java.net/~stefank/8080106/webrev.01/
>>
>> Thanks,
>