RFR(XS): 8036666: JVMTI GetObjectMonitorUsage does not return correct recursion count

[serguei.spitsyn at oracle.com]

On 3/11/14 12:05 PM, Staffan Larsen wrote:
>
> On 11 mar 2014, at 16:48, Siebenborn, Axel <axel.siebenborn at sap.com 
> <mailto:axel.siebenborn at sap.com>> wrote:
>
>> Hi Seguei,
>> I still can't upload files to the cr.openjdk server.
>> Meanwhile, I use our server for the new webrev:
>>
>> http://www.sapjvm.com/as/webrevs/8036666/
>>
>> Thanks,
>> Axel
>>
>> Comments inline:
>>
>> On 11.03.2014 09:50,serguei.spitsyn at oracle.com 
>> <mailto:serguei.spitsyn at oracle.com>wrote:
>>> Hi Axel,
>>>
>>> The webrev link is resolvable now.
>>> Thank you for taking care about your broken account on the 
>>> cr.openjdk server!
>>>
>>> I have some comments on the test case ...
>>>
>>> - This is nice test, thank you for providing it!
>>>
>>> - The location of the test must be different as it is a JVMTI test:
>>>      test/serviceability/jvmti/8036666  instead of test/runtime/8036666
>>
>> I moved the test.
>
> Tests should avoid the bug number in the name or path and instead use 
> a descriptive name. See this page for some background: 
> https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests

The test files have already descriptive names.
So, it would be enough to remove the bug number from the path.
Sorry, I had to catch it too in the first place.

Thanks,
Serguei

>
> Thanks,
> /Staffan
>
>>
>>>
>>> RecursiveObjectLock,java:
>>>
>>>  - A suggestion to add a synchronized method (say, nestedLock3) into 
>>> the chain
>>>     of calls started from the testMethod. In order to do so, the 
>>> class RecursiveObjectLock
>>>     needs to extend the ALock class. And the this object needs to be 
>>> used in the
>>>     synchronized statements and for wait()?
>>>     What do you think about such test enhancement for better coverage?
>>
>> In order to have a synchronized method in the call chain, I 
>> synchronize on the "this" object.
>>
>>> GetObjectLockCount.java:
>>>
>>>  - The comment line 283 seems to be obsolete as the "param out" is 
>>> not present anymore:
>>>
>>> 283      * @param out   Stream to copy to
>>>
>>>
>>>  - Could you, please, add e.printStackTrace() into the catch 
>>> statements at the lines 232 and 300?
>>>
>>>  - A question:
>>>       It seems the errThread and outThread are started a little bit 
>>> late.
>>>       Would it be better to start them earlier, or it was intentional?
>>
>> You're right! I moved to code up.
>>
>>>
>>> Some minor style-related comments (I hope, it is easy to fix this 
>>> before push):
>>>    - Unneeded extra empty lines:           149, 174-175, 244
>>>    - A space is missed before the '{':       180, 242, 243, 246
>>>    - Unneeded extra space after and before the "(":   235, 297
>>>    - The curly brackets '{' do not follow the common style:  142, 144
>>>
>>
>> I hope I fixed them all and added no new style violations.
>> Do you have a tool to check this?
>>
>>>
>>> We still need another reviewer for this fix.
>>> I'm ready to be a sponsor for it.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 3/10/14 12:00 AM, serguei.spitsyn at oracle.com 
>>> <mailto:serguei.spitsyn at oracle.com> wrote:
>>>> Hi Axel,
>>>>
>>>> The webrev link does not work now.
>>>> I'll check it again tomorrow.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 3/7/14 1:32 AM, serguei.spitsyn at oracle.com 
>>>> <mailto:serguei.spitsyn at oracle.com> wrote:
>>>>> Hi Axel,
>>>>>
>>>>> Thank you for fixing this issue.
>>>>> I'm reviewing it.
>>>>> It looks good in general, but a little bit more time is needed to 
>>>>> look at the tests.
>>>>>
>>>>> Do you need a sponsor for pushing?
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 3/6/14 12:08 AM, Siebenborn, Axel wrote:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> could I have a review for the following change?
>>>>>>
>>>>>> The recursive lock count for an object is not correct, in cases, 
>>>>>> where a monitor is inflated after recursive lightweight locks. In 
>>>>>> this case, the recursion count is taken from the heavyweight 
>>>>>> monitor, represented by the class ObjectMonitor. 
>>>>>> ObjectMonitor::_recursions is the number of times 
>>>>>> ObjectMonitor::enter() was called to acquire the lock minus 1. 
>>>>>> This counter does not include the recursions of lightweight 
>>>>>> locks, that happen before inflating the monitor and is not equal 
>>>>>> to the recursion count from a Java source level point of view.
>>>>>>
>>>>>> I added a test to the webrev to reproduce the problem.
>>>>>>
>>>>>> The suggested fix is to call count_locked_objects, even if 
>>>>>> there's a heavyweight monitor and get the recursion count by 
>>>>>> iterating the vframes.
>>>>>>
>>>>>> Bug:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8036666
>>>>>>
>>>>>> Webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~asiebenborn/8036666/webrev/ 
>>>>>> <http://cr.openjdk.java.net/%7Easiebenborn/8036666/webrev/><http://cr.openjdk.java.net/%7Easiebenborn/8036666/webrev/>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Axel
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140311/6afbb050/attachment-0001.html>