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.ti...@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-boun...@openjdk.java.net> on > behalf of Daniil Titov <daniil.x.ti...@oracle.com> > Date: Monday, August 27, 2018 at 11:05 AM > To: JC Beyler <jcbey...@google.com>, <serguei.spit...@oracle.com> > Cc: <serviceability-dev@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 <jcbey...@google.com> > Date: Monday, August 27, 2018 at 10:41 AM > To: <serguei.spit...@oracle.com> > Cc: <daniil.x.ti...@oracle.com>, <serviceability-dev@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.spit...@oracle.com <mailto: > serguei.spit...@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.ti...@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
