Re: PING: RFR: 8185796: jstack and clhsdb jstack should show lock objects

[serguei.spit...@oracle.com]

Hi Yasumasa, http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/java_lang_Class.java.udiff.html + public static String asExternalName(Oop aClass) { + Klass k = java_lang_Class.asKlass(aClass); + if (k == null) { // primitive + BasicType type = BasicType.T_VOID; + ArrayKlass ak = (ArrayKlass)Metadata.instantiateWrapperFor( + aClass.getHandle().getAddressAt(arrayKlassOffset)); + if (ak != null) { + type = BasicType.intToBasicType(ak.getElementType()); + } If I understand correctly, it is array of a primitive type, not a primitive. The comment needs to be updated accordingly. http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/BasicType.java.udiff.html It looks like the change is a little bit more complex than necessary. It could be enough to just introduce new method getName() like this: public String getName() { String name = "ILLEGAL TYPE"; switch (type) { case tBoolean: name = "boolean"; . . . }  return name; } http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaVFrame.java.udiff.html The logic in the printLockInfo() is unclear because there are two almost identical fragments here: + if (monitor.owner() != null) { + // the monitor is associated with an object, i.e., it is locked + + Mark mark = null; + String lockState = "locked"; + if (!foundFirstMonitor && frameCount == 0) { + // If this is the first frame and we haven't found an owned + // monitor before, then we need to see if we have completed + // the lock or if we are blocked trying to acquire it. Only + // an inflated monitor that is first on the monitor list in + // the first frame can block us on a monitor enter. + mark = new Mark(monitor.owner()); + if (mark.hasMonitor() && + ( // we have marked ourself as pending on this monitor + mark.monitor().equals(thread.getCurrentPendingMonitor()) || + // we are not the owner of this monitor + !mark.monitor().isEntered(thread) + )) { + lockState = "waiting to lock"; + } else { + // We own the monitor which is not as interesting so + // disable the extra printing below. + mark = null; + } + } else if (frameCount != 0) { + // This is not the first frame so we either own this monitor + // or we owned the monitor before and called wait(). Because + // wait() could have been called on any monitor in a lower + // numbered frame on the stack, we have to check all the + // monitors on the list for this frame. + mark = new Mark(monitor.owner()); + if (mark.hasMonitor() && + ( // we have marked ourself as pending on this monitor + mark.monitor().equals(thread.getCurrentPendingMonitor()) || + // we are not the owner of this monitor + !mark.monitor().isEntered(thread) + )) { + lockState = "waiting to re-lock in wait()"; + } else { + // We own the monitor which is not as interesting so + // disable the extra printing below. + mark = null; + } + } + printLockedObjectClassName(tty, monitor.owner(), lockState); + foundFirstMonitor = true; A way to simplify this part would be to add a method like this:   String identifyLockState(String waitingState) {     Mark mark = new Mark(monitor.owner());     String lockState = "locked";     if (mark.hasMonitor() &&         ( // we have marked ourself as pending on this monitor           mark.monitor().equals(thread.getCurrentPendingMonitor()) ||           // we are not the owner of this monitor           !mark.monitor().isEntered(thread)         )) {       lockState = waitingState;     }     return lockState;   } Then the fragment above could be reduced to:     if (monitor.owner() != null) {       // the monitor is associated with an object, i.e., it is locked       String lockState = "locked";       if (!foundFirstMonitor && frameCount == 0) {         lockState = identifyLockState("waiting to lock");       } else if (frameCount != 0) {         lockState = identifyLockState("waiting to re-lock in wait()");       }       printLockedObjectClassName(tty, monitor.owner(), lockState);       foundFirstMonitor = true;     } http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/test/hotspot/jtreg/serviceability/sa/LingeredAppWithLock.java.html The indent is inconsistent, the lines 29-37 have 2 instead of 4. 30 synchronized(lock) {   Space is missed before '('. 40 Thread classLock1 = new Thread( 41 () -> lockMethod(LingeredAppWithLock.class)); 42 Thread classLock2 = new Thread( 43 () -> lockMethod(LingeredAppWithLock.class)); 44 Thread objectLock = new Thread(() -> lockMethod(classLock1)); 45 Thread primitiveLock = new Thread(() -> lockMethod(int.class));   No need to separate lines at 40-43. http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/test/hotspot/jtreg/serviceability/sa/TestClhsdbJstackLock.java.html Indent 3 instead of 4 in the fragment 97-101. No need to to split the lines: 114 System.out.println( 115 pb.command().stream().collect(Collectors.joining(" "))); . . . 156 System.out.println( 157 "SA attach not expected to work - test skipped."); http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackLock.java.html 49 System.out.println( 50 "SA attach not expected to work - test skipped.");   No need to split the line above. On 11/19/17 05:37, Yasumasa Suenaga wrote: PING: Could you review it?     http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/ I want to merge this change to jdk 10. So I need a reviewer and sponsor. Jini, could you, take care about this sponsorship? Thanks, Serguei Yasumasa On 2017/11/14 9:58, Yasumasa Suenaga wrote: PING: Could you review it? We need a reviewer and sponsor.     http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/ Thanks, Yasumasa 2017-11-09 23:34 GMT+09:00 Yasumasa Suenaga <yasue...@gmail.com>: Thanks, Jini! I'm waiting for Reviewer and sponsor. Yasumasa On 2017/11/09 23:25, Jini George wrote: Hi Yasumasa, This looks fine to me. Thank you, Jini (Not a Reviewer). On 11/9/2017 6:55 PM, Yasumasa Suenaga wrote: Hi Jini, Thank you for your comment! I've fixed and uploaded new webrev:     http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.04/ * src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaVFrame.java -> Lines 198-212: I feel this commented out code could be removed and replaced by a call to printLockInfo(), though I am not entirely sure as to when this printOn() gets exercised. I agree with you to remove these comments. They are insufficient to show all locks like a my first webrev [1]. webrev.04 is implemented to follow HotSpot implementation. Thanks, Yasumasa [1] http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.00/ On 2017/11/09 2:19, Jini George wrote: Hi Yasumasa, Your changes look good to me overall. Some nits: * src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/BasicType.java (lines 138 to 152): -> It would be nice if you could indent the "case" statements. * src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ui/classbrowser/HTMLGenerator.java -> It would be good if the indentation here for the newly added lines matches that of the rest of the file. (4 spaces instead of 2). * src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaVFrame.java -> Lines 198-212: I feel this commented out code could be removed and replaced by a call to printLockInfo(), though I am not entirely sure as to when this printOn() gets exercised. * test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackLock.java -> You can remove these lines: import java.util.Scanner; import java.util.stream.Collectors; import java.io.File; Thanks, Jini (Not a Reviewer). On 11/1/2017 6:28 PM, Yasumasa Suenaga wrote: PING: Could you review and sponsor it? ?? http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.03/ Thanks, Yasumasa On 2017/10/09 23:19, Yasumasa Suenaga wrote: Hi all, I uploaded new webrev to be adapted to current jdk10/hs: ?? http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.03/ Please review and sponsor it. Thanks, Yasumasa On 2017/09/27 0:31, Yasumasa Suenaga wrote: Hi all, I uploaded new webrev to be adapted to jdk10/hs: ?? http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.02/ Thanks, Yasumasa On 2017/08/24 22:59, Yasumasa Suenaga wrote: Thanks Jini! I uploaded new webrev: ?? http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.01/ This webrev has been ported print_lock_info() to JavaVFrame.java, and I've added new testcase for `jhsdb jstack` and jstack command on `jhsdb clhsdb`. Yasumasa On 2017/08/24 18:01, Jini George wrote: Apologize for the late reply, Yasumasa. I think so, but I guess it is difficult. For example, test for CLHSDB command is provided as test/serviceability/sa/TestPrintMdo.java . But target process seems to be fixed to "LingeredApp". Can we change it to another program which generates lock contention? You can take a look at any of the hotspot/test/serviceability/sa/LingeredAppWith*.java files for this. The target process does not have to be be fixed to LingeredApp -- in these LingeredAppWith* cases, the targets are test-specific variations built on top of LingeredApp for ease of implementation. Thanks, Jini.