8209585: [Graal] vmTestbase/nsk/jvmti/scenarios/sampling tests fail with "Too small stack of resumed thread"
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
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
>>>
>>>
>
More information about the hotspot-compiler-dev
mailing list