[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2018-01-17 22:10:00
Message-ID: d08c2a9c-83ec-42b3-9829-9fe61089ca98 () oracle ! com
[Download RAW message or body]

Hi Daniil,

Looks good. Thanks for the -Xcomp testing.

Chris

On 1/17/18 1:36 PM, 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@oracle.com" <serguei.spitsyn@oracle.com> \
> wrote: 
> On 1/17/18 12:21, serguei.spitsyn@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@oracle.com"
> > > > <serguei.spitsyn@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
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > 
> > 
> 
> 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]