RFR(S) 8130448: thread dump improvements, comment, additions, new diagnostics inspired by 8077392
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Jul 10 00:22:52 UTC 2015
Thanks!
Dan
On 7/9/15 6:17 PM, David Holmes wrote:
> 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