RFR(S) 8130448: thread dump improvements, comment, additions, new diagnostics inspired by 8077392

[David Holmes]

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?

- 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).

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).

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);
      }
    }

?

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
>>
>>
>