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

[Claes Redestad]

FWIW, I like it.

However, the previous_created_workers is no longer printed when adding 
threads. While this could be deduced from looking at the previous log 
entry, it seems reasonable to print it when adding threads, no?

Thanks!

/Claes

On 2016-07-02 01:23, 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/
>
> 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
>>
>