Thanks Jini. Regards, Daniel
-----Original Message----- From: Jini George [mailto:jini.geo...@oracle.com] Sent: Sunday, February 4, 2018 10:56 PM To: stewartd.qdt <stewartd....@qualcommdatacenter.com>; David Holmes <david.hol...@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 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 >>>>>>
