RFR 8153629: Need to cover JVMTI's GetOwnedMonitorStackDepthInfo function

[Daniil Titov]

Thank you, Chris and Serguei!

I will extend the test as suggested and send an updated patch for review.

Best regards,
Daniil

On 1/12/18, 2:52 PM, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:

    Hi Chris,
    
    
    On 1/12/18 14:31, Chris Plummer wrote:
    > Hi Serguei,
    >
    > On 1/12/18 2:25 PM, 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.
    > 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.
    
    
    >> 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.
    
    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
    >>>
    >>>
    >>
    >
    >