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

JC Beyler jcbeyler at google.com
Tue Aug 28 01:00:56 UTC 2018


Hi Daniil,

(Not a reviewer for reference) but looks good to me :)

Thanks for fixing the comments!
Jc

On Mon, 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
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180827/8a962864/attachment.html>


More information about the serviceability-dev mailing list