RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

[sangheon.kim]

Hi Thomas,

On 11/03/2017 09:28 AM, Thomas Schatzl wrote:
> Hi,
>
>    can I have reviews for this change that makes refinement threads
> behave the same as the other thread groups we have in G1?
>
> I.e. with UseDynamicNumberOfGCThreads enabled (which is off by default)
> they are created lazily.
>
> This helps a little bit further with startup/footprint benchmarks (if
> enabled).
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8190426
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8190426/webrev/
> Testing:
> hs-tier1+2
I like this idea, thank you for improving this.

I have some comments.
------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp
37   _cg1r(NULL),
- Probably _cr or _g1cr.

54   assert(cg1r != NULL, "Passed g1ConcurrentRefine must not be NULL");
- G1ConcurrentRefine not g1ConcurrentRefine.

57   _threads = NEW_C_HEAP_ARRAY(G1ConcurrentRefineThread*, 
num_max_threads, mtGC);
58   for (uint i = 0; i < num_max_threads; i++) {
- Don't we need NULL check at line 58?

67 void G1ConcurrentRefineThreadControl::maybe_activate_next(uint 
cur_worker_id) {
- without 'next' seems better or 'maybe_activate_thread()'.

68   assert(cur_worker_id < _num_max_threads, "Tried to activate from 
impossible thread %u", cur_worker_
- I would prefer more detailed logging rather than 'impossible'. e.g. 
Activating current worker id exceeds maximum number of threads.

78     _threads[worker_id] = new G1ConcurrentRefineThread(_cg1r, worker_id);
79     thread_to_activate = _threads[worker_id];
- NULL check at line 79?
- If we fail to allocate and G1ConcRefinementThreads is set, printing 
warning message would needed.

81   thread_to_activate->activate();
- Probably NULL check is needed for 'thread_to_activate'.

198   _thread_control.initialize(this, max_num_threads());
- G1ConcurrentRefine::create() seems proper location to treat allocation 
failure.

389 void G1ConcurrentRefine::maybe_activate_more_threads(uint worker_id, 
size_t num_cur_buffers) {
- maybe_activate_more_thread() as this will activate only 1 thread.

------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentRefineThread.hpp
62   G1ConcurrentRefineThread(G1ConcurrentRefine* cg1r, uint worker_id);
- cg1r -> cr or g1cr?

Thanks,
Sangheon


> Thanks,
>    Thomas