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

[Siebenborn, Axel]

Hi Serguei,
new webrev:
http://www.sapjvm.com/as/webrevs/8036666_2/
I should review my own changes more carefully.
Sorry for that.
Thanks,
Axel



On 12.03.2014 18:34, serguei.spitsyn at oracle.com wrote:
> Hi Axel,
> 
> Thank you for the changes! It looks good, but one more place need a
> fix (expected must be 4 now):
> 
> 230         if (recursionCount != 4) { 231             throw new
> AssertionError("recursions: expected 3, but was " + recursionCount); 
> 232         }
> 
> 
> Thanks, Serguei
> 
> 
> On 3/12/14 8:21 AM, Siebenborn, Axel wrote:
>> Hi Serguei, I created a new webrev:
>> 
>> http://www.sapjvm.com/as/webrevs/8036666_1/
>> 
>> I incorporated your suggestions and moved the test files.
>> 
>> Thanks, Axel
>> 
>> 
>> On 11.03.2014 20:25, serguei.spitsyn at oracle.com wrote:
>>> 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
> 
>