RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: [0] adjustCompilationLevel [1] adjustCompilationLevel [2] testedMethod [3] run In this case the test looks for the test method only in 2 top frames and fails. The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 Thanks! --Daniil
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Looks good, but what about sp06t003? Doesn't it have the same problem? Are there any other tests using similar logic? dl On 3/1/19 8:33 PM, Daniil Titov wrote: Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: [0] adjustCompilationLevel [1] adjustCompilationLevel [2] testedMethod [3] run In this case the test looks for the test method only in 2 top frames and fails. The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 Thanks! --Daniil
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Hi Dean, You are right, test sp06t003 has the same problem. Please, review a new version of the change that fixes both tests. I checked other tests and no more tests use the this approach with "commonDepth". Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.02 Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 Thanks! --Daniil On 3/1/19, 9:14 PM, "serviceability-dev-boun...@openjdk.java.net on behalf of dean.l...@oracle.com" wrote: Looks good, but what about sp06t003? Doesn't it have the same problem? Are there any other tests using similar logic? dl On 3/1/19 8:33 PM, Daniil Titov wrote: > Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. > > The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). > > If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: > > [0] adjustCompilationLevel > [1] adjustCompilationLevel > [2] testedMethod > [3] run > > In this case the test looks for the test method only in 2 top frames and fails. > > The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > Thanks! > --Daniil > >
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Looks good! dl On 3/4/19 3:03 PM, Daniil Titov wrote: Hi Dean, You are right, test sp06t003 has the same problem. Please, review a new version of the change that fixes both tests. I checked other tests and no more tests use the this approach with "commonDepth". Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.02 Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 Thanks! --Daniil On 3/1/19, 9:14 PM, "serviceability-dev-boun...@openjdk.java.net on behalf of dean.l...@oracle.com" wrote: Looks good, but what about sp06t003? Doesn't it have the same problem? Are there any other tests using similar logic? dl On 3/1/19 8:33 PM, Daniil Titov wrote: > Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. > > The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). > > If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: > > [0] adjustCompilationLevel > [1] adjustCompilationLevel > [2] testedMethod > [3] run > > In this case the test looks for the test method only in 2 top frames and fails. > > The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > Thanks! > --Daniil > >
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Hi Daniil, I think the fix is fine. I was wondering why the test was originally written to use the smaller of the two stack sizes. I noticed that the "suspended == NSK_TRUE" checks seem to defend against test failures that might be introduced with your change (you can only get two different stack sizes when the thread is not suspended). This check was added as part of the fix for JDK-8051349, which was done a few months ago, so if the fix for JDK-8051349 were not in place then your fix would not have worked. thanks, Chris On 3/4/19 3:03 PM, Daniil Titov wrote: Hi Dean, You are right, test sp06t003 has the same problem. Please, review a new version of the change that fixes both tests. I checked other tests and no more tests use the this approach with "commonDepth". Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.02 Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 Thanks! --Daniil On 3/1/19, 9:14 PM, "serviceability-dev-boun...@openjdk.java.net on behalf of dean.l...@oracle.com" wrote: Looks good, but what about sp06t003? Doesn't it have the same problem? Are there any other tests using similar logic? dl On 3/1/19 8:33 PM, Daniil Titov wrote: > Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. > > The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). > > If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: > > [0] adjustCompilationLevel > [1] adjustCompilationLevel > [2] testedMethod > [3] run > > In this case the test looks for the test method only in 2 top frames and fails. > > The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > Thanks! > --Daniil > >
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Hi Chris, I also was wondering about what might be behind the decision to use "commonDepth" when the tests were originally written. You are right there were several issues in these tests when running with Graal. The first issue (the different stack sizes returned by jvmti->GetFrameCount and jvmti-> GetStackTrace when Graal is on and the thread is not suspended) was addressed by JDK-8051349 and another issue ( JDK- 8218167, that the test method is not found in the frame stack) is addressed by this change. Best regards, Daniil On 3/4/19, 7:00 PM, "Chris Plummer" wrote: Hi Daniil, I think the fix is fine. I was wondering why the test was originally written to use the smaller of the two stack sizes. I noticed that the "suspended == NSK_TRUE" checks seem to defend against test failures that might be introduced with your change (you can only get two different stack sizes when the thread is not suspended). This check was added as part of the fix for JDK-8051349, which was done a few months ago, so if the fix for JDK-8051349 were not in place then your fix would not have worked. thanks, Chris On 3/4/19 3:03 PM, Daniil Titov wrote: > Hi Dean, > > You are right, test sp06t003 has the same problem. Please, review a new version of the change that fixes both tests. I checked other tests and no more tests use the this approach with "commonDepth". > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.02 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > Thanks! > --Daniil > > On 3/1/19, 9:14 PM, "serviceability-dev-boun...@openjdk.java.net on behalf of dean.l...@oracle.com" wrote: > > Looks good, but what about sp06t003? Doesn't it have the same problem? > Are there any other tests using similar logic? > > dl > > On 3/1/19 8:33 PM, Daniil Titov wrote: > > Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. > > > > The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). > > > > If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: > > > > [0] adjustCompilationLevel > > [1] adjustCompilationLevel > > [2] testedMethod > > [3] run > > > > In this case the test looks for the test method only in 2 top frames and fails. > > > > The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. > > > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 > > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > > > Thanks! > > --Daniil > > > > > > > > >
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Thank you, Dean and Chris, for reviewing this change! --Daniil On 3/4/19, 7:00 PM, "Chris Plummer" wrote: Hi Daniil, I think the fix is fine. I was wondering why the test was originally written to use the smaller of the two stack sizes. I noticed that the "suspended == NSK_TRUE" checks seem to defend against test failures that might be introduced with your change (you can only get two different stack sizes when the thread is not suspended). This check was added as part of the fix for JDK-8051349, which was done a few months ago, so if the fix for JDK-8051349 were not in place then your fix would not have worked. thanks, Chris On 3/4/19 3:03 PM, Daniil Titov wrote: > Hi Dean, > > You are right, test sp06t003 has the same problem. Please, review a new version of the change that fixes both tests. I checked other tests and no more tests use the this approach with "commonDepth". > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.02 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > Thanks! > --Daniil > > On 3/1/19, 9:14 PM, "serviceability-dev-boun...@openjdk.java.net on behalf of dean.l...@oracle.com" wrote: > > Looks good, but what about sp06t003? Doesn't it have the same problem? > Are there any other tests using similar logic? > > dl > > On 3/1/19 8:33 PM, Daniil Titov wrote: > > Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. > > > > The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). > > > > If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: > > > > [0] adjustCompilationLevel > > [1] adjustCompilationLevel > > [2] testedMethod > > [3] run > > > > In this case the test looks for the test method only in 2 top frames and fails. > > > > The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. > > > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 > > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > > > Thanks! > > --Daniil > > > > > > > > >
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Hi Daniil, It looks okay. How did you test this fix? Did you run these tests in different compiler modes? Thanks, Serguei On 3/4/19 3:03 PM, Daniil Titov wrote: Hi Dean, You are right, test sp06t003 has the same problem. Please, review a new version of the change that fixes both tests. I checked other tests and no more tests use the this approach with "commonDepth". Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.02 Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 Thanks! --Daniil On 3/1/19, 9:14 PM, "serviceability-dev-boun...@openjdk.java.net on behalf of dean.l...@oracle.com" wrote: Looks good, but what about sp06t003? Doesn't it have the same problem? Are there any other tests using similar logic? dl On 3/1/19 8:33 PM, Daniil Titov wrote: > Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. > > The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). > > If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: > > [0] adjustCompilationLevel > [1] adjustCompilationLevel > [2] testedMethod > [3] run > > In this case the test looks for the test method only in 2 top frames and fails. > > The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > Thanks! > --Daniil > >
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Hi Serguei, I tested it with different options: -Xcomp and Graal. I also had tier1, tier2 and tier3 tests passed. Bests regards, Daniil On 3/5/19, 2:29 PM, "serguei.spit...@oracle.com" wrote: Hi Daniil, It looks okay. How did you test this fix? Did you run these tests in different compiler modes? Thanks, Serguei On 3/4/19 3:03 PM, Daniil Titov wrote: > Hi Dean, > > You are right, test sp06t003 has the same problem. Please, review a new version of the change that fixes both tests. I checked other tests and no more tests use the this approach with "commonDepth". > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.02 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > Thanks! > --Daniil > > On 3/1/19, 9:14 PM, "serviceability-dev-boun...@openjdk.java.net on behalf of dean.l...@oracle.com" wrote: > > Looks good, but what about sp06t003? Doesn't it have the same problem? > Are there any other tests using similar logic? > > dl > > On 3/1/19 8:33 PM, Daniil Titov wrote: > > Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. > > > > The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). > > > > If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: > > > > [0] adjustCompilationLevel > > [1] adjustCompilationLevel > > [2] testedMethod > > [3] run > > > > In this case the test looks for the test method only in 2 top frames and fails. > > > > The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. > > > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 > > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > > > Thanks! > > --Daniil > > > > > > > > >
Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails
Okay, thanks! Serguei On 3/5/19 2:52 PM, Daniil Titov wrote: Hi Serguei, I tested it with different options: -Xcomp and Graal. I also had tier1, tier2 and tier3 tests passed. Bests regards, Daniil On 3/5/19, 2:29 PM, "serguei.spit...@oracle.com" wrote: Hi Daniil, It looks okay. How did you test this fix? Did you run these tests in different compiler modes? Thanks, Serguei On 3/4/19 3:03 PM, Daniil Titov wrote: > Hi Dean, > > You are right, test sp06t003 has the same problem. Please, review a new version of the change that fixes both tests. I checked other tests and no more tests use the this approach with "commonDepth". > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.02 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > Thanks! > --Daniil > > On 3/1/19, 9:14 PM, "serviceability-dev-boun...@openjdk.java.net on behalf of dean.l...@oracle.com" wrote: > > Looks good, but what about sp06t003? Doesn't it have the same problem? > Are there any other tests using similar logic? > > dl > > On 3/1/19 8:33 PM, Daniil Titov wrote: > > Please review the change that fix intermittent failure for test nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal. > > > > The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and "frameStackSize"( returned by jvmti->GetStackTrace). > > > > If a compilation is triggered between these 2 calls then there are cases when "frameCount" is 2, "frameStackSize" is 4, and the frame stack is as the following: > > > > [0] adjustCompilationLevel > > [1] adjustCompilationLevel > > [2] testedMethod > > [3] run > > > > In this case the test looks for the test method only in 2 top frames and fails. > > > > The fix ensures that the test iterates over all frames in the frame stack when looking for the test method. > > > > Webrev: http://cr.openjdk.java.net/~dtitov/8218167/webrev.01 > > Bug: https://bugs.openjdk.java.net/browse/JDK-8218167 > > > > Thanks! > > --Daniil > > > > > > > > >
