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

Tue Aug 28 03:13:48 UTC 2018

Totally agree.

Thanks,
Serguei

On 8/27/18 19:14, Chris Plummer wrote:
> That was my first reaction when looking at the explanation, but then I 
> realized the test is testing that the stack sizes are the same for two 
> different snapshots of the same */unsuspended/* thread. That sounds 
> like reason enough to make the change, even if graal were not an issue.
>
> thanks,
>
> Chris
>
> On 8/27/18 6:19 PM, Igor Ignatyev wrote:
>> (cc'ing compiler alias)
>>
>> I ain't sure that we should change these tests given that w/ libgraal 
>> (as an opposite to the current way we use graal) we shouldn't see 
>> this issue at all. I'd rather left these tests in the graal-specific 
>> problem list till we switch to libraal.
>>
>> Vladimir, Katja, what do you think?
>>
>> Thanks,
>> -- Igor
>>
>>> On Aug 27, 2018, at 5:57 PM, Daniil Titov 
>>> <daniil.x.titov at oracle.com> 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 PM 
>>> mailto: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
>>>
>>>
>>>     --
>>>   Thanks,
>>> Jc
>>>
>>>
>>>   --
>>>   Thanks,
>>> Jc
>>>
>>>
>