RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

[serguei.spitsyn at oracle.com]

On 1/12/18 15:14, serguei.spitsyn at oracle.com wrote:
> On 1/12/18 15:07, Chris Plummer wrote:
>> On 1/12/18 2:52 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Chris,
>>>
>>>
>>> On 1/12/18 14:31, Chris Plummer wrote:
>>>> Hi Serguei,
>>>>
>>>> On 1/12/18 2:25 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Daniil,
>>>>>
>>>>> It is pretty good in general.
>>>>> Thank you for taking care about it!
>>>>>
>>>>> Some comments though.
>>>>>
>>>>> The test case is too trivial.
>>>>> I'd suggest to extend it to have at least a couple of locks in the 
>>>>> returned array.
>>>>> One way to do it would be to add a instance synchronized method and
>>>>> invoke it from the synchronized statement of the tested Thread.
>>>>> Then the verifyOwnedMonitors() can be invoked from this method.
>>>>>
>>>>> A couple of comments on the native agent.
>>>>>
>>>>> 72         // JNI_OnLoad has not been called yet, so can't 
>>>>> possibly be an instance of TEST_CLASS.
>>>>>
>>>>> Could you, please, rewrite this comment?
>>>>> Maybe just tell that there probably was an error in loading the 
>>>>> TEST_CLASS.
>>>> This was copied from my comment in GetOwnedMonitorInfoTest, which I 
>>>> assume this test was based on.
>>>
>>> Yes, I also assumed it was copied from the GetOwnedMonitorInfoTest.
>>> The comment looks incorrect and creates some confusion.
>> How is it incorrect? If testClass is NULL, that does imply that 
>> JNI_OnLoad has not been called yet, which itself implies that the 
>> object cannot be an instance of TEST_CLASS.
>
> It contradicts with the JNI_OnLoad code:
>
>  100     testClass = (*env)->FindClass(env, TEST_CLASS);
>  101     if (testClass != NULL) {
>  102       testClass = (*env)->NewGlobalRef(env, testClass);
>  103     }
>  104     if (testClass == NULL) {
>  105         fprintf(stderr, "Error: Could not load class %s!\n", 
> TEST_CLASS);
>  106         return JNI_ERR;
>  107     }

I forgot to tell that this comment is not needed if the fragment above 
is moved to the verifyOwnedMonitors().

Thanks,
Serguei


>>>>> What about moving the FindClass(env, TEST_CLASS) to the 
>>>>> verifyOwnedMonitors() function?
>>>>> It will make the testClass variable local.
>>>>>
>>>> Also from GetOwnedMonitorInfoTest. This is code I reworked in that 
>>>> test recently to fix 8191229.
>>>
>>> Yes, I remember.
>>>
>>>> These two tests should be kept consistent.
>>>
>>> I still think, making it a part of the verifyOwnedMonitors() would 
>>> simplify the test.
>>> Why do we need the testClass to be volatile and global if it is used 
>>> only in the context of verification?
>>> It generates less questions if it is local.
>>> We could attempt to fix the GetOwnedMonitorInfoTest as well if we 
>>> want this kind of consistency.
>> I think in this test you could make looking up testClass local to 
>> verifyOwnedMonitors() since calling it is under our control. In 
>> GetOwnedMonitorInfoTest, there is no place to safely make it local, 
>> because we need testClass during a JVMTI callback, and we can't 
>> always correctly look up TEST_CLASS in the callback. That's why the 
>> lookup was moved as part of 8191229 into JNI_OnLoad.
>
> Good explanation, thanks!
> So, we do not need a consistency with the GetOwnedMonitorInfoTest in 
> such a case.
>
> Thanks,
> Serguei
>
>> thanks,
>>
>> Chris
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>> Chris
>>>>>  200 fprintf(stderr, "VerifyOwnedMonitors: FAIL: 
>>>>> stackDepthInfo[0].stack_depth should be 1.\n");
>>>>>
>>>>>  207         fprintf(stderr, "VerifyOwnedMonitors: FAIL: 
>>>>> monitorCount should be 1.\n");
>>>>>
>>>>>
>>>>> It'd better to rephrase the messages above to tell about actual 
>>>>> values vs expected.
>>>>> It normally simplifies the analysis of failures as there is no 
>>>>> need to find
>>>>> what values were printed before and that they are exactly what 
>>>>> needed for comparison.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>>
>>>>> On 1/11/18 17:45, Daniil Titov wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the following fix that adds a jtreg test for 
>>>>>> GetOwnedMonitorStackDepthInfo JVMTI function.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153629
>>>>>> Webrev: http://cr.openjdk.java.net/~dtitov/8153629/webrev.00
>>>>>>
>>>>>>
>>>>>> The tests ran successfully with Mach5.
>>>>>>
>>>>>> Best regards,
>>>>>> Daniil
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>