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

[Jon Masamitsu]

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/

Thanks.

Jon

On 6/30/2016 10:42 AM, Jon Masamitsu wrote:
> Thomas,
>
> Thanks for looking at this.
>
> On 06/30/2016 01:40 AM, Thomas Schatzl wrote:
>> Hi Jon,
>>
>> On Fri, 2016-06-24 at 19:33 -0700, Jon Masamitsu wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8157240
>>>
>>> Fix the logging for GC worker creation so that it is consistent and
>>> adds the GC worker name to the logging.
>>>
>>> http://cr.openjdk.java.net/~jmasa/8157240/webrev.00/index.html
>>>
>>> Tested by hand to verify output and with
>>> TestDynamicNumberOfGCThreads.java.
>>   - How is it possible that the worker_name() methods return NULL? Is
>> this supposed to help for threads not created yet?
>
> Yes, if the thread has not been created yet, NULL is returned.
>
>>
>> If that were the case, the logging code would crash too.
>>
>> Wouldn't that be an error in any case, and as such, I would prefer some
>> assert in the worker_name() methods for invalid index (or something
>> like this).
>
> I would have preferred to work with just the generic name of the
> thread such as "G1 Marker" but name() for a NamedThread always
> appended the thread number (such as #0) to the name of the thread.
> Using the field from NamedThread avoided the difference between
> AbstactWorkGang and GCTaskManager  but I'll figure out how to dig
> out the thread name and use that instead.
>
>>
>> - The indentation of the parameters for
>>
>> +  static void log_worker_creation(WorkerType* holder,
>> +                           uint previous_created_workers,
>> +                           uint active_workers,
>> +                           uint created_workers,
>> +                           bool initializing) {
>>
>> in workerManager.hpp is wrong
>
> Will fix.
>
>>
>> - the output seems to really be some kind of developer oriented debug
>> output, mentioning method names and variable names. Maybe something
>> more readable would be nice, but then again it is some trace level
>> message.
>
> I'll work on a better format and get back with it.
>
>>
>> No particular opinion here, but maybe somebody else has a similar
>> opinion.
>>
>> - would it be possible to write a short regression test checking the
>> output?
>
> I'll add some tests.
>
> Jon
>
>>
>> Thanks,
>>    Thomas
>