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

Siebenborn, Axel axel.siebenborn at sap.com
Wed Mar 19 08:05:40 UTC 2014


Hi Dmitri,
Thank you for the review.
You're right, the code gets cleaner this way. 
New webrev:
http://www.sapjvm.com/as/webrevs/8036666_3/
Thanks,
Axel

On 17.03.2014 23:01, Dmitry Samersoff wrote:
> Axel,
>
> The changes it self looks good for me.
>
> But it looks like the owning_thread is always NULL if
> owner is NULL, so we can safely move this code
> to ll.1017 and join two identical ifs together.
>
> Also comment on ll. 1019 is misleading, could you remove it?
>
> -Dmitry
>
> On 2014-03-13 12:19, Siebenborn, Axel wrote:
>> 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
>>>
>>>
>>
>
>


More information about the hotspot-runtime-dev mailing list