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

stewartd.qdt stewartd.qdt at qualcommdatacenter.com
Mon Feb 5 13:59:57 UTC 2018 

Thanks Jini. 

Regards,
Daniel

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

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.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
>>>>>>

More information about the serviceability-dev mailing list