6633613: (str) StringCoding optimizations to avoid unnecessary array copies with Charset arg
I propose to partially fix 6633613: (str) StringCoding optimizations to avoid unnecessary array copies with Charset arg with the patches below (a more ambitious patch will hopefully follow later): Iris, please review. Martin First, warning suppression: diff --git a/src/share/classes/java/lang/StringCoding.java b/src/share/classes/java/lang/StringCoding.java --- a/src/share/classes/java/lang/StringCoding.java +++ b/src/share/classes/java/lang/StringCoding.java @@ -53,22 +53,23 @@ class StringCoding { private StringCoding() { } - /* The cached coders for each thread - */ - private static ThreadLocal decoder = new ThreadLocal(); - private static ThreadLocal encoder = new ThreadLocal(); + /** The cached coders for each thread */ + private final static ThreadLocal<SoftReference<StringDecoder>> decoder = + new ThreadLocal<SoftReference<StringDecoder>>(); + private final static ThreadLocal<SoftReference<StringEncoder>> encoder = + new ThreadLocal<SoftReference<StringEncoder>>(); private static boolean warnUnsupportedCharset = true; - private static Object deref(ThreadLocal tl) { - SoftReference sr = (SoftReference)tl.get(); + private static <T> T deref(ThreadLocal<SoftReference<T>> tl) { + SoftReference<T> sr = tl.get(); if (sr == null) return null; return sr.get(); } - private static void set(ThreadLocal tl, Object ob) { - tl.set(new SoftReference(ob)); + private static <T> void set(ThreadLocal<SoftReference<T>> tl, T ob) { + tl.set(new SoftReference<T>(ob)); } // Trim the given byte array to the given length @@ -174,7 +175,7 @@ class StringCoding { static char[] decode(String charsetName, byte[] ba, int off, int len) throws UnsupportedEncodingException { - StringDecoder sd = (StringDecoder)deref(decoder); + StringDecoder sd = deref(decoder); String csn = (charsetName == null) ? "ISO-8859-1" : charsetName; if ((sd == null) || !(csn.equals(sd.requestedCharsetName()) || csn.equals(sd.charsetName()))) { @@ -273,7 +274,7 @@ class StringCoding { static byte[] encode(String charsetName, char[] ca, int off, int len) throws UnsupportedEncodingException { - StringEncoder se = (StringEncoder)deref(encoder); + StringEncoder se = deref(encoder); String csn = (charsetName == null) ? "ISO-8859-1" : charsetName; if ((se == null) || !(csn.equals(se.requestedCharsetName()) || csn.equals(se.charsetName()))) { second, actual fix: diff --git a/src/share/classes/java/lang/StringCoding.java b/src/share/classes/java/lang/StringCoding.java --- a/src/share/classes/java/lang/StringCoding.java +++ b/src/share/classes/java/lang/StringCoding.java @@ -194,8 +194,7 @@ class StringCoding { static char[] decode(Charset cs, byte[] ba, int off, int len) { StringDecoder sd = new StringDecoder(cs, cs.name()); - byte[] b = Arrays.copyOf(ba, ba.length); - return sd.decode(b, off, len); + return sd.decode(Arrays.copyOfRange(ba, off, off + len), 0, len); } static char[] decode(byte[] ba, int off, int len) { @@ -293,8 +292,7 @@ class StringCoding { static byte[] encode(Charset cs, char[] ca, int off, int len) { StringEncoder se = new StringEncoder(cs, cs.name()); - char[] c = Arrays.copyOf(ca, ca.length); - return se.encode(c, off, len); + return se.encode(Arrays.copyOfRange(ca, off, off + len), 0, len); } static byte[] encode(char[] ca, int off, int len) {
Hi, Martin.
6633613: (str) StringCoding optimizations to avoid unnecessary array copies with Charset arg
Not necessarily your problem, but it would be nice if your "Subject:" line contained the word "review" or "review request" somewhere in it. It would make this message easier to find. I suppose I should come up with a recommendation, float it about, and get it written up in the appropriate section of the Guide.
with the patches below (a more ambitious patch will hopefully follow later):
Presumably I'll see that review later under one of the charset bugids?
First, warning suppression:
I don't believe that StringCoding.java produces build warnings using the default javac options. Should I assume that you're talking about eliminating all build warnings produced using "-Xlint:all" for this file only (not all of the String* files)?
diff --git a/src/share/classes/java/lang/StringCoding.java b/src/share/classes/java/lang/StringCoding.java
These changes are fine.
second, actual fix:
diff --git a/src/share/classes/java/lang/StringCoding.java b/src/share/classes/java/lang/StringCoding.java
The code you modified was added _extremely_ late during jdk6 development to fix another bug. There's a regression test for that bug. Hopefully you ran it? (Not that I expect it to fail... ) Speaking of regression tests, you didn't include one in these diffs and I don't see an appropriate "noreg-*" keyword in the bug. Please resolve this. See Step 6 ofthe Guide, "Change Planning and Guidelines" [1] for a list of possible keywords. The supplied fix is the minimal amount of change necessary to address this bug (as indicated in your evaluation). Assuming that you still intend to further modify code to improve performance in this area, please remember to add the relevant bugids to the evaluation (or at the very least, references to these bugs in the "See Also" section). Finally, I know that we don't have an externally-visible repository for webrevs yet, but we do need to keep a webrev of these changes as it would aid review should we need to backport this fix. Thanks, iris [1] http://openjdk.java.net/guide/changePlanning.html
iris clark wrote:
Hi, Martin.
6633613: (str) StringCoding optimizations to avoid unnecessary array copies with Charset arg
First, warning suppression:
I don't believe that StringCoding.java produces build warnings using the default javac options. Should I assume that you're talking about eliminating all build warnings produced using "-Xlint:all" for this file only (not all of the String* files)?
My personal development environment includes a private build system that recompiles all modified .java source files with -Xlint:all. I like to remove warnings when working on a source file.
second, actual fix:
diff --git a/src/share/classes/java/lang/StringCoding.java b/src/share/classes/java/lang/StringCoding.java
The code you modified was added _extremely_ late during jdk6 development to fix another bug. There's a regression test for that bug. Hopefully you ran it? (Not that I expect it to fail... )
Yup.
Speaking of regression tests, you didn't include one in these diffs and I don't see an appropriate "noreg-*" keyword in the bug. Please resolve this. See Step 6 ofthe Guide, "Change Planning and Guidelines" [1] for a list of possible keywords.
I've added noreg-perf. Martin
participants (2)
-
iris clark
-
Martin Buchholz