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

[Marcus Larsson]

On 02/29/2016 09:39 AM, David Holmes 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.
>
>> 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.

It does mean os and thread. The order of tags is irrelevant when 
combined, they're just tag sets. Using wildcard matches *at least* the 
tags requested, but will also enable messages with additional tags (in 
this case, os).

Marcus

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