Your changes look good, Daniel. I can sponsor the changes.
Thank you,
Jini.
On 2/2/2018 7:28 PM, stewartd.qdt wrote:
Hi Jini,
Thank you for the review. I have made the requested changes and posted them at
http://cr.openjdk.java.net/~dstewart/8196361/webrev.03/
Please have a look and review the changes.
Thanks,
Daniel
-----Original Message-----
From: Jini George [mailto:jini.geo...@oracle.com]
Sent: Friday, February 2, 2018 1:19 AM
To: David Holmes <david.hol...@oracle.com>; stewartd.qdt
<stewartd....@qualcommdatacenter.com>
Cc: serviceability-dev <serviceability-dev@openjdk.java.net>;
hotspot-...@openjdk.java.net
Subject: Re: RFR: 8196361: JTReg failure in serviceability/sa/ClhsdbInspect.java
Hi Daniel,
Your changes look good to me overall. Just some nits:
* Please do add 2018 to the copyright year.
* Since the rest of the file follows 4 spaces for indentation, please keep the
indentation to 4 spaces.
* Line 81: It would be great if the opening brace is at line 80, so that it
would be consistent with the rest of the file.
* Line 65: The declaration could be a part of line 79.
* Line 51: Please add the 'oop address of a java.lang.Class' to the comment.
Thanks!
Jini.
On 2/2/2018 7:31 AM, David Holmes wrote:
On 2/02/2018 1:50 AM, stewartd.qdt wrote:
Please have a look at the newest changes at:
http://cr.openjdk.java.net/~dstewart/8196361/webrev.02/
The only difference between this and the last changeset is the use of
"\\R" instead of whatever is the platform line.separator.
Thanks for that.
The overall changes seem reasonable but I'll defer to Jini for final
approval. If Jini approves then consider this Reviewed.
Thanks,
David
Thank you,
Daniel
-----Original Message-----
From: David Holmes [mailto:david.hol...@oracle.com]
Sent: Thursday, February 1, 2018 2:51 AM
To: stewartd.qdt <stewartd....@qualcommdatacenter.com>; Jini George
<jini.geo...@oracle.com>
Cc: serviceability-dev <serviceability-dev@openjdk.java.net>;
hotspot-...@openjdk.java.net
Subject: Re: RFR: 8196361: JTReg failure in
serviceability/sa/ClhsdbInspect.java
Hi Daniel,
On 1/02/2018 2:45 AM, stewartd.qdt wrote:
Hi Jini, David,
Please have a look at the revised webrev:
http://cr.openjdk.java.net/~dstewart/8196361/webrev.01/
In this webrev I have changed the approach to finding the addresses.
This was necessary because in the case of matching for the locks the
addresses are before what is matched and in the case of Method the
address is after it. The existing code only looked for the
addresses after the matched string. I've also tried to align what
tokens are being looked for in the lock case. I've taken an
approach of breaking the jstack output into lines and then searching
each line for it containing what we want. Once found, the line is
broken into pieces to find the actual address we want.
Please let me know if this is an unacceptable approach or any
changes you would like to see.
I'm not clear on the overall approach as I'm unclear exactly how
inspect operates or exactly what the test is trying to verify. One
comment on breaking things into lines though:
73 String newline =
System.getProperty("line.separator");
74 String[] lines = jstackOutput.split(newline);
As split() takes a regex, I suggest using \R to cover all potential
line-breaks, rather than the platform specific line-seperator. We've
been recently bitten by the distinction between output that comes
from reading a process's stdout/stderr (and for which a newline \n is
translated into the platform line-seperator), and output that comes
across a socket connection (for which \n is not translated). This
could result in failing to parse things correctly on Windows. It's
safer/simpler to expect any kind of line-seperator.
Thanks,
David
Thanks,
Daniel
-----Original Message-----
From: Jini George [mailto:jini.geo...@oracle.com]
Sent: Tuesday, January 30, 2018 6:58 AM
To: David Holmes <david.hol...@oracle.com>; stewartd.qdt
<stewartd....@qualcommdatacenter.com>
Cc: serviceability-dev <serviceability-dev@openjdk.java.net>;
hotspot-...@openjdk.java.net
Subject: Re: RFR: 8196361: JTReg failure in
serviceability/sa/ClhsdbInspect.java
Hi Daniel, David,
Thanks, Daniel, for bringing this up. The intent of the test is to
get the oop address corresponding to a
java.lang.ref.ReferenceQueue$Lock,
which can typically be obtained from the stack traces of the
Common-Cleaner or the Finalizer threads. The stack traces which I
had been noticing were typically of the form:
"Common-Cleaner" #8 daemon prio=8 tid=0x00007f09c82ac000 nid=0xf6e
in
Object.wait() [0x00007f09a18d2000]
java.lang.Thread.State: TIMED_WAITING (on object monitor)
JavaThread state: _thread_blocked
- java.lang.Object.wait(long) @bci=0, pc=0x00007f09b7d6480b,
Method*=0x00007f09acc43d60 (Interpreted frame)
- waiting on <0x000000072e61f6e0> (a
java.lang.ref.ReferenceQueue$Lock)
- java.lang.ref.ReferenceQueue.remove(long) @bci=59, line=151,
pc=0x00007f09b7d55243, Method*=0x00007f09acdab9b0 (Interpreted
frame)
- waiting to re-lock in wait() <0x000000072e61f6e0> (a
java.lang.ref.ReferenceQueue$Lock)
...
I chose 'waiting to re-lock in wait' since that was what I had been
observing next to the oop address of java.lang.ref.ReferenceQueue$Lock.
But I see how with a timing difference, one could get 'waiting to lock'
as in your case. So, a good way to fix might be to check for the
line containing '(a java.lang.ref.ReferenceQueue$Lock)', getting the
oop address from that line (should be the address appearing
immediately before '(a java.lang.ref.ReferenceQueue$Lock)') and
passing that to the 'inspect' command.
Thanks much,
Jini.
On 1/30/2018 3:35 AM, David Holmes wrote:
Hi Daniel,
Serviceability issues should go to
serviceability-dev@openjdk.java.net
- now cc'd.
On 30/01/2018 7:53 AM, stewartd.qdt wrote:
Please review this webrev [1] which attempts to fix a test error
in serviceability/sa/ClhsdbInspect.java when it is run under an
AArch64 system (not necessarily exclusive to this system, but it
was the system under test). The bug report [2] provides further details.
Essentially the line "waiting to re-lock in wait" never actually
occurs. Instead I have the line "waiting to lock" which occurs for
the referenced item of /java/lang/ref/ReferenceQueue$Lock.
Unfortunately the test is written such that only the first
"waiting to lock"
occurrence is seen (for java/lang/Class), which is already
accounted for in the test.
I can't tell exactly what the test expects, or why, but it would be
extremely hard to arrange for "waiting to re-lock in wait" to be
seen for the ReferenceQueue lock! That requires acquiring the lock
yourself, issuing a notify() to unblock the wait(), and then
issuing the jstack command while still holding the lock!
David
-----
I'm not overly happy with this approach as it actually removes a
test line. However, the test line does not actually appear in the
output (at least on my system) and the test is not currently
written to look for the second occurrence of the line "waiting to lock".
Perhaps the original author could chime in and provide further
guidance as to the intention of the test.
I am happy to modify the patch as necessary.
Regards,
Daniel Stewart
[1] - http://cr.openjdk.java.net/~dstewart/8196361/webrev.00/
[2] - https://bugs.openjdk.java.net/browse/JDK-8196361
