Re: Review request for 8029378: com/sun/jdi/BadHandshakeTest.java failed with java.util.concurrent.TimeoutException
On 16/01/2014 5:37 PM, shanliang wrote: Hi, Please review this simple fix, the test needs more time to wait Phaser.awaitAdvanceInterruptibly(...). Integer.MAX_VALUE? There's no point using a timed form at all. David webrev: http://cr.openjdk.java.net/~sjiang/JDK-8029378/00/ bug: https://bugs.openjdk.java.net/browse/JDK-8029378 Thanks, Shanliang
Re: RFR(L): 8031581: PPC64: Addons and fixes for AIX to pass the jdk regression tests
On Wed, Jan 15, 2014 at 12:05 PM, Volker Simonis volker.simo...@gmail.comwrote: On Wed, Jan 15, 2014 at 10:03 AM, Alan Bateman alan.bate...@oracle.comwrote: On 15/01/2014 06:24, David Holmes wrote: I'm not a fan of runtime checks of this kind though if it is only a very samll number of values it might be okay. Another option would be to make those classes into templates as done with Version.java.template and substitute the right values at build time. But I'll let Alan and net-dev folk come back with their preferred technique for this. I plan to spend time on Volker's webrev later in the week (just too busy with other things right now). For the translation issue then it's an oversight in the original implementation, it just hasn't come up before (to my knowledge anyway). The simplest solution here maybe to to just move them to sun.net.ch.Net and have them initialized to their native value. Do you mean sun.nio.ch.Net right? Do you propose to completely remove the definitions of the POLL constants from: src/share/classes/sun/nio/ch/AbstractPollArrayWrapper.java src/solaris/classes/sun/nio/ch/Port.java and replace all their usages by Net.POLL* ? Hi Alan, I think sun.nio.ch.IOUtil seems even more appropriate to me for these constants. What do you think? Would it be OK for you if I initialize them right in the static initializer of IOUtil based on os.name or do you prefer to have native methods which return the right constants? Regards, Volker In general then I'm not too concerned about that one, it's the changes to support async close on AIX that are leaping out at me. -Alan
Re: RFR(L): 8031581: PPC64: Addons and fixes for AIX to pass the jdk regression tests
On 16/01/2014 09:38, Volker Simonis wrote: Hi Alan, I think sun.nio.ch.IOUtil seems even more appropriate to me for these constants. What do you think? Would it be OK for you if I initialize them right in the static initializer of IOUtil based on os.name http://os.name or do you prefer to have native methods which return the right constants? I have a small preference for sun.nio.ch.Net because these constants are used with Net.poll. Would you be open to separating this one from the AIX changes? The reason is that it isn't AIX specific, rather just an oversight that hasn't been an issue because it doesn't impact other platforms. Using os.name initially would be okay although we could change that over time. -Alan
Re: RFR(L): 8031581: PPC64: Addons and fixes for AIX to pass the jdk regression tests
On Thu, Jan 16, 2014 at 11:05 AM, Alan Bateman alan.bate...@oracle.com wrote: On 16/01/2014 09:38, Volker Simonis wrote: Hi Alan, I think sun.nio.ch.IOUtil seems even more appropriate to me for these constants. What do you think? Would it be OK for you if I initialize them right in the static initializer of IOUtil based on os.name or do you prefer to have native methods which return the right constants? I have a small preference for sun.nio.ch.Net because these constants are used with Net.poll. I just thought because poll is more file-descriptor oriented and not network specific. And the constants are also used for example in: src/macosx/classes/sun/nio/ch/KQueueArrayWrapper.java: src/solaris/classes/sun/nio/ch/sctp/Sctp* src/solaris/classes/sun/nio/ch/Port.java But actually I have no prefernece here so I can put them just as well to sun.nio.ch.Net Would you be open to separating this one from the AIX changes? The reason is that it isn't AIX specific, rather just an oversight that hasn't been an issue because it doesn't impact other platforms. Sure, no problem. Although I still would like to push this to ppc-aix-port/stage-9 and ppc-aix-port/stage first because that's where we really need it. Anyway, the current plan is to merge ppc-aix-port/stage-9 into jdk9 mainline by the end of January and ppc-aix-port/stage into 8u-dev by the end of March (for 8u20). Would that be ok? Using os.name initially would be okay although we could change that over time. I've already written the native methods:) -Alan
Re: Review request for 8029378: com/sun/jdi/BadHandshakeTest.java failed with java.util.concurrent.TimeoutException
David Holmes wrote: On 16/01/2014 5:37 PM, shanliang wrote: Hi, Please review this simple fix, the test needs more time to wait Phaser.awaitAdvanceInterruptibly(...). Integer.MAX_VALUE? There's no point using a timed form at all. David Yes Phaser.awaitAdvanceInterruptibly(int phase) could be used here, but the call of this method is wrapped in: jdk.testlibrary.ProcessTools.startProcess(...) So we have to add a new method ProcessTools.startProcess(...) which has no timeout parameter. I did not do this because I thought to have a simple fix only within the test. If this is useful, here is the new web: http://cr.openjdk.java.net/~sjiang/JDK-8029378/01/ Thanks, Shanliang webrev: http://cr.openjdk.java.net/~sjiang/JDK-8029378/00/ bug: https://bugs.openjdk.java.net/browse/JDK-8029378 Thanks, Shanliang
Re: RFR (S): 7185591: jcmd-big-script.sh ERROR: could not find app's Java pid.
Staffan,
I've missed to rename toolArgs in JcmdBase.java.
The webrev with changes can be found here:
http://cr.openjdk.java.net/~ykantser/7185591/webrev.03
If it looks good, could you please be my sponsor and push the changes?
The patch is attached to this mail.
Thanks,
Katja
On 01/15/2014 03:30 PM, Yekaterina Kantserova wrote:
Staffan,
The webrev with changes can be found here:
http://cr.openjdk.java.net/~ykantser/7185591/webrev.02/
If it looks good, could you please be my sponsor and push the changes?
The patch is attached to this mail.
Thanks,
Katja
On 01/15/2014 11:37 AM, Staffan Larsen wrote:
Katja,
test/lib/testlibrary/jdk/testlibrary/JcmdBase.java
The toolArgs parameter should be renamed jcmdArgs.
test/lib/testlibrary/jdk/testlibrary/OutputAnalyzer.java
423 public int indexOf(String pattern) {
You don’t have to convert the List to an array before iterating
through it. Use lLines.get(i).matches(…) instead.
470 public int shouldMatchByLine(String from, String to, String
pattern) {
This method still reads the lines into an ArrayList (at most) three
times. It calls asLines() once and indexOf() twice. There could be a
version of indexOf() that takes a ListString. Actually indexOf
could be private and always take a ListString.
Otherwise ok!
/Staffan
On 15 jan 2014, at 10:13, Yekaterina Kantserova
yekaterina.kantser...@oracle.com
mailto:yekaterina.kantser...@oracle.com wrote:
Staffan, thank you for pointing out performance and test.src issues!
The webrev with fixes can be found here:
http://cr.openjdk.java.net/~ykantser/7185591/webrev.01/
http://cr.openjdk.java.net/%7Eykantser/7185591/webrev.01/
Please, see my comments in-lined.
Thanks,
Katja
On 01/13/2014 01:19 PM, Staffan Larsen wrote:
Katja,
test/lib/testlibrary/jdk/testlibrary/JcmdBase.java
68 * Run jcmd standalone
I think you should expand a bit on what “standalone” means here. It
took me a while to understand the difference.
Fixed.
test/lib/testlibrary/jdk/testlibrary/OutputAnalyzer.java
423 public int indexOf(String pattern) {
This seems very inefficient. Add all lines to an ArrayList and then
walk through them one at a time to if it matches and then walk
through them once again to find the index of that line.
469 public int shouldMatchByLine(String from, String to, String
pattern) {
Same inefficiency here, but worse because both asLines() and
indexOf() does the same work.
test/lib/testlibrary/jdk/testlibrary/Utils.java
65 public static final String TEST_SRC =
System.getProperty(test.src).trim();
Fixed.
I wonder if this really works. Isn’t “test.src” different for
different tests? A property that jtreg changes
Yes, it is different.
before invoking each test? Or does this work because each test is
run in a different class loader and Utils.java will exist once in
each class loader?
I've learned now it works because jtreg compiles all classes a test
needs to a separate location. For example when I run
TestJcmdDefaults there will be both TestJcmdDefaults.class and
jdk/testlibrary/Utils.class under
/tmp/jtreg/jtreg-workdir/classes/sun/tools/jcmd/. The class loader
will load Utils for TestJcmdDefaults from
/tmp/jtreg/jtreg-workdir/classes/sun/tools/jcmd/.
...
[Loaded jdk.testlibrary.Utils from
file:/tmp/jtreg/jtreg-workdir/classes/sun/tools/jcmd/]
...
But I think it's better to declare TEST_SRC per test precisely
because it's different for different tests.
/Staffan
On 10 jan 2014, at 13:50, Yekaterina Kantserova
yekaterina.kantser...@oracle.com
mailto:yekaterina.kantser...@oracle.com wrote:
Hi,
Could I please have a review of this fix.
In this fix I've rewritten sun/tools/jcmd/* tests in pure java to
get rid of all intermittent failures depending on for example MKS
or race conditions (test application has not yet started when the
test start to run).
Webrev:
http://cr.openjdk.java.net/~ykantser/7185591/webrev.00/
http://cr.openjdk.java.net/%7Eykantser/7185591/webrev.00/
Primal bug:
https://bugs.openjdk.java.net/browse/JDK-7185591
Similar bugs:
https://bugs.openjdk.java.net/browse/JDK-6977426
https://bugs.openjdk.java.net/browse/JDK-8020798
https://bugs.openjdk.java.net/browse/JDK-7130656 (this one is
blocked by https://bugs.openjdk.java.net/browse/JDK-8031482 so far)
Thanks,
Katja
# HG changeset patch
# User ykantser
# Date 1389868669 -3600
# Node ID 1633359cf5e40f1bd4b60d55d18bc4ad9f8bf7e2
# Parent 335dc9c16a363ac2ae1f5b47dde9d7f3d6ca3752
7185591: jcmd-big-script.sh ERROR: could not find app's Java pid.
Reviewed-by: egahlin, sla, jbachorik
diff --git a/test/ProblemList.txt b/test/ProblemList.txt
--- a/test/ProblemList.txt
+++ b/test/ProblemList.txt
@@ -277,3 +277,10 @@
# jdk_util
+
+# svc_tools
+
+# 8031482
+sun/tools/jcmd/TestJcmdSanity.javawindows-all
+
Re: Review request for 8029378: com/sun/jdi/BadHandshakeTest.java failed with java.util.concurrent.TimeoutException
On 16.1.2014 11:48, shanliang wrote: David Holmes wrote: On 16/01/2014 5:37 PM, shanliang wrote: Hi, Please review this simple fix, the test needs more time to wait Phaser.awaitAdvanceInterruptibly(...). Integer.MAX_VALUE? There's no point using a timed form at all. David Yes Phaser.awaitAdvanceInterruptibly(int phase) could be used here, but the call of this method is wrapped in: jdk.testlibrary.ProcessTools.startProcess(...) So we have to add a new method ProcessTools.startProcess(...) which has no timeout parameter. I did not do this because I thought to have a simple fix only within the test. This timeoud seems to be caused by using the fastdebug build. You could use Utils.TIMEOUT_FACTOR to scale the original timeout (1500) according to the parameters specified for tests running fastdebug builds. -JB- If this is useful, here is the new web: http://cr.openjdk.java.net/~sjiang/JDK-8029378/01/ Thanks, Shanliang webrev: http://cr.openjdk.java.net/~sjiang/JDK-8029378/00/ bug: https://bugs.openjdk.java.net/browse/JDK-8029378 Thanks, Shanliang
Re: Review request for 8029378: com/sun/jdi/BadHandshakeTest.java failed with java.util.concurrent.TimeoutException
Jaroslav Bachorik wrote: On 16.1.2014 11:48, shanliang wrote: David Holmes wrote: On 16/01/2014 5:37 PM, shanliang wrote: Hi, Please review this simple fix, the test needs more time to wait Phaser.awaitAdvanceInterruptibly(...). Integer.MAX_VALUE? There's no point using a timed form at all. David Yes Phaser.awaitAdvanceInterruptibly(int phase) could be used here, but the call of this method is wrapped in: jdk.testlibrary.ProcessTools.startProcess(...) So we have to add a new method ProcessTools.startProcess(...) which has no timeout parameter. I did not do this because I thought to have a simple fix only within the test. This timeoud seems to be caused by using the fastdebug build. You could use Utils.TIMEOUT_FACTOR to scale the original timeout (1500) according to the parameters specified for tests running fastdebug builds. Yes it could be a solution, but to wait the jtreg timeout seems better, we do not need to take care of timeout. Thanks, Shanliang -JB- If this is useful, here is the new web: http://cr.openjdk.java.net/~sjiang/JDK-8029378/01/ Thanks, Shanliang webrev: http://cr.openjdk.java.net/~sjiang/JDK-8029378/00/ bug: https://bugs.openjdk.java.net/browse/JDK-8029378 Thanks, Shanliang
Re: RFR 8028623: SA: hash codes in SymbolTable mismatching java_lang_String::hash_code for extended characters.
Thanks - just need that second reviewer then... On 13/01/14 12:38, Staffan Larsen wrote: Looks good! Thanks for taking the time to re-write the test in Java. Thanks, /Staffan On 8 jan 2014, at 15:59, Kevin Walls kevin.wa...@oracle.com wrote: Hi Staffan - http://cr.openjdk.java.net/~kevinw/8028623/webrev.01/ Yes it's better now, getting pid and launching a tool etc have been previous reasons to use a script, but with this new help it's not too bad!... Thanks, Kevin On 07/01/14 09:43, Staffan Larsen wrote: Kevin, The fix looks good. For tests, we are trying to avoid adding new shell-script based tests since they too often cause problems. Would it be possible to rewrite the test in pure Java code? There are some helper routines in test/testlibrary/com/oracle/java/testlibrary/ that could be useful. /Staffan On 3 jan 2014, at 18:42, Kevin Walls kevin.wa...@oracle.com wrote: Hi, This problem means you can't use the SA if the target app contains a symbol which uses a non-ascii character. The SA tool will fail with an error, the JVM itself and the SA having calculated different hashes for such Strings. bug: https://bugs.openjdk.java.net/browse/JDK-8028623 webrev: http://cr.openjdk.java.net/~kevinw/8028623/webrev.00/ Thanks Kevin
Re: Review request for 8029378: com/sun/jdi/BadHandshakeTest.java failed with java.util.concurrent.TimeoutException
On 16/01/2014 9:01 PM, shanliang wrote: Jaroslav Bachorik wrote: On 16.1.2014 11:48, shanliang wrote: David Holmes wrote: On 16/01/2014 5:37 PM, shanliang wrote: Hi, Please review this simple fix, the test needs more time to wait Phaser.awaitAdvanceInterruptibly(...). Integer.MAX_VALUE? There's no point using a timed form at all. David Yes Phaser.awaitAdvanceInterruptibly(int phase) could be used here, but the call of this method is wrapped in: jdk.testlibrary.ProcessTools.startProcess(...) So we have to add a new method ProcessTools.startProcess(...) which has no timeout parameter. I did not do this because I thought to have a simple fix only within the test. This timeoud seems to be caused by using the fastdebug build. You could use Utils.TIMEOUT_FACTOR to scale the original timeout (1500) according to the parameters specified for tests running fastdebug builds. Yes it could be a solution, but to wait the jtreg timeout seems better, we do not need to take care of timeout. Okay. It would have been clearer if you had stated that you were removing the timeout in the test and relying on jtreg timing out the test instead. :) Thanks, David Thanks, Shanliang -JB- If this is useful, here is the new web: http://cr.openjdk.java.net/~sjiang/JDK-8029378/01/ Thanks, Shanliang webrev: http://cr.openjdk.java.net/~sjiang/JDK-8029378/00/ bug: https://bugs.openjdk.java.net/browse/JDK-8029378 Thanks, Shanliang
Re: Review request for 8029378: com/sun/jdi/BadHandshakeTest.java failed with java.util.concurrent.TimeoutException
David Holmes wrote: On 16/01/2014 9:01 PM, shanliang wrote: Jaroslav Bachorik wrote: On 16.1.2014 11:48, shanliang wrote: David Holmes wrote: On 16/01/2014 5:37 PM, shanliang wrote: Hi, Please review this simple fix, the test needs more time to wait Phaser.awaitAdvanceInterruptibly(...). Integer.MAX_VALUE? There's no point using a timed form at all. David Yes Phaser.awaitAdvanceInterruptibly(int phase) could be used here, but the call of this method is wrapped in: jdk.testlibrary.ProcessTools.startProcess(...) So we have to add a new method ProcessTools.startProcess(...) which has no timeout parameter. I did not do this because I thought to have a simple fix only within the test. This timeoud seems to be caused by using the fastdebug build. You could use Utils.TIMEOUT_FACTOR to scale the original timeout (1500) according to the parameters specified for tests running fastdebug builds. Yes it could be a solution, but to wait the jtreg timeout seems better, we do not need to take care of timeout. Okay. It would have been clearer if you had stated that you were removing the timeout in the test and relying on jtreg timing out the test instead. :) Thanks David and Jaroslav for reviewing, I will push the first version. Shanliang Thanks, David Thanks, Shanliang -JB- If this is useful, here is the new web: http://cr.openjdk.java.net/~sjiang/JDK-8029378/01/ Thanks, Shanliang webrev: http://cr.openjdk.java.net/~sjiang/JDK-8029378/00/ bug: https://bugs.openjdk.java.net/browse/JDK-8029378 Thanks, Shanliang
Re: RFR(L): 8031581: PPC64: Addons and fixes for AIX to pass the jdk regression tests
On 16/01/2014 10:34, Volker Simonis wrote: : I just thought because poll is more file-descriptor oriented and not network specific. And the constants are also used for example in: src/macosx/classes/sun/nio/ch/KQueueArrayWrapper.java: src/solaris/classes/sun/nio/ch/sctp/Sctp* src/solaris/classes/sun/nio/ch/Port.java But actually I have no prefernece here so I can put them just as well to sun.nio.ch.Net It's not used for anything except sockets here (there aren't any selectable channels that aren't also network channels). So I think sun.nio.ch.Net is marginly cleaner here. : Sure, no problem. Although I still would like to push this to ppc-aix-port/stage-9 and ppc-aix-port/stage first because that's where we really need it. Anyway, the current plan is to merge ppc-aix-port/stage-9 into jdk9 mainline by the end of January and ppc-aix-port/stage into 8u-dev by the end of March (for 8u20). Would that be ok? I see you've created a bug for this. I guess it's okay if comes via the ppc port although would still be good to get it into jdk9/dev early as it impacts all platforms. -Alan.
