Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails

2019-03-05 Thread serguei . spitsyn

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
 >  >
 >  >
 >
 >
 >
 >
 >
 
 







Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails

2019-03-05 Thread Daniil Titov
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: 8218464: vmTestbase/nsk/jdi/VirtualMachine/allThreads/allthreads001/TestDescription.java failed

2019-03-05 Thread serguei . spitsyn

Hi Daniil,

The fix looks good.

Thanks,
Serguei


On 3/5/19 1:59 PM, Daniil Titov wrote:

Please review a fix for this test that intermittently fails with Graal on.

The problem here is that the test does two consequent calls to 
VirtualMachine.allThreads() and checks that the number of threads returned by 
these calls is the same. With Graal on there is a chance that a new JVMCI 
compiler thread could be started between these calls that makes test to fail. 
The fix temporary suspends the debuggee VM while these two calls to 
VirtualMachine.allThreads() are made.

Webrev: http://cr.openjdk.java.net/~dtitov/8218464/webrev.01
Bug: https://bugs.openjdk.java.net/browse/JDK-8218464

Thanks!
--Daniil







Re: RFR:8219721: jcmd from earlier release will hang attaching to VM with JDK-8215622 applied

2019-03-05 Thread serguei . spitsyn

Hi Lin,

It looks good to me too.

Thanks,
Serguei

On 3/4/19 1:13 AM, 臧琳 wrote:

Dear All,
May I ask your help to review the fix of compatibility issue introduced by 
JDK-8215622
webrev:  http://cr.openjdk.java.net/~xiaofeya/8219721/webrev.01/
Bug:  https://bugs.openjdk.java.net/browse/JDK-8219721

FYI, this webrev is forked from the discussion 
https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-February/027240.html.

Thanks,
Lin




Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails

2019-03-05 Thread serguei . spitsyn

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
 >
 >
 
 
 







RFR: 8218464: vmTestbase/nsk/jdi/VirtualMachine/allThreads/allthreads001/TestDescription.java failed

2019-03-05 Thread Daniil Titov
Please review a fix for this test that intermittently fails with Graal on.

The problem here is that the test does two consequent calls to 
VirtualMachine.allThreads() and checks that the number of threads returned by 
these calls is the same. With Graal on there is a chance that a new JVMCI 
compiler thread could be started between these calls that makes test to fail. 
The fix temporary suspends the debuggee VM while these two calls to 
VirtualMachine.allThreads() are made.

Webrev: http://cr.openjdk.java.net/~dtitov/8218464/webrev.01
Bug: https://bugs.openjdk.java.net/browse/JDK-8218464

Thanks!
--Daniil





Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails

2019-03-05 Thread Daniil Titov
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(XS) 8220030: JdbStopThreadidTest.java failed due to "Unexpected IO error while writing command 'quit' to jdb stdin stream"

2019-03-05 Thread Alex Menkov

LGTM too.

--alex

On 03/05/2019 04:47, Gary Adams wrote:

This looks OK to me.

If a specific return code is expected,
then the test should wait until it gets the response it needs.

If a specific return is not required, the test should be lenient
about what information is required for the test to complete.

On 3/4/19, 10:41 PM, Chris Plummer wrote:

Hello,

Please review the following. Details are in the bug. In short, the way 
I was checking for the application exit was a bit non-standard and 
exposed what is probably a bug in the JdbTest shutdown code. I changed 
the test to conform to the way other test run to exit:


https://bugs.openjdk.java.net/browse/JDK-8220030

diff --git a/test/jdk/com/sun/jdi/JdbStopThreadidTest.java 
b/test/jdk/com/sun/jdi/JdbStopThreadidTest.java

--- a/test/jdk/com/sun/jdi/JdbStopThreadidTest.java
+++ b/test/jdk/com/sun/jdi/JdbStopThreadidTest.java
@@ -32,6 +32,7 @@
  * @run main/othervm JdbStopThreadidTest
  */

+import jdk.test.lib.process.OutputAnalyzer;
 import lib.jdb.Jdb;
 import lib.jdb.JdbCommand;
 import lib.jdb.JdbTest;
@@ -138,6 +139,7 @@
 jdb.command(JdbCommand.cont().waitForPrompt("Breakpoint hit: 
\"thread=MYTHREAD-2\", \\S+MyThread.brkMethod", true));
 // Continue until the application exits. Once again, hitting 
a breakpoint will cause

 // a failure because we are not suppose to hit one.
- jdb.command(JdbCommand.cont().waitForPrompt(Jdb.APPLICATION_EXIT, 
true));

+    jdb.contToExit(1);
+    new 
OutputAnalyzer(getJdbOutput()).shouldContain(Jdb.APPLICATION_EXIT);


thanks,

Chris






Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

2019-03-05 Thread Severin Gehwolf
Hi Daniel,

Latest webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/03/webrev/

It's incorporating your changes wrt. to some timeouts and asserting the
expected exit code. Instead of the ProcessThread changes, I'm using the
sendMessage() approach. That API is already there.

Unfortunately when running both SSL and plain sockets tests, it would
randomly fail for me. Even if I choose different sets of ports for
plain/ssl. So I've taken a different route of randomly selecting SSL or
plain. Overall, this should give reasonable coverage for both (plain
and SSL). This made test stability improve a lot on my Linux x86_64
machine. Ran for 100 iterations without failure.

I'll run this through jdk/submit now. Could you run this through your
test system too, please?

Would you be OK with getting this patch pushed once we'd have positive
results for both?

Thanks,
Severin

On Fri, 2019-03-01 at 15:08 +, Daniel Fuchs wrote:
> Hi Severin,
> 
> On 28/02/2019 15:47, Severin Gehwolf wrote:
> > Yes, thanks for looking at this Daniel. That was my determination as
> > well. However, even if we do all of the above, and add a test config
> > with /etc/hosts mapping a non-loopback address to "localhost" it would
> > break other tests. E.g. this one:
> > java/net/InetAddress/GetLocalHostWithSM.java
> > 
> > So I'd have to explore whether your suggestion with
> > InetAddress.getLocalHost() is viable. It seems promising.
> > 
> > I'll keep you posted.
> 
> Thanks for keeping the investigation going!
> 
> For what it's worth this is what I have been experimenting with:
> http://cr.openjdk.java.net/~dfuchs/experiment-8219585/experiment.00/
> 
> It's only a POC and obviously need more cleaning.
> Maybe some of the arbitrary timeouts in the test could be scaled
> in accordance to timeout-factor (I think there's an adjustTimeout(long)
> function somewhere in the test libs that does that).
> 
> I ran it 50 times in our test system - and it passed on all platforms,
> so there's yet hope :-)
> 
> hope this helps,
> 
> -- daniel
> 
> 



Re: RFR:8219721: jcmd from earlier release will hang attaching to VM with JDK-8215622 applied

2019-03-05 Thread Yasumasa Suenaga

Looks good.


Yasumasa (ysuenaga)


On 2019/03/04 18:13, zangl...@jd.com wrote:

Dear All,
May I ask your help to review the fix of compatibility issue introduced by 
JDK-8215622
webrev:  http://cr.openjdk.java.net/~xiaofeya/8219721/webrev.01/
Bug:  https://bugs.openjdk.java.net/browse/JDK-8219721

FYI, this webrev is forked from the discussion 
https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-February/027240.html.

Thanks,
Lin



Re: RFR(XS) 8220030: JdbStopThreadidTest.java failed due to "Unexpected IO error while writing command 'quit' to jdb stdin stream"

2019-03-05 Thread Gary Adams

This looks OK to me.

If a specific return code is expected,
then the test should wait until it gets the response it needs.

If a specific return is not required, the test should be lenient
about what information is required for the test to complete.

On 3/4/19, 10:41 PM, Chris Plummer wrote:

Hello,

Please review the following. Details are in the bug. In short, the way 
I was checking for the application exit was a bit non-standard and 
exposed what is probably a bug in the JdbTest shutdown code. I changed 
the test to conform to the way other test run to exit:


https://bugs.openjdk.java.net/browse/JDK-8220030

diff --git a/test/jdk/com/sun/jdi/JdbStopThreadidTest.java 
b/test/jdk/com/sun/jdi/JdbStopThreadidTest.java

--- a/test/jdk/com/sun/jdi/JdbStopThreadidTest.java
+++ b/test/jdk/com/sun/jdi/JdbStopThreadidTest.java
@@ -32,6 +32,7 @@
  * @run main/othervm JdbStopThreadidTest
  */

+import jdk.test.lib.process.OutputAnalyzer;
 import lib.jdb.Jdb;
 import lib.jdb.JdbCommand;
 import lib.jdb.JdbTest;
@@ -138,6 +139,7 @@
 jdb.command(JdbCommand.cont().waitForPrompt("Breakpoint hit: 
\"thread=MYTHREAD-2\", \\S+MyThread.brkMethod", true));
 // Continue until the application exits. Once again, hitting 
a breakpoint will cause

 // a failure because we are not suppose to hit one.
- jdb.command(JdbCommand.cont().waitForPrompt(Jdb.APPLICATION_EXIT, 
true));

+jdb.contToExit(1);
+new 
OutputAnalyzer(getJdbOutput()).shouldContain(Jdb.APPLICATION_EXIT);


thanks,

Chris






RE: RFR:8219721: jcmd from earlier release will hang attaching to VM with JDK-8215622 applied

2019-03-05 Thread 臧琳

Hi David, 
Thanks a lot for your review. 

Hi All, 
May I get more review about this patch, it is a trivial patch to fix 
compatibility issue. 

Thanks, 
Lin


> -Original Message-
> From: David Holmes 
> Sent: Tuesday, March 5, 2019 2:12 PM
> To: 臧琳 ; serviceability-dev@openjdk.java.net
> serviceability-dev@openjdk.java.net 
> Subject: Re: RFR:8219721: jcmd from earlier release will hang attaching to VM
> with JDK-8215622 applied
> 
> Hi Lin,
> 
> On 4/03/2019 7:13 pm, 臧琳 wrote:
> > Dear All,
> > May I ask your help to review the fix of compatibility issue introduced 
> > by
> JDK-8215622
> > webrev:  http://cr.openjdk.java.net/~xiaofeya/8219721/webrev.01/
> > Bug:  https://bugs.openjdk.java.net/browse/JDK-8219721
> >
> > FYI, this webrev is forked from the discussion
> https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-
> February/027240.html.
> 
> This looks good to me. I can confirm that it fixes the problem of tools from
> earlier releases hanging.
> 
> Thanks,
> David
> 
> > Thanks,
> > Lin
> >