David, Thanks for the review. I'll change the split() to look for '\r' instead. I was unaware of the problems with line.separator, and was actually trying to avoid cross-platform issues by using it. But things are always more complicated than they seem!
As far as the original intent of the test and how inspect operates, I will defer to the original author. I was just trying to get the same information, but had to change the original search approach because the original approach assumed the addresses came _after_ the string that was being searched. In searching for the name of the actual class instead of just "waiting to lock", the address comes before the string. I assume the classes I am searching for is correct, as these are the classes that actually get found in the original approach and the classes that seem to be looked for by the subsequent OutputAnalyzer. 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 >>>
