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/
Advertising
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 >>>>>
