RFR(xs): 8150619: Improve thread based logging introduced with 8149036

[David Holmes]

On 29/02/2016 11:19 PM, Thomas Stüfe wrote:
> On Mon, Feb 29, 2016 at 9:39 AM, David Holmes <david.holmes at oracle.com
>     On 29/02/2016 6:26 PM, Thomas Stüfe wrote:
>
>         New Version:
>         http://cr.openjdk.java.net/~stuefe/webrevs/8150619-addon-to-thread-logging/webrev.01/webrev/index.html
>
>
>     Looks fine - thanks. If there are no more comments/changes I'll
>     sponsor this in the morning.

It is on its way ... more below ...

>
> Thank you!
>
>         See comments inline.
>
>
>     One follow up below ...
>
>         On Mon, Feb 29, 2016 at 6:55 AM, David Holmes wrote
>
>              General: ensure copyright years are updated - somehow some got
>              missed in the push of 8149036.
>
>
>         I fixed all copyright years in the file headers. It would be nice if
>         jcheck would check this automatically.
>
>              ---
>
>              os_solaris.cpp
>
>              The indent is still not right here:
>
>              893 static char* describe_thr_create_attributes(char* buf,
>         size_t
>              buflen,
>                894                                  size_t stacksize,
>         long flags) {
>
>              size_t should line up with the 'c' in (char*
>
>              Ditto for os_windows.cpp
>
>
>         Okay, I fixed that. Sorry, I did not understand the indentation
>         rules
>         before.
>
>              ---
>
>              test/runtime/logging/ThreadLoggingTest.java
>
>                2  * Copyright (c) 2015, 2016, Oracle and/or its
>         affiliates. All
>              rights reserved.
>
>              This is new test so only a single copyright year should be
>         present.
>
>
>         Fixed.
>
>              You have this:
>
>                 54         output.shouldContain("stack guard pages");
>                 55         output.shouldContain("stack guard pages");
>
>              but I think the intent was:
>
>                      output.shouldContain("stack guard pages activated");
>                      output.shouldContain("stack guard pages removed");
>
>
>         Changed to one line of "output.shouldContain("stack guard pages");".
>         This matches both cases.
>
>                68         pb =
>              ProcessTools.createJavaProcessBuilder("-Xlog:thread*",
>         "-version");
>
>              Okay I'm confused - I thought the whole point of using
>         os+thread
>              here was that you had to enable os _and_ thread level
>         logging ???
>              Else why bother using the two? I would not have expected
>         the above
>              to work! :(
>
>
>         No, the logging line is just tagged with both keywords. As far as I
>         understand multiple tags in UL there is no way to specify conditions
>         like "&&" or "||" - you just tag them - and specifying rules
>         happens on
>         the command line with "os+thread" or "os,thread". So all of
>         this: "os*",
>         "thread*", "os+thread" should produce an output, the last one
>         will stop
>         producing output as soon as someone adds a third tag to the
>         logging calls.
>
>
>     I thought "os,thread" meant os _and_ thread not os _or_ thread.
>     Having "thread*" enable this particular logging is very unintuitive
>     to me - though "*thread" might make some sense.
>
>
> I do not understand what you mean with "*thread".

I was considering the ordering of tags to be significant - which as 
Markus points out it is not. So I did not expect "thread*" to match 
"os,thread" - but I would have considered "*thread" a match :)

> When marking these log lines with "os" and "thread", I may either
>
> - enable "os*" and see all these log lines and anything else marked with
> "os" - aka "anything os-related"
> - enable "thread*" and see all these log lines and anything else marked
> with "thread" - aka "anything thread related"
> - enable "os+thread" and see only these log lines - aka "anything thread
> related at os level"
 >
> Not sure why you find this unintuitive?

I was considering "os,thread" to form a "thread" namespace within the 
"os" namespace and so to see "os,thread" you had to first enable the 
"os" part.

Cheers,
David

> Kind Regards, Thomas
>
>     Thanks,
>     David
>     -----
>
>
>         Note that I added the "thread" keyword because Marcus requested
>         this:
>         http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-February/018154.html
>
>         But I simplified the test to test once for "os+thread" instead
>         of each
>         tag separately. Note that with this change, the test will have to be
>         modified if someone adds a third tag.
>
>
>              FYI the test passed on our JPRT system.
>
>
>         Great, thanks.
>
>         Thank you,
>
>         Thomas
>
>              Thanks,
>              David
>
>
>                  Thanks & Kind Regards, Thomas
>
>
>