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

[serguei.spitsyn at oracle.com]

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

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?

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?

  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


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 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 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/>
>>>
>>> Thanks,
>>>
>>> Axel
>>>
>>
>

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