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

[David Holmes]

Hi Thomas,

On 29/02/2016 6:26 PM, Thomas Stüfe wrote:
> Hi David,
>
> 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.

> See comments inline.

One follow up below ...

> On Mon, Feb 29, 2016 at 6:55 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi Thomas,
>
>
>
>     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.

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
>
>