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