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:55:38 UTC 2015
> 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...
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