ok, then. I see now that the same concern has been already raised and discussed in this thread.
-- Igor > On Aug 27, 2018, at 8:13 PM, serguei.spit...@oracle.com wrote: > > 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.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 >>>> >>>> >> >
