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.spit...@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 <yasue...@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.
