Re: [cp-patches] RFC: use helper method to clone char array in java.lang.String
Robert Lougher wrote: Hi, On 2/4/08, Ian Rogers [EMAIL PROTECTED] wrote: Hi, xalan performs 1.4 million char array clones per iteration of the normal size DaCapo benchmark. All of the character array clones are coming from java.lang.String. The attached patch changes the use of char[].clone (which maps to java.lang.Object.clone) to a helper method that allocates the character array and then array copies from the source to the new array. On the Jikes RVM I get the following performance results from 10 iterations of DaCapo using the large data set size: current java.lang.String using char[].clone: run 1: 99157ms run 2: 98700ms run 3: 97927ms patched java.lang.String using the helper routine: run 1: 97710ms run 2: 97406ms run 3: 96762ms The speed up is between 0.22% and 1.2%. Do people think using the helper is a sensible change? I would like to see evidence that this is a win, or at least has no slowdown on other VMs (i.e. it is VM independent). I think it would be inappropriate if it was only to address implementation issues in JikesRVM. For example, why is the helper faster than clone? Surely all clone() should be doing is an alloc and then an arraycopy? Rob. Hi Rob, Twisti, Tromey, so what happens in the case of the clone is: 1) call into clone 2) determine that this is a clone of an array 3) create array of same length as that which we're cloning (we inline as far as here in the case of Jikes RVM) 4) call into array copy 5) determine type of array we're copying 6) check for overlaps 7) copy arrays 8) leave array copy and clone 9) check that the resulting array casts back to a char[] in the case of the helper method: 1) call into helper method 2) create array of same length as that which we're cloning 3) call into array copy 4) determine type of array we're copying 5) check for overlaps 6) copy arrays (we inline as far as here in the case of Jikes RVM) 7) leave array copy and helper method The Jikes RVM is performing a lot of partial evaluation to determine that a lot of bounds checks, casts, instanceof tests are unnecessary and the result is code that just allocates the array and performs a copy without checks. In the case of clone our partial evaluation breaks down because we need the results of runtime reflection calls or to know that these calls are analogous to bytecodes when the arguments are constant. I plan to do optimizations in this direction, but I don't want to flatter the optimizations when they probably only effect a small number of situations and alternate view is that code may have been written in a slightly esoteric manner. I think the Jikes RVM is performing more optimizations than other Classpath VMs, so its likely the performance win will be less marked for those VMs (if there's any win at all). I think Tromey's point is valid and I'll try to write a better patch to address it. Once I have posted the new patch maybe we can return to the question as to whether to make the change. Thanks for all the feedback! Ian
[cp-patches] RFC: tweaks to java.util.zip.ZipEntry
Hi,
the attached patch stops the lazy initialization of a Calendar object in
ZipEntry and instead uses a static final one. It also modifies the clone
method to instead of using Object.clone to use the ZipEntry's own copy
constructor. This patch has implications to bootstrapping and subtly
changes ZipEntry's behavior. I welcome feedback prior to committing.
Thanks,
Ian
--- java/util/zip/ZipEntry.java 2007-10-07 19:48:46.0 +0100
+++ java/util/zip/ZipEntry.java 2008-02-05 12:32:22.0 +
@@ -56,9 +56,10 @@
private static final int KNOWN_TIME = 8;
private static final int KNOWN_EXTRA = 16;
- private static Calendar cal;
+ /** Calendar used for time computations */
+ private static final Calendar cal = Calendar.getInstance();
- private String name;
+ private final String name;
private int size;
private long compressedSize = -1;
private int crc;
@@ -140,18 +141,13 @@
*/
public Object clone()
{
-try
- {
- // The JCL says that the `extra' field is also copied.
- ZipEntry clone = (ZipEntry) super.clone();
- if (extra != null)
- clone.extra = (byte[]) extra.clone();
- return clone;
- }
-catch (CloneNotSupportedException ex)
+ZipEntry clone = new ZipEntry(this);
+if (extra != null)
{
- throw new InternalError();
+ clone.extra = new byte[extra.length];
+ System.arraycopy(extra, 0, clone.extra, 0, extra.length);
}
+return clone;
}
/**
@@ -169,7 +165,6 @@
*/
public void setTime(long time)
{
-Calendar cal = getCalendar();
synchronized (cal)
{
cal.setTimeInMillis(time);
@@ -204,7 +199,6 @@
try
{
- cal = getCalendar();
synchronized (cal)
{
cal.set(year, mon, day, hrs, min, sec);
@@ -219,14 +213,6 @@
}
}
- private static synchronized Calendar getCalendar()
- {
-if (cal == null)
- cal = Calendar.getInstance();
-
-return cal;
- }
-
/**
* Sets the size of the uncompressed data.
* @exception IllegalArgumentException if size is not in 0..0xL
Re: [cp-patches] RFC: use helper method to clone char array in java.lang.String
Hello all,
here's a revised patch that removes the use of clone in preference to
just array copying the live portion of the String. Here are the DaCapo
xalan figures:
run 1: 97972ms
run 2: 97837ms
run 3: 95290ms
which represents anything from a 0.04% slow down to a 2.8% speed up.
There will also be a space saving as we're not cloning whole arrays any
more. Please let me know if you think this patch is suitable for inclusion.
Thanks,
Ian
--- java/lang/String.java 2008-01-23 09:01:02.0 +
+++ java/lang/String.java 2008-02-05 10:07:56.0 +
@@ -1303,13 +1303,13 @@
break;
if (i 0)
return this;
-char[] newStr = (char[]) value.clone();
-newStr[x] = newChar;
+char[] newStr = toCharArray();
+newStr[x-offset] = newChar;
while (--i = 0)
if (value[++x] == oldChar)
-newStr[x] = newChar;
+newStr[x-offset] = newChar;
// Package constructor avoids an array copy.
-return new String(newStr, offset, count, true);
+return new String(newStr, 0, count, true);
}
/**
@@ -1450,23 +1450,25 @@
// Now we perform the conversion. Fortunately, there are no multi-character
// lowercase expansions in Unicode 3.0.0.
-char[] newStr = (char[]) value.clone();
+char[] newStr = new char[count];
+VMSystem.arraycopy(value, offset, newStr, 0, count-(x-offset));
do
{
char ch = value[x];
// Hardcoded special case.
if (ch != '\u0049')
{
-newStr[x++] = Character.toLowerCase(ch);
+newStr[x-offset] = Character.toLowerCase(ch);
}
else
{
-newStr[x++] = '\u0131';
+newStr[x-offset] = '\u0131';
}
+x++;
}
while (--i = 0);
// Package constructor avoids an array copy.
-return new String(newStr, offset, count, true);
+return new String(newStr, 0, count, true);
}
/**
@@ -1504,16 +1506,18 @@
// Now we perform the conversion. Fortunately, there are no
// multi-character lowercase expansions in Unicode 3.0.0.
-char[] newStr = (char[]) value.clone();
+char[] newStr = new char[count];
+VMSystem.arraycopy(value, offset, newStr, 0, count-(x-offset));
do
{
char ch = value[x];
// Hardcoded special case.
-newStr[x++] = Character.toLowerCase(ch);
+newStr[x-offset] = Character.toLowerCase(ch);
+x++;
}
while (--i = 0);
// Package constructor avoids an array copy.
-return new String(newStr, offset, count, true);
+return new String(newStr, 0, count, true);
}
}
@@ -1557,22 +1561,24 @@
i = count;
if (expand == 0)
{
-char[] newStr = (char[]) value.clone();
+char[] newStr = new char[count];
+VMSystem.arraycopy(value, offset, newStr, 0, count-(x-offset));
while (--i = 0)
{
char ch = value[x];
// Hardcoded special case.
if (ch != '\u0069')
{
-newStr[x++] = Character.toUpperCase(ch);
+newStr[x-offset] = Character.toUpperCase(ch);
}
else
{
-newStr[x++] = '\u0130';
+newStr[x-offset] = '\u0130';
}
+x++;
}
// Package constructor avoids an array copy.
-return new String(newStr, offset, count, true);
+return new String(newStr, 0, count, true);
}
// Expansion is necessary.
@@ -1642,14 +1648,16 @@
i = count;
if (expand == 0)
{
-char[] newStr = (char[]) value.clone();
+char[] newStr = new char[count];
+VMSystem.arraycopy(value, offset, newStr, 0, count-(x-offset));
while (--i = 0)
{
char ch = value[x];
-newStr[x++] = Character.toUpperCase(ch);
+newStr[x-offset] = Character.toUpperCase(ch);
+x++;
}
// Package constructor avoids an array copy.
-return new String(newStr, offset, count, true);
+return new String(newStr, 0, count, true);
}
// Expansion is necessary.
@@ -1730,9 +1738,6 @@
*/
public char[] toCharArray()
{
-if (count == value.length)
- return (char[]) value.clone();
-
char[] copy = new char[count];
VMSystem.arraycopy(value, offset, copy, 0, count);
return copy;
RE: [cp-patches] RFC: tweaks to java.util.zip.ZipEntry
Ian Rogers wrote: the attached patch stops the lazy initialization of a Calendar object in ZipEntry and instead uses a static final one. It also modifies the clone method to instead of using Object.clone to use the ZipEntry's own copy constructor. ZipEntry isn't final, so you must use Object.clone. Regards, Jeroen
Re: [cp-patches] RFC: tweaks to java.util.zip.ZipEntry
Ian == Ian Rogers [EMAIL PROTECTED] writes: Ian the attached patch stops the lazy initialization of a Calendar object Ian in ZipEntry and instead uses a static final one. I thought this could lead to odd results sometimes, when the calendrical data is in a zip. But it is hard to remember. Maybe a bit of archaeology is in order -- looking through the cvs logs or the list archives to see why the code is the way it is. (That info should be in comments, of course...) Ian It also modifies the clone method to instead of using Ian Object.clone to use the ZipEntry's own copy constructor. This isn't a safe transform if the class is not final. Cloning a subclass will yield the wrong type. Tom
Re: [cp-patches] RFC: tweaks to java.util.zip.ZipEntry
On Tue, 2008-02-05 at 06:07 -0700, Tom Tromey wrote: Ian == Ian Rogers [EMAIL PROTECTED] writes: Ian the attached patch stops the lazy initialization of a Calendar object Ian in ZipEntry and instead uses a static final one. I thought this could lead to odd results sometimes, when the calendrical data is in a zip. But it is hard to remember. Maybe a bit of archaeology is in order -- looking through the cvs logs or the list archives to see why the code is the way it is. (That info should be in comments, of course...) I don't remember all the details, but here are some pointers: http://gcc.gnu.org/ml/java-patches/2002-q4/msg00294.html http://gcc.gnu.org/ml/java/2003-03/msg00253.html http://gcc.gnu.org/ml/java/2003-11/msg00105.html Sorry for being lazy myself... Cheers, Mark
Re: [cp-patches] RFC: use helper method to clone char array in java.lang.String
Ian == Ian Rogers [EMAIL PROTECTED] writes: Ian Please let me know if you think this patch is suitable for Ian inclusion. It looks fine. I do have one nit, which is that we put spaces around operators... this problem is pervasive in the patch, but here's one example: Ian +newStr[x-offset] = newChar; That should read newStr[x - offset] = newChar; Tom
