8209585: [Graal] vmTestbase/nsk/jvmti/scenarios/sampling tests fail with "Too small stack of resumed thread"

Chris Plummer chris.plummer at oracle.com
Fri Aug 31 16:24:51 UTC 2018


+2

Chris

On 8/30/18 4:52 PM, Alex Menkov wrote:
> +1
>
> --alex
>
> On 08/30/2018 15:43, serguei.spitsyn at oracle.com wrote:
>> Hi Daniil,
>>
>> Thank you a lot for this extra update and cleanup!
>> I feels that it has a real value in improving the tests reliability.
>>
>> Some minor comments:
>>
>> http://cr.openjdk.java.net/~dtitov/8209585/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t001/TestDescription.java.udiff.html 
>>
>>
>>    I'd suggest to update the fragment:
>>        * Checked statements:
>>    to:
>>        * Checked statements for suspended threads:
>>
>>    A suggestion to remove the dot in the original comment:
>>
>>        *           are not less than expected minimal stack depth.
>>
>>
>> http://cr.openjdk.java.net/~dtitov/8209585/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t002/sp02t002.c.udiff.html 
>>
>>
>>    Two typos are inherited taken from the original comment (numbers 
>> => number, frame => frames):
>>
>> - * - compare numbers of stack frame returned
>> - * - find stck frane with expected methodID
>> + * - for suspended thread compare numbers of stack frame returned
>> + * - find stack frame with expected methodID
>>
>>
>> http://cr.openjdk.java.net/~dtitov/8209585/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t001/TestDescription.java.udiff.html 
>>
>>
>>    It is nice you fixed several pre-existed typos in the comment.
>>
>>    I'd suggest to update the fragment:
>>        * Checked statements:
>>    to:
>>        * Checked statements for suspended threads:
>>
>>    The dot at the end is not needed:
>>
>> + * are not less than expected minimal stack depth.
>>
>>
>> http://cr.openjdk.java.net/~dtitov/8209585/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t002/TestDescription.java.udiff.html 
>>
>>
>>    Looks good.
>>
>>
>> http://cr.openjdk.java.net/~dtitov/8209585/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t002/sp06t002.c.udiff.html 
>>
>>
>>    Two typos are inherited taken from the original comment (numbers 
>> => number, frame => frames):
>>
>> - * - compare numbers of stack frame returned
>> - * - find stck frane with expected methodID
>> + * - for suspended thread compare numbers of stack frame returned
>> + * - find stack frame with expected methodID
>>
>>
>> No updated webrev is needed if you fix the above.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 8/30/18 14:28, Daniil Titov wrote:
>>> Hi Serguei,
>>>
>>> Please review a new version of the webrev that has the suggested 
>>> changes applied for two tests:
>>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t001/TestDescription.java 
>>> and 
>>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t001/TestDescription.java.
>>>
>>> The other two tests 
>>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t002/TestDescription.java 
>>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t002/TestDescription.java 
>>> use stack trace to check that the list of stack frames includes the 
>>> frame for the tested method so I left them unchanged ( they still do 
>>> test both running and suspended threads).
>>>
>>> I also fixed the comments to make them relevant to what the tests 
>>> actually do.
>>>
>>> Webrev:http://cr.openjdk.java.net/~dtitov/8209585/webrev.03/
>>> Issue:https://bugs.openjdk.java.net/browse/JDK-8209585
>>>
>>> Thanks!
>>> Daniil
>>>
>>>
>>> On 8/27/18, 8:29 PM,"serguei.spitsyn at oracle.com" 
>>> <serguei.spitsyn at oracle.com>  wrote:
>>>
>>>      Hi Daniil and Jc,
>>>                I'm thinking if it make sense to call checkThreads() 
>>> when the threads
>>>      are not suspended.
>>>      In fact, it does not check much for non-suspended threads now.
>>>           As an example, consider the test:
>>> http://cr.openjdk.java.net/~dtitov/8209585/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t001/sp02t001.c.frames.html
>>>           The call to JVMTI GetStackTrace() for non-suspended case 
>>> is not really
>>>      needed.
>>>      It is only to print the frameStackSize value:
>>>             275         NSK_DISPLAY1("    stack depth: %d\n", 
>>> (int)frameStackSize);
>>>                The only real check left is:
>>>             277         /* check frame count */
>>>        278         if (frameCount < threadsDesc[i].minDepth) {
>>>        279             NSK_COMPLAIN5("Too few frameCount of %s 
>>> thread #%d (%s):\n"
>>>        280                             "#   got frameCount: %d\n"
>>>        281                             "#   expected minimum: %d\n",
>>>        282                             kind, i, 
>>> threadsDesc[i].threadName,
>>>        283                             (int)frameCount, 
>>> threadsDesc[i].minDepth);
>>>        284             nsk_jvmti_setFailStatus();
>>>        285         }
>>>           I don't think this check has a real value for 
>>> non-suspended case.
>>>      A real value for us is simplicity and reliability.
>>>           My suggestion is to get rid of "suspended" parameter and 
>>> all non-suspended call of the checkThreads.
>>>      But, please, note, the comments will need another update. :)
>>>           Jc, nice catch about the comments!
>>>                Thanks,
>>>      Serguei
>>>                     On 8/27/18 17:57, Daniil Titov wrote:
>>>      > Hi JC, Serguei, and Alex,
>>>      >
>>>      > Please review an updated version of the webrev that has these 
>>> comments fixed.
>>>      >
>>>      > Webrev:http://cr.openjdk.java.net/~dtitov/8209585/webrev.02/
>>>      > Issue:https://bugs.openjdk.java.net/browse/JDK-8209585
>>>      >
>>>      > Thanks!
>>>      > Daniil
>>>      >
>>>      >
>>>      >
>>>      > From: 
>>> serviceability-dev<serviceability-dev-bounces at openjdk.java.net> on 
>>> behalf of Daniil Titov<daniil.x.titov at oracle.com>
>>>      > Date: Monday, August 27, 2018 at 11:05 AM
>>>      > To: JC Beyler<jcbeyler at google.com>,<serguei.spitsyn at oracle.com>
>>>      > Cc:<serviceability-dev at openjdk.java.net>
>>>      > Subject: Re: 8209585: [Graal] 
>>> vmTestbase/nsk/jvmti/scenarios/sampling tests fail with "Too small 
>>> stack of resumed thread"
>>>      >
>>>      > Hi Jc,
>>>      >
>>>      > Thank you for spotting this!  I will send on review an 
>>> updated webrev with these comments fixed.
>>>      >
>>>      > Best regards,
>>>      > Daniil
>>>      >
>>>      > From: JC Beyler<jcbeyler at google.com>
>>>      > Date: Monday, August 27, 2018 at 10:41 AM
>>>      > To:<serguei.spitsyn at oracle.com>
>>>      > 
>>> Cc:<daniil.x.titov at oracle.com>,<serviceability-dev at openjdk.java.net>
>>>      > Subject: Re: RFR: 8209585: [Graal] 
>>> vmTestbase/nsk/jvmti/scenarios/sampling tests fail with "Too small 
>>> stack of resumed thread"
>>>      >
>>>      > Hi Serguei,
>>>      >
>>>      > Fair enough, at least this removes a bit of the chance of 
>>> flakiness :-)
>>>      >
>>>      > Should we at least clean up the comment for methods that are 
>>> changed?
>>>      >
>>>      > /**
>>>      >   * Testcase: check tested threads
>>>      >   *    - invoke getFrameCount() for each thread
>>>      >   *    - check if frameCount is not less than minimal stack 
>>> depth
>>>      >   *    - invoke getStackTrace() for each thread
>>>      >   *    - check if stack depth is not less than frameCount
>>>      >   *    - for suspended thread check if stack depth is equal 
>>> to frameCount
>>>      >   *
>>>      >   * Returns NSK_TRUE if test may continue; or NSK_FALSE for 
>>> test break.
>>>      >   */
>>>      > static int checkThreads(int suspended, const char* kind) {
>>>      >
>>>      > The "  *    - check if stack depth is not less than 
>>> frameCount" is no longer done with this webrev.
>>>      >
>>>      > Thanks,
>>>      > Jc
>>>      >
>>>      >
>>>      > On Sun, Aug 26, 2018 at 9:52 
>>> PMmailto:serguei.spitsyn at oracle.com 
>>> <mailto:serguei.spitsyn at oracle.com>  wrote:
>>>      > Hi Jc,
>>>      >
>>>      > Initially, I has the same concern.
>>>      > But now I think there is no point to take these values on 
>>> non-suspended threads.
>>>      > It has to be good enough to compare the values taken on 
>>> suspended threads only.
>>>      >
>>>      > Thanks,
>>>      > Serguei
>>>      >
>>>      >
>>>      > On 8/24/18 16:49, JC Beyler wrote:
>>>      > Hi Daniil,
>>>      >
>>>      > Just my two cents about this :)
>>>      >
>>>      > I was looking at this and wondered if it made sense to fix 
>>> the test this way (I always worry about simplifying a test and 
>>> losing coverage). I understand the bug is that it is possible that 
>>> between both calls, Graal could add some frames and therefore might 
>>> trip this test:
>>>      >
>>>      > -        if (frameStackSize < frameCount) {
>>>      >
>>>      > However, by removing the test altogether and only relying on 
>>> the suspended frames, are we not reducing our coverage of the test 
>>> (basically never really testing the running threads anymore, only 
>>> the suspended ones?).
>>>      >
>>>      > Alternatively, when we look at this code and the hypothesis 
>>> of Graal stacks "slipping in between calls", two cases could occur:
>>>      >    A) The Graal frames are present in the first call but not 
>>> the second
>>>      >    B) The Graal frames are present in the second call but not 
>>> the first
>>>      >
>>>      > In the (B) case, the test would not trip, as frameStackSize 
>>> would be >= frameCount so that is not an issue.
>>>      > In the (A) case, we could simply recall the frameCount and 
>>> assure ourselves the frames have disappeared, no?
>>>      >
>>>      > Something like:
>>>      >
>>>      >          if (frameStackSize < frameCount) {
>>>      >             // This can occur for Graal if graal frames crept 
>>> in. Call getFrameCount again and see if they have disappeared since
>>>      >            // frameStackSize seems to say so.
>>>      >             ... insert call here and a new check...
>>>      >
>>>      >              NSK_COMPLAIN5("Too small stack of %s thread #%d 
>>> (%s):\n"
>>>      >                              "#   getStackTrace(): %d\n"
>>>      >                              "#   getFrameCount(): %d\n",
>>>      >                              kind, i, threadsDesc[i].threadName,
>>>      >                              (int)frameStackSize, 
>>> (int)frameCount);
>>>      >              nsk_jvmti_setFailStatus();
>>>      >          }
>>>      >
>>>      > Just my 2 cents because I worry about simplifying a test for 
>>> Graal but losing coverage in the general case,
>>>      > Jc
>>>      >
>>>      >
>>>      >
>>>      > On Thu, Aug 23, 2018 at 8:29 PM Daniil 
>>> Titov<mailto:daniil.x.titov at oracle.com>  wrote:
>>>      > Please review the change that fixes 4 JVMTI tests when 
>>> running with Graal.
>>>      >
>>>      > One of the checks these tests perform compares the number of 
>>> frames in the thread's stack returned by JVMTI GetFrameCount() with 
>>> the number of frames entries returned by JVMTI GetStackTrace(). The 
>>> thread to be tested executes arithmetic operations in the loop so 
>>> the consequent calls of GetFrameCount() or/and  GetStackTrace() 
>>> should return the stack trace of the same depth.
>>>      >
>>>      > However,  with Graal on, additional "adjustCompilationLevel" 
>>> frames could appear on the stack trace, e.g.:
>>>      >
>>>      > adjustCompilationLevel:166, HotSpotGraalCompilerFactory 
>>> (org.graalvm.compiler.hotspot)
>>>      > adjustCompilationLevel:504, HotSpotJVMCIRuntime 
>>> (jdk.vm.ci.hotspot)
>>>      > testedMethod:56, Test$Runner
>>>      > run:67, Test$Runner
>>>      >
>>>      > that results in the stack depth reported by the first 
>>> invocation of GetFrameCount() or GetStackTrace() might differ from 
>>> the stack depth reported by the consequent calls of the same methods.
>>>      >
>>>      > The fix modifies the tests to ensure the check that 
>>> GetFrameCount () and GetStackTrace() report the same stack depth is 
>>> performed only if the thread is suspended. For two tests 
>>> (vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t001/TestDescription.java 
>>> and 
>>> vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t001/TestDescription.java) 
>>> such check for suspended threads already exists so in these tests 
>>> the problematic check was not modified by just removed.
>>>      >
>>>      > Issue:https://bugs.openjdk.java.net/browse/JDK-8209585
>>>      > Webrev:http://cr.openjdk.java.net/~dtitov/8209585/webrev.01
>>>      >
>>>      > Thanks,
>>>      > Daniil
>>>      >
>>>      >
>>>      >
>>>      >
>>>
>>>
>>



More information about the serviceability-dev mailing list