RFR(S) 8130448: thread dump improvements, comment, additions, new diagnostics inspired by 8077392
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jul 9 14:11:28 UTC 2015
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