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

[Jon Masamitsu]

Thomas,

Thank you for the review.  I've incorporated your suggestions.

Delta
http://cr.openjdk.java.net/~jmasa/8157240/webrev_delta_1_2/

Full
http://cr.openjdk.java.net/~jmasa/8157240/webrev.02/

Jon

On 7/7/2016 2:20 PM, Thomas Schatzl wrote:
> 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
>