Hi Daniil,
Some nits.
207 if (monitorCount == 3) {
208 for (idx = 0; idx < 3; idx++) {
Need to get rid of hardcoded number of monitors (3).
184 err = (*jvmti) -> GetCurrentThread(jvmti, &thread);
Extra spaces around '->'.
214 TEST_CLASS,
TEST_OBJECT_LOCK_DEPTH,stackDepthInfo[idx].stack_depth);
221 LOCK1_CLASS,
LOCK1_DEPTH,stackDepthInfo[idx].stack_depth);
Missed a space before stackDepthInfo[...
Also, I have more suggestions to add to the Chris's comment:
- Remove the big condition if (monitorCount == 3) around the loop and
replace it with:
for (idx = 0; idx < monitorCount; idx++) {
and print monitor miscount separately before of after the loop:
if (monitorCount != 3) {
fprintf(stderr, "VerifyOwnedMonitors: FAIL: invalid
monitorCount, expected: %d, found: %d.\n",
3, monitorCount);
}
Then it would look like this:
status = PASSED;
if (monitorCount != EXP_MON_COUNT) {
fprintf(stderr, "VerifyOwnedMonitors: FAIL: invalid monitorCount, expected:
%d, found: %d.\n",
EXP_MON_COUNT, monitorCount);
status = FAILED;
}
for (idx = 0; idx < monitorCount; idx++) {
if (CheckLockObject(env, stackDepthInfo[idx].monitor, testClass) ==
JNI_TRUE) {
if (stackDepthInfo[idx].stack_depth != TEST_OBJECT_LOCK_DEPTH) {
fprintf(stderr, "FAILED: invalid stack_depth for %s monitor,
expected: %d, found: %d.\n",
TEST_CLASS, TEST_OBJECT_LOCK_DEPTH,
stackDepthInfo[idx].stack_depth);
status = FAILED;
}
} else if (CheckLockObject(env, stackDepthInfo[idx].monitor,
lock1Class) == JNI_TRUE) {
if (stackDepthInfo[idx].stack_depth != LOCK1_DEPTH) {
fprintf(stderr, "FAILED: invalid stack_depth for %s monitor,
expected: %d, found: %d.\n",
LOCK1_CLASS,
LOCK1_DEPTH,stackDepthInfo[idx].stack_depth);
status = FAILED;
}
} else if (CheckLockObject(env, stackDepthInfo[idx].monitor,
lock2Class) == JNI_TRUE) {
if (stackDepthInfo[idx].stack_depth != LOCK2_DEPTH) {
fprintf(stderr, "FAILED: invalid stack_depth for %s monitor,
expected: %d, found: %d.\n",
LOCK2_CLASS, LOCK2_DEPTH,
stackDepthInfo[idx].stack_depth);
status = FAILED;
}
} else {
fprintf(stderr, "FAILED: monitor[%d] should be instance of %s, %s, or
%s\n",
idx, TEST_CLASS, LOCK1_CLASS, LOCK2_CLASS);
status = FAILED;
}
}
Also, it does not seem that important to have two different lock classes Lock1
and Lock2.
Would it be simpler to keep just one of them?
Thanks,
Serguei
On 1/17/18 11:29, Chris Plummer wrote:
Hi Daniil,
Have you run with -Xcomp. I'm concerned that inlining will cause the
test to fail because stack depths will not be as expected. Also, if
you find the owned monitor, but the depth is not as expected, I think
you are better off printing an error message right away rather than
the somewhat misleading "FAIL: invalid number of %s monitors" message
later on.
thanks,
Chris
On 1/16/18 6:31 PM, Daniil Titov wrote:
Hello,
Please review an updated fix that makes the test more extensive and
includes suggested changes.
Bug: https://bugs.openjdk.java.net/browse/JDK-8153629
Webrev: http://cr.openjdk.java.net/~dtitov/8153629/webrev.02/
Thank you!
Best regards,
Daniil
On 1/12/18, 2:26 PM, "serguei.spit...@oracle.com"
<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.
What about moving the FindClass(env, TEST_CLASS) to the
verifyOwnedMonitors() function?
It will make the testClass variable local.
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
>
>
