Hi Serguei,
Please review a new version of the webrev that has the suggested
changes applied for two tests:
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t001/TestDescription.java
and
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t001/TestDescription.java.
The other two tests
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t002/TestDescription.java
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t002/TestDescription.java
use stack trace to check that the list of stack frames includes the
frame for the tested method so I left them unchanged ( they still do
test both running and suspended threads).
I also fixed the comments to make them relevant to what the tests
actually do.
Webrev:http://cr.openjdk.java.net/~dtitov/8209585/webrev.03/
Issue:https://bugs.openjdk.java.net/browse/JDK-8209585
Thanks!
Daniil
On 8/27/18, 8:29 PM,"serguei.spit...@oracle.com"
<serguei.spit...@oracle.com> wrote:
Hi Daniil and Jc,
I'm thinking if it make sense to call checkThreads()
when the threads
are not suspended.
In fact, it does not check much for non-suspended threads now.
As an example, consider the test:
http://cr.openjdk.java.net/~dtitov/8209585/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP02/sp02t001/sp02t001.c.frames.html
The call to JVMTI GetStackTrace() for non-suspended case
is not really
needed.
It is only to print the frameStackSize value:
275 NSK_DISPLAY1(" stack depth: %d\n",
(int)frameStackSize);
The only real check left is:
277 /* check frame count */
278 if (frameCount < threadsDesc[i].minDepth) {
279 NSK_COMPLAIN5("Too few frameCount of %s
thread #%d (%s):\n"
280 "# got frameCount: %d\n"
281 "# expected minimum: %d\n",
282 kind, i,
threadsDesc[i].threadName,
283 (int)frameCount,
threadsDesc[i].minDepth);
284 nsk_jvmti_setFailStatus();
285 }
I don't think this check has a real value for
non-suspended case.
A real value for us is simplicity and reliability.
My suggestion is to get rid of "suspended" parameter and
all non-suspended call of the checkThreads.
But, please, note, the comments will need another update. :)
Jc, nice catch about the comments!
Thanks,
Serguei
On 8/27/18 17:57, Daniil Titov 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
PMmailto: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
>
>
>
>
