PING: RFR: 8185796: jstack and clhsdb jstack should show lock objects
Jini George
jini.george at oracle.com
Thu Nov 23 16:06:03 UTC 2017
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