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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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 <[hidden email]>:

> 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.
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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.


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 <[hidden email]>:
>> 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.
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@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 [hidden email]:
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.

Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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, [hidden email] 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 <[hidden email]>:
>>>> 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.
>
Reply | Threaded
Open this post in threaded view
|

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

Jini George
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, [hidden email] 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 <[hidden email]>:
>>>>> 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.
>>
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
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, [hidden email] 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 <[hidden email]>:
>>>>>> 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.
>>>