[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8209585: [Graal] vmTestbase/nsk/jvmti/scenarios/sampling tests fail with "Too small stack o
From: JC Beyler <jcbeyler () google ! com>
Date: 2018-08-27 17:40:48
Message-ID: CAF9BGBwitdJma0t928nZFmrNX_SwiJBZ0tHrJ+K+E4Kcthc2Dg () mail ! gmail ! com
[Download RAW message or body]
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 serguei.spitsyn@oracle.com <
serguei.spitsyn@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 <daniil.x.titov@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
[Attachment #3 (text/html)]
<div dir="ltr">Hi Serguei,<br><div><br></div><div>Fair enough, at least this removes \
a bit of the chance of flakiness :-)</div><div><br></div><div>Should we at least \
clean up the comment for methods that are \
changed?</div><div><br></div><div>/**<br></div><div><div> * Testcase: check tested \
threads</div><div> * - invoke getFrameCount() for each thread</div><div> * \
- check if frameCount is not less than minimal stack depth</div><div> * - \
invoke getStackTrace() for each thread</div><div> * - check if stack depth is \
not less than frameCount</div><div> * - for suspended thread check if stack \
depth is equal to frameCount</div><div> *</div><div> * Returns NSK_TRUE if test may \
continue; or NSK_FALSE for test break.</div><div> */</div><div>static int \
checkThreads(int suspended, const char* kind) {</div></div><div><br></div><div>The \
" * - check if stack depth is not less than frameCount" is no longer \
done with this webrev.</div><div><br></div><div>Thanks,</div><div>Jc</div><div><br></div></div><br><div \
class="gmail_quote"><div dir="ltr">On Sun, Aug 26, 2018 at 9:52 PM <a \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> <<a \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div class="m_-893941195003187993moz-cite-prefix">Hi Jc,<br>
<br>
Initially, I has the same concern.<br>
But now I think there is no point to take these values on
non-suspended threads.<br>
It has to be good enough to compare the values taken on suspended
threads only.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 8/24/18 16:49, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi Daniil,
<div><br>
</div>
<div>Just my two cents about this :)</div>
<div><br>
</div>
<div>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:</div>
<div>
<div><br>
</div>
<div>- if (frameStackSize < frameCount) {</div>
</div>
<div><br>
</div>
<div>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?).</div>
<div><br>
</div>
<div>Alternatively, when we look at this code and the hypothesis
of Graal stacks "slipping in between calls", two cases could
occur:</div>
<div> A) The Graal frames are present in the first call but not
the second</div>
<div> B) The Graal frames are present in the second call but
not the first</div>
<div><br>
</div>
<div>In the (B) case, the test would not trip, as frameStackSize
would be >= frameCount so that is not an issue.</div>
<div>In the (A) case, we could simply recall the frameCount and
assure ourselves the frames have disappeared, no?</div>
<div><br>
</div>
<div>Something like:</div>
<div><br>
</div>
<div>
<div> if (frameStackSize < frameCount) {</div>
<div> // This can occur for Graal if graal frames
crept in. Call getFrameCount again and see if they have
disappeared since</div>
<div> // frameStackSize seems to say so.</div>
<div> ... insert call here and a new check...</div>
<div><br>
</div>
<div> NSK_COMPLAIN5("Too small stack of %s thread
#%d (%s):\n"</div>
<div> "# getStackTrace(): \
%d\n"</div>
<div> "# getFrameCount(): \
%d\n",</div> <div> kind, i,
threadsDesc[i].threadName,</div>
<div> (int)frameStackSize,
(int)frameCount);</div>
<div> nsk_jvmti_setFailStatus();</div>
<div> }</div>
<div><br>
</div>
</div>
<div>Just my 2 cents because I worry about simplifying a test
for Graal but losing coverage in the general case,</div>
<div>Jc</div>
<div><br>
</div>
<div><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Thu, Aug 23, 2018 at 8:29 PM Daniil Titov
<<a href="mailto:daniil.x.titov@oracle.com" \
target="_blank">daniil.x.titov@oracle.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote">Please review the change that
fixes 4 JVMTI tests when running with Graal.<br>
<br>
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.<br>
<br>
However, with Graal on, additional "adjustCompilationLevel"
frames could appear on the stack trace, e.g.:<br>
<br>
adjustCompilationLevel:166, HotSpotGraalCompilerFactory
(org.graalvm.compiler.hotspot)<br>
adjustCompilationLevel:504, HotSpotJVMCIRuntime
(jdk.vm.ci.hotspot)<br>
testedMethod:56, Test$Runner<br>
run:67, Test$Runner<br>
<br>
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.<br>
<br>
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.<br>
<br>
Issue: <a href="https://bugs.openjdk.java.net/browse/JDK-8209585" \
rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8209585</a> \
<br> Webrev: <a href="http://cr.openjdk.java.net/%7Edtitov/8209585/webrev.01" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~dtitov/8209585/webrev.01</a><br>
<br>
Thanks,<br>
Daniil<br>
<br>
<br>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr" \
class="m_-893941195003187993gmail-m_7608656757592986711gmail_signature"> <div \
dir="ltr"> <div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" \
class="gmail_signature" data-smartmail="gmail_signature"><div \
dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic