RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

[serguei.spitsyn at oracle.com]

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     }


>>>> 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
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>