PING: RFR: 8185796: jstack and clhsdb jstack should show lock objects
Yasumasa Suenaga
yasuenag at gmail.com
Thu Nov 23 13:36:23 UTC 2017
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 at 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 at 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.
>
More information about the serviceability-dev
mailing list