On 1/12/18 15:14, serguei.spit...@oracle.com wrote:
On 1/12/18 15:07, Chris Plummer wrote:
On 1/12/18 2:52 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
On 1/12/18 14:31, Chris Plummer wrote:
Hi Serguei,
On 1/12/18 2:25 PM, serguei.spit...@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
