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
