PING: RFR: 8185796: jstack and clhsdb jstack should show lock objects
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Nov 23 21:46:10 UTC 2017
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 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