RFR: 8196361: JTReg failure in serviceability/sa/ClhsdbInspect.java

Fri Feb 2 13:58:19 UTC 2018

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.george at oracle.com] 
Sent: Friday, February 2, 2018 1:19 AM
To: David Holmes <david.holmes at oracle.com>; stewartd.qdt <stewartd.qdt at qualcommdatacenter.com>
Cc: serviceability-dev <serviceability-dev at openjdk.java.net>; hotspot-dev at 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.holmes at oracle.com]
>> Sent: Thursday, February 1, 2018 2:51 AM
>> To: stewartd.qdt <stewartd.qdt at qualcommdatacenter.com>; Jini George 
>> <jini.george at oracle.com>
>> Cc: serviceability-dev <serviceability-dev at openjdk.java.net>;
>> hotspot-dev at 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.george at oracle.com]
>>> Sent: Tuesday, January 30, 2018 6:58 AM
>>> To: David Holmes <david.holmes at oracle.com>; stewartd.qdt 
>>> <stewartd.qdt at qualcommdatacenter.com>
>>> Cc: serviceability-dev <serviceability-dev at openjdk.java.net>;
>>> hotspot-dev at 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 at 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
>>>>>