Thank you for the review. I have made the requested changes and posted them at
Please have a look and review the changes.
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
Cc: serviceability-dev <email@example.com>;
Subject: Re: RFR: 8196361: JTReg failure in serviceability/sa/ClhsdbInspect.java
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.
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:
>> 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.
>> Thank you,
>> -----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
>> Cc: serviceability-dev <firstname.lastname@example.org>;
>> Subject: Re: RFR: 8196361: JTReg failure in
>> Hi Daniel,
>> On 1/02/2018 2:45 AM, stewartd.qdt wrote:
>>> Hi Jini, David,
>>> Please have a look at the revised webrev:
>>> 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 =
>> 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.
>>> -----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
>>> Cc: serviceability-dev <email@example.com>;
>>> Subject: Re: RFR: 8196361: JTReg failure in
>>> Hi Daniel, David,
>>> Thanks, Daniel, for bringing this up. The intent of the test is to
>>> get the oop address corresponding to a
>>> 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
>>> 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.remove(long) @bci=59, line=151,
>>> pc=0x00007f09b7d55243, Method*=0x00007f09acdab9b0 (Interpreted
>>> - waiting to re-lock in wait() <0x000000072e61f6e0> (a
>>> 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,
>>> On 1/30/2018 3:35 AM, David Holmes wrote:
>>>> Hi Daniel,
>>>> Serviceability issues should go to
>>>> - now cc'd.
>>>> On 30/01/2018 7:53 AM, stewartd.qdt wrote:
>>>>> Please review this webrev  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  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!
>>>>> 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.
>>>>> Daniel Stewart
>>>>>  - http://cr.openjdk.java.net/~dstewart/8196361/webrev.00/
>>>>>  - https://bugs.openjdk.java.net/browse/JDK-8196361