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

[Jon Masamitsu]

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