[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: PING: RFR: 8185796: jstack and clhsdb jstack should show lock objects
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2017-11-23 21:46:10
Message-ID: 6ca5c669-420f-16d9-9abc-05fce01a8e9a () oracle ! com
[Download RAW message or body]
Thanks, Jini!
The update looks good to me.
Thanks,
Serguei
On 11/23/17 08:06, Jini George wrote:
> Yes, Yasumasa and Serguei, will sponsor this.
>
> Thanks,
> Jini.
>
> On 11/23/2017 7:06 PM, Yasumasa Suenaga wrote:
> > Hi Serguei,
> >
> > Thank you for your comment.
> > I've fixed them in new webrev:
> >
> > http://cr.openjdk.java.net/~ysuenaga/JDK-8185796/webrev.05/
> >
> >
> > > Jini, could you, take care about this sponsorship?
> >
> > Please sponsor for this change :-)
> >
> >
> > Yasumasa
> >
> >
> > On 2017/11/23 8:57, serguei.spitsyn@oracle.com wrote:
> > > 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 <yasuenag@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.
> > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic