Request for Review (s) - 8157240: GC task trace logging is incomprehensible

[Thomas Schatzl]

Hi Jon,

  sorry for being a bit late...

On Fri, 2016-07-01 at 16:23 -0700, Jon Masamitsu wrote:
> Changed the tracing message to be less "debug" like.
> 
> Changed the way the holder name is taken.
> 
> Fixed indentations.
> 
> Delta webrev
> 
> http://cr.openjdk.java.net/~jmasa/8157240/webrev_delta_0_1/
> 
> Full webrev
> 
> http://cr.openjdk.java.net/~jmasa/8157240/webrev.01/

  - gcTaskThread.cpp: I would prefer if the PARGCTHREAD define were
removed and the task_name() static method used in the GCTaskThread
constructor.

E.g.

48   set_name("%s#%d", task_name(), which);

and

57   const char* GCTaskThread::task_name() { return "ParGC Thread"; }

It seems unnecessary to have a define here.

Maybe it would be even better if the code used
GCTaskManager::group_name() here instead, i.e.

set_name("%s#%d", manager->group_name(), which);

The extra task_name() method also seems unnecessary, and some name
prefix for the set of task threads seems to be better defined and
located by its "manager" (since parallel gc "Work Gang" equivalent is
GCTaskManager imo, and you use the name of the workgang for g1/CMS
group_name() implementation).

- workerManager.hpp:

  82       log_trace(gc, task)("%s %s(s) active workers %u created
workers %u",
  83         initializing_msg, holder->group_name(), active_workers,
created_workers);

the parameters are not correctly aligned under the first quotation mark
of the log message.

- I kinda agree with Claes that it would be nice to know the amount of
previously created worker threads.

- some pre-existing issues: ignore them at your convenience:

  gcTaskManager.cpp:393: the uint cast seems to be unnecessary here.

  workerManager.hpp:61: missing space between "if" and the bracket.

Otherwise looks good. Thanks for fixing this.

Thanks,
  Thomas