RFR: 8141211: Convert TraceExceptions to Unified Logging

[Rachel Protacio]

Hi,
Thanks for the review. Replies inline.

Updated webrev: http://cr.openjdk.java.net/~rprotacio/8141211.01/

On 12/8/2015 10:53 PM, David Holmes wrote:
> Hi Rachel,
>
> On 9/12/2015 1:42 AM, Rachel Protacio wrote:
>> Hello,
>>
>> Please review my conversion of -XX:+TraceExceptions to
>> -Xlog:exceptions=info. The existing (product) flag is aliased to the
>> logging flag at the info level.
>
> Q: how does use of ttyLocker map into UL? I see an awful lot of 
> multi-line logging blocks that are going to be completely messed up 
> without locking!
My impression was that the logging works as-is, but I will check in with 
Marcus.
>
> src/share/vm/opto/runtime.cpp
>
> 1239     ResourceMark rm;
>
> What requires the introduction of the ResourceMark?
That's because I'm passing in a LogHandle output stream. Each of those 
require a ResourceMark.
>
> ---
>
> src/share/vm/runtime/thread.cpp
>
> 2084       if (log_is_enabled(Info, exceptions)) {
> 2085          ResourceMark rm;
>
> An extra leading space has crept in at +2085
>
> 2087         logstream->print("Async. exception installed at runtime 
> exit (" INTPTR_FORMAT ")", p2i(this));
> 2088          if (has_last_Java_frame()) {
> 2089            frame f = last_frame();
> 2090           logstream->print(" (pc: " INTPTR_FORMAT " sp: " 
> INTPTR_FORMAT " )", p2i(f.pc()), p2i(f.sp()));
> 2091          }
>
> Indention of these lines is wrong at #2088 and #2089 - need an 
> additional space.
Thanks for catching those!
>
> 2092         logstream->print_cr(" of type: %s", 
> InstanceKlass::cast(_pending_async_exception->klass())->external_name());
>
> Why did the InstanceKlass::cast need to be introduced ??
Thanks for noticing. I believe someone else's changeset modified that 
line so the merging didn't update it. I've removed the cast.
>
> ---
>
> test/runtime/CommandLine/TraceExceptionsTest.java
>
> You improved the readability of the source code by breaking the 
> @summary over two lines, but IIRC jtreg will write the summary into 
> the test log with all the additional spaces you added, as the summary 
> extends until the next tag.
Ok, I didn't realize that - I've put it back as it was.
>
> ---
>
> test/runtime/logging/ExceptionsTest.java
>
> Can you avoid the code duplication please.
>
> Also perhaps you could/should also check there are no logging 
> statements present when logging is supposed to be disabled. I'm 
> guessing the other logging tests haven't considered this. ??
Good points. I've added a check.
>
>> If you have any questions on the alias table (in the arguments.cpp and
>> .hpp files), Max will chime in as he is the one who implemented that
>> portion.
>
> Okay some general questions.
>
> First I would expect that aliased logging options are also marked 
> deprecated so that we can eventually make them obsolete and remove them.
Not sure about this one. Max? Coleen?
>
> Secondly, arguably if someone specifies -XX:-TraceExceptions it should 
> disable exception logging - which may have been turned on by -Xlog:all.
>
> Which leads to: how does the position of -XX:+/-TraceExceptions 
> interact with the position of -Xlog:xxx on the command-line (or the 
> other argument introducing mechanisms) ?
Very true, I had forgotten about the option to turn it off. Have added 
an entry in the alias table to that effect.
>
> src/share/vm/runtime/arguments.cpp
>
> 2743       if(lookup_logging_aliases(option->optionString, 
> aliased_logging_option)){
>
> Need space after "if", and before {
Done.
Thanks,
Rachel
>
> Thanks,
> David
> -----
>
>
>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8141211/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8141211
>>
>> Testing: jtreg, JPRT, jck vm tests, refworkload performance tests, rbt
>> quick & non-colo tests
>>
>> Thank you!
>> Rachel