RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Jan 17 21:49:52 UTC 2018
Daniil,
Looks great!
Could you, please, make a minor cleanup of status handling?
60 jint status = FAILED;
=>
60 jint status = PASSED;
At the lines 167, 173, 179, 186 and 192:
return status;
=>
return FAILED;
And remove the line:
205 status = PASSED;
No need for another round of review.
Thanks,
Serguei
On 1/17/18 13:36, Daniil Titov wrote:
> Thank you, Serguei and Chris!
>
> The test runs successfully in mach5 when –Xcomp Java option is added. Please review a new version of patch that changes the way how the error messages are printed as suggested.
>
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8153629/webrev.03/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8153629
>
> Best regards,
> Daniil
>
>
> On 1/17/18, 12:29 PM, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:
>
> On 1/17/18 12:21, serguei.spitsyn at oracle.com wrote:
> > 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?
>
> Please, skip this last question.
> I see the reason two have two different classes.
> It is in the suggestion above this question.
>
> Thanks,
> Serguei
>
>
> > 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.spitsyn at oracle.com"
> >>> <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.
> >>> 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
> >>> >
> >>> >
> >>>
> >>>
> >>
> >
>
>
>
>
More information about the serviceability-dev
mailing list