Re: review request for 7021209, use try-with-resources in lang, math, util
Stuart Marks wrote: Hi all, The following webrev changes a variety of files in java.lang, math, and util, and their corresponding tests, to use the new Java 7 try-with-resources construct. I think most of the changes should be straightforward. In a few cases (e.g., Currency.java and LocalGregorianCalendar.java) there was no close() call in the original code so the open file was always leaked. Naoto, several of the util changes look like they're in the internationalization area. I don't know if you're the right person to review them; if not, please forward or cc this to the right person. Or, should I forward this to something like i18n-dev? Webrev is here: http://cr.openjdk.java.net/~smarks/reviews/7021209/webrev.0/ Thanks! s'marks Looks good to me and great to it fixing the bugs in Currency and LocalGregorianCalendar. Minor comment on test/java/util/Formatter/FailingConstructors.java is that an alternative to using try-with-resources is to just replace it with Files.write(file.toPath(), FILE_CONTENTS.getBytes()). Same thing in test/java/util/Scanner/FailingConstructors.java. -Alan.
hg: jdk7/tl/jdk: 3 new changesets
Changeset: dbdafe65af60 Author:alanb Date: 2011-02-21 13:54 + URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/dbdafe65af60 7020517: (fs) FileStore.equals returns true if both volumes have the same serial number Reviewed-by: chegar ! src/windows/classes/sun/nio/fs/WindowsFileStore.java ! test/java/nio/file/FileStore/Basic.java Changeset: d7cb44a4d08a Author:alanb Date: 2011-02-22 10:19 + URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/d7cb44a4d08a Merge Changeset: 9d8a0369b906 Author:alanb Date: 2011-02-22 12:04 + URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/9d8a0369b906 7020888: (file) Miscellaneous and trivial clean-ups (typos and opportunities to use suppressed exceptions) Reviewed-by: mduigou, chegar ! src/share/classes/java/io/BufferedReader.java ! src/share/classes/java/io/BufferedWriter.java ! src/share/classes/java/io/File.java ! src/share/classes/java/io/FilterOutputStream.java ! src/share/classes/java/io/PushbackInputStream.java ! src/share/classes/java/io/PushbackReader.java ! src/share/classes/java/nio/channels/AsynchronousFileChannel.java ! src/share/classes/java/nio/channels/SocketChannel.java ! src/share/classes/java/nio/file/CopyMoveHelper.java ! src/share/classes/java/nio/file/Files.java ! src/share/classes/sun/nio/ch/FileChannelImpl.java ! src/solaris/classes/sun/nio/ch/UnixAsynchronousServerSocketChannelImpl.java ! src/solaris/classes/sun/nio/ch/UnixAsynchronousSocketChannelImpl.java ! test/java/lang/ProcessBuilder/Basic.java
Re: hg: jdk7/tl/jdk: 3 new changesets
Too many balls in the air, looks like I created the change-set for
7020888 while have another patch in my working copy. This means the
change included changes to several java.io classes that should have been
there. Nothing wrong with the change, they fix long standing issues that
were touched on here recently, but they involve behavior changes and are
not reviewed. Chris - I need anti-delta 5 files, can you review?
diff --git a/src/share/classes/java/io/BufferedReader.java
b/src/share/classes/java/io/BufferedReader.java
--- a/src/share/classes/java/io/BufferedReader.java
+++ b/src/share/classes/java/io/BufferedReader.java
@@ -512,14 +512,11 @@ public class BufferedReader extends Read
public void close() throws IOException {
synchronized (lock) {
-if (in != null) {
-try {
-in.close();
-} finally {
-in = null;
-cb = null;
-}
-}
+if (in == null)
+return;
+in.close();
+in = null;
+cb = null;
}
}
}
diff --git a/src/share/classes/java/io/BufferedWriter.java
b/src/share/classes/java/io/BufferedWriter.java
--- a/src/share/classes/java/io/BufferedWriter.java
+++ b/src/share/classes/java/io/BufferedWriter.java
@@ -255,16 +255,17 @@ public class BufferedWriter extends Writ
}
}
-@SuppressWarnings(try)
public void close() throws IOException {
synchronized (lock) {
-if (out != null) {
-try (Writer w = out) {
-flushBuffer();
-} finally {
-out = null;
-cb = null;
-}
+if (out == null) {
+return;
+}
+try {
+flushBuffer();
+} finally {
+out.close();
+out = null;
+cb = null;
}
}
}
diff --git a/src/share/classes/java/io/FilterOutputStream.java
b/src/share/classes/java/io/FilterOutputStream.java
--- a/src/share/classes/java/io/FilterOutputStream.java
+++ b/src/share/classes/java/io/FilterOutputStream.java
@@ -152,10 +152,11 @@ class FilterOutputStream extends OutputS
* @seejava.io.FilterOutputStream#flush()
* @seejava.io.FilterOutputStream#out
*/
-@SuppressWarnings(try)
public void close() throws IOException {
-try (OutputStream ostream = out) {
-flush();
+try {
+ flush();
+} catch (IOException ignored) {
}
+out.close();
}
}
diff --git a/src/share/classes/java/io/PushbackInputStream.java
b/src/share/classes/java/io/PushbackInputStream.java
--- a/src/share/classes/java/io/PushbackInputStream.java
+++ b/src/share/classes/java/io/PushbackInputStream.java
@@ -374,13 +374,10 @@ class PushbackInputStream extends Filter
* @exception IOException if an I/O error occurs.
*/
public synchronized void close() throws IOException {
-if (in != null) {
-try {
-in.close();
-} finally {
-in = null;
-buf = null;
-}
-}
+if (in == null)
+return;
+in.close();
+in = null;
+buf = null;
}
}
diff --git a/src/share/classes/java/io/PushbackReader.java
b/src/share/classes/java/io/PushbackReader.java
--- a/src/share/classes/java/io/PushbackReader.java
+++ b/src/share/classes/java/io/PushbackReader.java
@@ -245,11 +245,8 @@ public class PushbackReader extends Filt
* @exception IOException If an I/O error occurs
*/
public void close() throws IOException {
-try {
-super.close();
-} finally {
-buf = null;
-}
+super.close();
+buf = null;
}
/**
diff --git a/test/java/lang/ProcessBuilder/Basic.java
b/test/java/lang/ProcessBuilder/Basic.java
--- a/test/java/lang/ProcessBuilder/Basic.java
+++ b/test/java/lang/ProcessBuilder/Basic.java
@@ -1762,9 +1762,9 @@ public class Basic {
equal(p.exitValue(), 5);
-try { p.getInputStream().close(); } catch (IOException
ignore) { }
-try {p.getErrorStream().close(); } catch (IOException
ignore) { }
-try { p.getOutputStream().close(); } catch (IOException
ignore) { }
+p.getInputStream().close();
+p.getErrorStream().close();
+p.getOutputStream().close();
InputStream[] streams = { p.getInputStream(),
p.getErrorStream() };
for (final InputStream in : streams) {
hg: jdk7/tl/jdk: 7021327: Changes for 7020888 included changes to other files in error
Changeset: bac152c6491a Author:alanb Date: 2011-02-22 14:28 + URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/bac152c6491a 7021327: Changes for 7020888 included changes to other files in error Reviewed-by: chegar ! src/share/classes/java/io/BufferedReader.java ! src/share/classes/java/io/BufferedWriter.java ! src/share/classes/java/io/FilterOutputStream.java ! src/share/classes/java/io/PushbackInputStream.java ! src/share/classes/java/io/PushbackReader.java ! test/java/lang/ProcessBuilder/Basic.java
hg: jdk7/tl/jdk: 6702400: ChunkedInputStream expecting -1 from int read, but int-char comparision is wrong
Changeset: b853414b8eef Author:michaelm Date: 2011-02-22 14:44 + URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/b853414b8eef 6702400: ChunkedInputStream expecting -1 from int read, but int-char comparision is wrong Reviewed-by: chegar ! src/share/classes/sun/net/httpserver/ChunkedInputStream.java
Re: Zlib level in JDK7
On 2/20/2011 9:39 AM, Dr Andrew John Hughes wrote: On 15 February 2011 20:23, Phil Racephilip.r...@oracle.com wrote: On 2/15/2011 6:07 AM, Dr Andrew John Hughes wrote: Yes, IcedTea uses system libraries for everything bar LCMS, where local changes in OpenJDK mean we are still forced to use the in-tree version. There hasn't been any success upstreaming these changes, though I haven't looked at LCMS 2.x. LittleCMS 1.x didn't provide the support necessary to pass JCK. So we talked to the LittleCMS maintainer and he added the necessary APIs in 2.0 JDK 7 has had LittleCMS 2.0 for almost 6 months now and that is included without any code modifications, so I think it should now be possible to use a system library, although we didn't do the work to actually enable that, so its built into a JDK library which has the littlecms code and the glue code. We need to provide the ability to separate these. When we pushed LCMS 2.0, I asked for a bug to be filed to remember to do this work but I can't find it in the database. I'll ask for that to be filed if it wasn't already. NB It didn't seem super-urgent since we pulled in LCMS 2.0 relatively soon after its release we thought shipping distros weren't likely to have the library upgrade anyway, but that's probably changing. -phil. Hi Phil, Thanks for making me aware of this. I briefly glanced at some for the release notes for LCMS 2 when it was released, and saw something that may support the missing functionality, but never had chance to look in detail. I've also not had chance to look at OpenJDK 7 recently, so it's great to hear that support has already gone in. Do you have any idea as to whether this would be safe to backport to OpenJDK 6? I presume it doesn't alter any public API. It ought to be OK. If someone else wants to take on the work that is :-) I've not seen any use of OpenJDK 7 by distros as yet. We've managed with the other libraries without in-tree support by using local patching. There's a big libraries patch in IcedTea that would be nice to integrate but it would need considerable work to do optional system linking rather than the current brute force of deleting the in-tree version and always linking. There's also no TCK for 7 yet, which is I believe what caught many of the issues with missing LCMS support before. I don't know how distros would want to present/package the 7 EA builds so I'm not too surprised they aren't common. We believe LCMS 2.0 should pass JCK, but I don't know if a full JCK run has been performed against a fully open 7 build since it went in. A 6-open backport would find any problems there. I did a quick survey of distro support for LCMS 2. It's in Gentoo (which is what made me aware of it), but Ubuntu, Debian and Fedora all seem to still be on the 1.x series. So it doesn't seem to be changing yet. Maybe OpenJDK could be the kick they need to support it? ;-) yep. -phil.
java.awt.Graphics: not a candidate for AutoCloseable?
Hello, community! Is java.awt.Graphics not a candidate for AutoCloseable? In most cases, a Graphics object has to be dispose()'d at the end. Couldn't it be extended with close() which would be a delegate to dispose()? Best regards, Ivan G Shevchenko
Re: java.awt.Graphics: not a candidate for AutoCloseable?
Aside from the fact that its weird, if not downright wrong to think of a Graphics instance as closeable, Graphics instances are not used in the same way as all the stream type resources for which AutoCloseable is intended. In most cases they are provided to you in a call to paint() Releasing a Graphics via explicit call to dispose() only should be used in the relatively rare use case where you obtained it yourself to draw on an image or some other surface. And even then dispose() is not a requirement since the system does dispose of the graphics for you in a prompt way that does not require finalisation. The docs on java.awt.Graphics are extremely dated in that respect. -Phil-of-the-graphics-team. On 2/22/2011 12:00 PM, assembling signals wrote: Hello, community! Is java.awt.Graphics not a candidate for AutoCloseable? In most cases, a Graphics object has to be dispose()'d at the end. Couldn't it be extended with close() which would be a delegate to dispose()? Best regards, Ivan G Shevchenko
Re: java.awt.Graphics: not a candidate for AutoCloseable?
Right, I was dealing with sources, where accidentally all Graphics objects were manually created from BufferedImage objects ;) So I wasn't thinking about the more common case of paint(). 22.02.2011, 21:19, Phil Race philip.r...@oracle.com: Aside from the fact that its weird, if not downright wrong to think of a Graphics instance as closeable, Graphics instances are not used in the same way as all the stream type resources for which AutoCloseable is intended. In most cases they are provided to you in a call to paint() Releasing a Graphics via explicit call to dispose() only should be used in the relatively rare use case where you obtained it yourself to draw on an image or some other surface. And even then dispose() is not a requirement since the system does dispose of the graphics for you in a prompt way that does not require finalisation. The docs on java.awt.Graphics are extremely dated in that respect. -Phil-of-the-graphics-team. On 2/22/2011 12:00 PM, assembling signals wrote: Hello, community! Is java.awt.Graphics not a candidate for AutoCloseable? In most cases, a Graphics object has to be dispose()'d at the end. Couldn't it be extended with close() which would be a delegate to dispose()? Best regards, Ivan G Shevchenko
Re: review request for 7021209, use try-with-resources in lang, math, util
Looks good to me. Naoto (2/21/11 6:05 PM), Stuart Marks wrote: Hi all, The following webrev changes a variety of files in java.lang, math, and util, and their corresponding tests, to use the new Java 7 try-with-resources construct. I think most of the changes should be straightforward. In a few cases (e.g., Currency.java and LocalGregorianCalendar.java) there was no close() call in the original code so the open file was always leaked. Naoto, several of the util changes look like they're in the internationalization area. I don't know if you're the right person to review them; if not, please forward or cc this to the right person. Or, should I forward this to something like i18n-dev? Webrev is here: http://cr.openjdk.java.net/~smarks/reviews/7021209/webrev.0/ Thanks! s'marks
hg: jdk7/tl/jdk: 6604496: Support for CKM_AES_CTR (counter mode)
Changeset: 75216854fb53 Author:valeriep Date: 2011-02-22 12:01 -0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/75216854fb53 6604496: Support for CKM_AES_CTR (counter mode) Summary: Enhanced SunPKCS11 provider to support AES/CTR/NoPadding transformation. Reviewed-by: vinnie ! src/share/classes/sun/security/pkcs11/P11Cipher.java ! src/share/classes/sun/security/pkcs11/SunPKCS11.java + src/share/classes/sun/security/pkcs11/wrapper/CK_AES_CTR_PARAMS.java ! src/share/classes/sun/security/pkcs11/wrapper/CK_MECHANISM.java ! src/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java ! src/share/native/sun/security/pkcs11/wrapper/p11_convert.c + src/share/native/sun/security/pkcs11/wrapper/pkcs-11v2-20a3.h ! src/share/native/sun/security/pkcs11/wrapper/pkcs11wrapper.h ! test/sun/security/pkcs11/Cipher/TestSymmCiphers.java ! test/sun/security/pkcs11/Cipher/TestSymmCiphersNoPad.java
Re: review request for 7021209, use try-with-resources in lang, math, util
On 2/22/11 12:44 AM, Alan Bateman wrote: Minor comment on test/java/util/Formatter/FailingConstructors.java is that an alternative to using try-with-resources is to just replace it with Files.write(file.toPath(), FILE_CONTENTS.getBytes()). Same thing in test/java/util/Scanner/FailingConstructors.java. Ah, good point. It's not directly related to try-with-resources but it's good cleanup nonetheless. I'll integrate this change. Thanks. s'marks
hg: jdk7/tl/jdk: 7021209: convert lang, math, util to use try-with-resources
Changeset: 84e339f1033b Author:smarks Date: 2011-02-22 15:34 -0800 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/84e339f1033b 7021209: convert lang, math, util to use try-with-resources Reviewed-by: alanb, darcy, naoto ! src/share/classes/java/lang/Package.java ! src/share/classes/java/util/Currency.java ! src/share/classes/sun/util/calendar/LocalGregorianCalendar.java ! src/solaris/classes/java/util/prefs/FileSystemPreferences.java ! test/java/lang/Character/CheckScript.java ! test/java/lang/Runtime/shutdown/ShutdownHooks.java ! test/java/lang/instrument/BootClassPath/Setup.java ! test/java/lang/instrument/ilib/Inject.java ! test/java/math/BigInteger/BigIntegerTest.java ! test/java/util/Currency/ValidateISO4217.java ! test/java/util/Formatter/FailingConstructors.java ! test/java/util/Locale/LocaleEnhanceTest.java ! test/java/util/ResourceBundle/Bug6204853.java ! test/java/util/Scanner/FailingConstructors.java
Re: Zlib level in JDK7
On 22 February 2011 17:26, Phil Race philip.r...@oracle.com wrote: On 2/20/2011 9:39 AM, Dr Andrew John Hughes wrote: On 15 February 2011 20:23, Phil Racephilip.r...@oracle.com wrote: On 2/15/2011 6:07 AM, Dr Andrew John Hughes wrote: Yes, IcedTea uses system libraries for everything bar LCMS, where local changes in OpenJDK mean we are still forced to use the in-tree version. There hasn't been any success upstreaming these changes, though I haven't looked at LCMS 2.x. LittleCMS 1.x didn't provide the support necessary to pass JCK. So we talked to the LittleCMS maintainer and he added the necessary APIs in 2.0 JDK 7 has had LittleCMS 2.0 for almost 6 months now and that is included without any code modifications, so I think it should now be possible to use a system library, although we didn't do the work to actually enable that, so its built into a JDK library which has the littlecms code and the glue code. We need to provide the ability to separate these. When we pushed LCMS 2.0, I asked for a bug to be filed to remember to do this work but I can't find it in the database. I'll ask for that to be filed if it wasn't already. NB It didn't seem super-urgent since we pulled in LCMS 2.0 relatively soon after its release we thought shipping distros weren't likely to have the library upgrade anyway, but that's probably changing. -phil. Hi Phil, Thanks for making me aware of this. I briefly glanced at some for the release notes for LCMS 2 when it was released, and saw something that may support the missing functionality, but never had chance to look in detail. I've also not had chance to look at OpenJDK 7 recently, so it's great to hear that support has already gone in. Do you have any idea as to whether this would be safe to backport to OpenJDK 6? I presume it doesn't alter any public API. It ought to be OK. If someone else wants to take on the work that is :-) Consider it on my TODO list ;-) I've not seen any use of OpenJDK 7 by distros as yet. We've managed with the other libraries without in-tree support by using local patching. There's a big libraries patch in IcedTea that would be nice to integrate but it would need considerable work to do optional system linking rather than the current brute force of deleting the in-tree version and always linking. There's also no TCK for 7 yet, which is I believe what caught many of the issues with missing LCMS support before. I don't know how distros would want to present/package the 7 EA builds so I'm not too surprised they aren't common. We believe LCMS 2.0 should pass JCK, but I don't know if a full JCK run has been performed against a fully open 7 build since it went in. A 6-open backport would find any problems there. I wasn't aware there was a JCK for 7 yet. At least, not one under the same terms as the one used for OpenJDK6. I did a quick survey of distro support for LCMS 2. It's in Gentoo (which is what made me aware of it), but Ubuntu, Debian and Fedora all seem to still be on the 1.x series. So it doesn't seem to be changing yet. Maybe OpenJDK could be the kick they need to support it? ;-) yep. -phil. -- Andrew :-) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Support Free Java! Contribute to GNU Classpath and the OpenJDK http://www.gnu.org/software/classpath http://openjdk.java.net PGP Key: F5862A37 (https://keys.indymedia.org/) Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
Re: Zlib level in JDK7
On 2/22/2011 3:51 PM, Dr Andrew John Hughes wrote: On 22 February 2011 17:26, Phil Racephilip.r...@oracle.com wrote: We believe LCMS 2.0 should pass JCK, but I don't know if a full JCK run has been performed against a fully open 7 build since it went in. A 6-open backport would find any problems there. I wasn't aware there was a JCK for 7 yet. At least, not one under the same terms as the one used for OpenJDK6. Strictly there cannot be one until the java se 7 spec is final because otherwise it doesn't know what to test, but it has to be worked on concurrently else you'd then have to wait a long time after the JDK work was done .. so that ongoing work is what I refer to. I don't think they run JDK 6 against it because it would fail straight off the bat on all the new APIs. However in this stable area I rather doubt its anything other than the existing JCK6 tests. -phil.
Review CR #6611830: UUID thread-safety and performance improvements
Daniel Aioanei reported via Josh Bloch a data race issue with the UUID accessor and hashCode() methods. I've prepared a webrev with the suggested changes: http://cr.openjdk.java.net/~mduigou/6611830/webrev.0/webrev/ I've tested the change against the standard UUID tests and did a microbenchmark test of one method, variant(), to see what impact not caching had on performance. Since there was only negligible change in performance vs. the existing UUID implementation I am comfortable with eliminating the cache values. It would appear that a field access plus a shift is not a significant cost over a field access. Mike
Re: Review CR #6611830: UUID thread-safety and performance improvements
Hi Vitaly, Vitaly Davidovich said the following on 02/23/11 11:46: Unless i missed something, seems like the race can be fixed via same mechanics as String.hashcode; ie since all of the cached values are based on least and most significant bits which are final and the long members are volatile, using lazy evaluation with use of temps should yield the same results even to racing threads, making the race benign. Are you able to see the actual bug report for this? The key point is that the -1 uninitialized values of the fields are not guaranteed to be seen, so if zero is seen instead the wrong calculation will be done for the variant and hashcode. The conclusion is that the simplest fix is to not bother with lazy evaluation and caching for any of these fields. David Holmes Vitaly On Feb 22, 2011 7:30 PM, Mike Duigou mike.dui...@oracle.com wrote: Daniel Aioanei reported via Josh Bloch a data race issue with the UUID accessor and hashCode() methods. I've prepared a webrev with the suggested changes: http://cr.openjdk.java.net/~mduigou/6611830/webrev.0/webrev/ I've tested the change against the standard UUID tests and did a microbenchmark test of one method, variant(), to see what impact not caching had on performance. Since there was only negligible change in performance vs. the existing UUID implementation I am comfortable with eliminating the cache values. It would appear that a field access plus a shift is not a significant cost over a field access. Mike
Re: Review CR #6611830: UUID thread-safety and performance improvements
I think you have a potential visibility problem here. You use -1 as the initial value, but observing threads might see instead the default value if the initializing write is not visible, and mistakenly think that the zero default value represents a computed value. (This is different from the trick employed by String.hashCode(), which does not use an initializer. Can you get the same result using zero instead?) The key to making the String.hashCode() trick work is that, while there is definitely a data race, it is benign because the field can only ever take on one non-default value, and that value is computed deterministically from immutable state. On 2/22/2011 7:29 PM, Mike Duigou wrote: Daniel Aioanei reported via Josh Bloch a data race issue with the UUID accessor and hashCode() methods. I've prepared a webrev with the suggested changes: http://cr.openjdk.java.net/~mduigou/6611830/webrev.0/webrev/ I've tested the change against the standard UUID tests and did a microbenchmark test of one method, variant(), to see what impact not caching had on performance. Since there was only negligible change in performance vs. the existing UUID implementation I am comfortable with eliminating the cache values. It would appear that a field access plus a shift is not a significant cost over a field access. Mike
