RFR(S) 8130448: thread dump improvements, comment, additions, new diagnostics inspired by 8077392
David Holmes
david.holmes at oracle.com
Fri Jul 10 00:17:17 UTC 2015
Sorry I missed this update before previous reply.
Thumbs up!
David
On 10/07/2015 12:11 AM, Daniel D. Daugherty wrote:
> On 7/9/15 7:55 AM, Daniel D. Daugherty wrote:
>> > Any particular reason to use "fatal(...)" instead of
>> > "guarantee(false, ...)". I've seen both...
>>
>> Answering my own question... :-)
>>
>> fatal() and guarantee() both reach the same place (report_vm_error())
>> with the same bells-and-whistles on the way...
>>
>> So "guarantee(false, ...)" is the same as "fatal(...)"
>> so I'll switch to the more direct "fatal(...)".
>>
>> I don't think that change requires a re-review, please let me know
>> if you disagree...
>
> Sigh... I meant "webrev"... Here's the diffs:
>
> $ diff -c src/share/vm/runtime/synchronizer.cpp{.cr1,}
> *** src/share/vm/runtime/synchronizer.cpp.cr1 Wed Jul 8 07:15:07 2015
> --- src/share/vm/runtime/synchronizer.cpp Thu Jul 9 07:07:28 2015
> ***************
> *** 1651,1661 ****
> Handle obj((oop) mid->object());
> tty->print("INFO: unexpected locked object:");
> javaVFrame::print_locked_object_class_name(tty, obj, "locked");
> }
> - guarantee(ObjectMonitor::Knob_VerifyMatch == 0,
> - err_msg("exiting JavaThread=" INTPTR_FORMAT
> - " unexpectedly owns ObjectMonitor="
> INTPTR_FORMAT,
> - THREAD, mid));
> (void)mid->complete_exit(CHECK);
> }
> }
> --- 1651,1660 ----
> 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);
> }
> }
>
> Dan
>
>
>>
>> Dan
>>
>>
>> On 7/9/15 7:08 AM, Daniel D. Daugherty wrote:
>>> 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