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

[Thomas Stüfe]

Hi David,

On Mon, Feb 29, 2016 at 9:39 AM, David Holmes <david.holmes at oracle.com>
wrote:

> 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.
>
>
Thank you!


> 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.
>
>
I do not understand what you mean with "*thread".

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?

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