RFR(S) 8130448: thread dump improvements, comment, additions, new diagnostics inspired by 8077392
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jul 9 13:08:41 UTC 2015
David,
Thanks for the quick re-review!
Responses embedded below...
On 7/9/15 12:34 AM, David Holmes wrote:
> Hi Dan,
>
> On 9/07/2015 7:33 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I've updated this patch based on David H's comments along with
>> some additional cleanup.
>
> So to address the high-level changes:
>
> - Use ObjectMonitor::Knob_Verbose as the verbosity flag for printing
> the lockbits in the stack dump. Okay - though I do wonder why the
> SyncVerbose flag also exists?
I'll ping Dice to see if there's a reason for two different verbose
flags in the monitor subsystem. I have to admit that I didn't notice
that one in the large collection of knobs and switches... :-(
> - Use ObjectMonitor::Knob_ExitRelease to control whether Java threads
> are checked for locked monitors when they exit
> - Use ObjectMonitor::Knob_VerifyMatch to control whether finding a
> locked (hence unmatched) monitor causes an abort
>
> That seems like a good way to work within the existing mechanism for
> customizing the sync logic - and no new global options (though SQE
> will still need tests for using these knobs I think).
Glad that this version is more acceptable.
> Some specific comments below ...
>
>> This work is being tracked by the following bug ID:
>>
>> JDK-8130448 8077392 inspired thread dump improvements, comment
>> additions, new diagnostics
>> https://bugs.openjdk.java.net/browse/JDK-8130448
>>
>> Here is the webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/8130448-webrev/1-jdk9-hs-rt/
>
> src/share/vm/runtime/objectMonitor.cpp
>
> 2393 if (Knob_ReportSettings && v != NULL) {
> 2394 tty->print_cr("INFO: SyncKnob: %s %d(%d)", Key, rslt, Default) ;
> 2395 tty->flush();
> 2396 }
>
> I suspect these were not tty using because this might get called
> before the tty has been initialized - but I can't see how that might
> arise (nor can I be certain it can't).
I couldn't see how tty could not be initialized at these points either.
My current plan is to go with this change to 'tty' and if we run into
an early use case, then we do something else _and_ comment it.
Although with unified logging coming... this may be moot (but the
tty version will be easier to port...)
>
> Ditto for similar changes throughout.
>
> ---
>
> src/share/vm/runtime/synchronizer.cpp
>
> 1648 void do_monitor(ObjectMonitor* mid) {
> 1649 if (mid->owner() == THREAD) {
> 1650 if (ObjectMonitor::Knob_VerifyMatch != 0) {
> 1651 Handle obj((oop) mid->object());
> 1652 tty->print("INFO: unexpected locked object:");
> 1653 javaVFrame::print_locked_object_class_name(tty, obj,
> "locked");
> 1654 }
> 1655 guarantee(ObjectMonitor::Knob_VerifyMatch == 0,
> 1656 err_msg("exiting JavaThread=" INTPTR_FORMAT
> 1657 " unexpectedly owns ObjectMonitor="
> INTPTR_FORMAT,
> 1658 THREAD, mid));
> 1659 (void)mid->complete_exit(CHECK);
> 1660 }
> 1661 }
>
> Took me a while to figure out the expected control flow here. Doesn't
> the above simplify to:
>
> void do_monitor(ObjectMonitor* mid) {
> if (mid->owner() == THREAD) {
> if (ObjectMonitor::Knob_VerifyMatch != 0) {
> Handle obj((oop) mid->object());
> tty->print("INFO: unexpected locked object:");
> javaVFrame::print_locked_object_class_name(tty, obj, "locked");
> fatal(err_msg("exiting JavaThread=" INTPTR_FORMAT
> " unexpectedly owns ObjectMonitor=" INTPTR_FORMAT,
> THREAD, mid));
> }
> (void)mid->complete_exit(CHECK);
> }
> }
>
> ?
Yes, your version is cleaner.
Any particular reason to use "fatal(...)" instead of
"guarantee(false, ...)". I've seen both...
Dan
>
> Thanks,
> David
>
>>
>> The easiest way to review the delta is to download the two patch
>> files and compare them in something like jfilemerge:
>>
>> http://cr.openjdk.java.net/~dcubed/8130448-webrev/0-jdk9-hs-rt/hotspot.patch
>>
>>
>> http://cr.openjdk.java.net/~dcubed/8130448-webrev/1-jdk9-hs-rt/hotspot.patch
>>
>>
>>
>> Testing:
>>
>> - Aurora Adhoc RT/SVC nightly batches (in process)
>> - JPRT test jobs
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Gory details about the changes are below...
>>
>> Dan
>>
>> Changes between CR0 and CR1 are:
>>
>> src/cpu/x86/vm/macroAssembler_x86.cpp
>>
>> - fix comment typos, clarify comment wording
>>
>> src/share/vm/oops/markOop.cpp
>>
>> - clarify output messages
>> - get rid of confusing local variable
>>
>> src/share/vm/runtime/globals.hpp
>>
>> - drop VerboseStackTrace, GuaranteeOnMonitorMismatch
>> and JavaThreadExitReleasesMonitors options
>> - there are no longer changes to this file
>>
>> src/share/vm/runtime/objectMonitor.cpp
>>
>> - add ObjectMonitor::Knob_ExitRelease suboption to replace
>> JavaThreadExitReleasesMonitors
>> - add ObjectMonitor::Knob_VerifyMatch suboption to replace
>> GuaranteeOnMonitorMismatch
>> - cleanup messy logging:
>> - switch from '::printf()' -> 'tty->print_cr()'
>> - switch from '::fflush()' -> 'tty->flush()'
>>
>> src/share/vm/runtime/objectMonitor.hpp
>>
>> - add ObjectMonitor::Knob_ExitRelease suboption
>> - add ObjectMonitor::Knob_VerifyMatch suboption
>> - cleanup messy logging
>>
>> src/share/vm/runtime/synchronizer.cpp
>>
>> - cleanup messy logging
>> - switch from GuaranteeOnMonitorMismatch to
>> ObjectMonitor::Knob_VerifyMatch
>> - add diagnostic information about the locked object
>> when ReleaseJavaMonitorsClosure::do_monitor() finds
>> a problem
>>
>> src/share/vm/runtime/thread.cpp
>>
>> - clarify comments
>> - switch from JavaThreadExitReleasesMonitors to
>> ObjectMonitor::Knob_ExitRelease
>>
>> src/share/vm/runtime/vframe.cpp
>>
>> - print_locked_object_class_name() is now public
>> - clarify comments
>> - clarify output messages
>> - switch from VerboseStackTrace to the already existing
>> ObjectMonitor::Knob_Verbose
>>
>> src/share/vm/runtime/vframe.hpp
>>
>> - print_locked_object_class_name() is now public
>>
>>
>> On 7/3/15 6:14 PM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> The hunt for the following bug:
>>>
>>> JDK-8077392 Stream fork/join tasks occasionally fail to complete
>>>
>>> and David C's work on the following bug:
>>>
>>> JDK-8069412 Locks need better debug-printing support
>>>
>>> have inspired additional thread dump improvements, comment additions
>>> to some Java monitor code and some new diagnostic options.
>>>
>>> This work is being tracked by the following bug ID:
>>>
>>> JDK-8130448 8077392 inspired thread dump improvements, comment
>>> additions, new diagnostics
>>> https://bugs.openjdk.java.net/browse/JDK-8130448
>>>
>>> Here is the webrev URL:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8130448-webrev/0-jdk9-hs-rt/
>>>
>>> Testing:
>>>
>>> - RBT vm.quick batches (in process)
>>> - JPRT test jobs
>>>
>>> Thanks, in advance, for any comments, questions or suggestions.
>>>
>>> Gory details about the changes are below...
>>>
>>> Dan
>>>
>>> 8130448 summary of changes:
>>>
>>> src/cpu/x86/vm/macroAssembler_x86.cpp
>>> - comment additions for the assembly code
>>>
>>> src/share/vm/oops/markOop.cpp
>>> - has_monitor() has to be checked before is_locked() since
>>> the has_monitor() bits are a superset of the is_locked() bits
>>> - code style fixes
>>>
>>> src/share/vm/runtime/globals.hpp
>>> - add VerboseStackTrace diagnostic option
>>> - add GuaranteeOnMonitorMismatch diagnostic option
>>> - add JavaThreadExitReleasesMonitors product option;
>>> this is the Java equivalent of JNIDetachReleasesMonitors
>>>
>>> src/share/vm/runtime/objectMonitor.cpp
>>> - delete unused ObjectMonitor::try_enter()
>>> - fix assert wording
>>>
>>> src/share/vm/runtime/objectMonitor.hpp
>>> - delete unused ObjectMonitor::try_enter()
>>>
>>> src/share/vm/runtime/synchronizer.cpp
>>> - add GuaranteeOnMonitorMismatch support with some
>>> diagnostic output
>>>
>>> src/share/vm/runtime/thread.cpp
>>> - add JavaThreadExitReleasesMonitors support
>>>
>>> src/share/vm/runtime/vframe.cpp
>>> - clarify existing comments
>>> - add comments to clarify what "waiting on" means
>>> - add line to show when we are "waiting on", but
>>> there are no locals available (previously we were
>>> "waiting on" silently)
>>> - add support for "waiting to relock" which can happen
>>> for any of the monitors and not just the top monitor
>>> - switch mark->print_on() verbose output to new
>>> VerboseStackTrace switch; thread dumps are not enabled
>>> with a specific switch so the '-XX:+Verbose' model of
>>> being a modifier for another option does not fit
>>>
>>>
>>
More information about the hotspot-runtime-dev
mailing list