Mark Wielaard wrote:
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
Here's a revision to my original patch that only uses the copy
constructor in the case the class is a regular ZipEntry. As Mark pointed
out, the normal case for a ZipEntry is that we just read it, its
therefore only when writing out we may need to convert between the DOS
and millisecond time representations. Having a global shared Calendar
causes bootstrap issues, so what the patch does is just uses a locally
created Calendar in the case we must convert from DOS to millisecond
time or vice versa. The time value is cached so that we only take the
hit of using Calendar once per conversion, as opposed to frequently in
some cases before. The net result is to push up the size of a ZipEntry,
avoid boot strap issues, possibly speed up some patterns of getting and
setting the entry's time but take a hit in creating a Calendar instance
when a conversion is necessary (ie potentially when writing out zip
entries). I think this is what we probably want, but would welcome feedback.
Thanks,
Ian
Index: java/util/zip/ZipEntry.java
===================================================================
RCS file: /sources/classpath/classpath/java/util/zip/ZipEntry.java,v
retrieving revision 1.20
diff -u -r1.20 ZipEntry.java
--- java/util/zip/ZipEntry.java 7 Oct 2007 18:48:46 -0000 1.20
+++ java/util/zip/ZipEntry.java 7 Feb 2008 10:09:38 -0000
@@ -50,23 +50,39 @@
*/
public class ZipEntry implements ZipConstants, Cloneable
{
- private static final int KNOWN_SIZE = 1;
- private static final int KNOWN_CSIZE = 2;
- private static final int KNOWN_CRC = 4;
- private static final int KNOWN_TIME = 8;
- private static final int KNOWN_EXTRA = 16;
-
- private static Calendar cal;
-
- private String name;
+ private static final byte KNOWN_SIZE = 1;
+ private static final byte KNOWN_CSIZE = 2;
+ private static final byte KNOWN_CRC = 4;
+ private static final byte KNOWN_TIME = 8;
+ private static final byte KNOWN_DOSTIME = 16
+ private static final byte KNOWN_EXTRA = 32;
+
+ /** Immutable name of the entry */
+ private final String name;
+ /** Uncompressed size */
private int size;
+ /** Compressed size */
private long compressedSize = -1;
+ /** CRC of uncompressed data */
private int crc;
+ /** Comment or null if none */
+ private String comment = null;
+ /** The compression method. Either DEFLATED or STORED, by default -1. */
+ private byte method = -1;
+ /** Flags specifying what we know about this entry */
+ private byte known = 0;
+ /**
+ * The 32bit DOS encoded format for the time of this entry. Only valid if
+ * KNOWN_DOSTIME is set in known.
+ */
private int dostime;
- private short known = 0;
- private short method = -1;
+ /**
+ * The 64bit Java encoded millisecond time since the beginning of the epoch.
+ * Only valid if KNOWN_TIME is set in known.
+ */
+ private long time;
+ /** Extra data */
private byte[] extra = null;
- private String comment = null;
int flags; /* used by ZipOutputStream */
int offset; /* used by ZipFile and ZipOutputStream */
@@ -113,6 +129,7 @@
compressedSize = e.compressedSize;
crc = e.crc;
dostime = e.dostime;
+ time = e.time;
method = e.method;
extra = e.extra;
comment = e.comment;
@@ -121,37 +138,60 @@
final void setDOSTime(int dostime)
{
this.dostime = dostime;
- known |= KNOWN_TIME;
+ known |= KNOWN_DOSTIME;
+ known &= ~KNOWN_TIME;
}
final int getDOSTime()
{
- if ((known & KNOWN_TIME) == 0)
- return 0;
- else
+ if ((known & KNOWN_DOSTIME) != 0)
return dostime;
+ else if ((known & KNOWN_TIME) != 0)
+ {
+ Calendar cal = Calendar.getInstance();
+ cal.setTimeInMillis(time);
+ dostime = (cal.get(Calendar.YEAR) - 1980 & 0x7f) << 25
+ | (cal.get(Calendar.MONTH) + 1) << 21
+ | (cal.get(Calendar.DAY_OF_MONTH)) << 16
+ | (cal.get(Calendar.HOUR_OF_DAY)) << 11
+ | (cal.get(Calendar.MINUTE)) << 5
+ | (cal.get(Calendar.SECOND)) >> 1;
+ known |= KNOWN_DOSTIME;
+ return dostime;
+ }
+ else
+ return 0;
}
/**
* Creates a copy of this zip entry.
*/
- /**
- * Clones the entry.
- */
public Object clone()
{
- try
+ // JCL defines this as being the same as the copy constructor above,
+ // except that value of the "extra" field is also copied. Take care
+ // that in the case of a subclass we use clone() rather than the copy
+ // constructor.
+ ZipEntry clone;
+ if (this.getClass() == ZipEntry.class)
+ clone = new ZipEntry(this);
+ else
{
- // 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;
+ try
+ {
+ clone = (ZipEntry) super.clone();
+ }
+ catch (CloneNotSupportedException e)
+ {
+ throw new InternalError();
+ }
}
- catch (CloneNotSupportedException ex)
+ if (extra != null)
{
- throw new InternalError();
+ clone.extra = new byte[extra.length];
+ System.arraycopy(extra, 0, clone.extra, 0, extra.length);
}
+ return clone;
}
/**
@@ -169,18 +209,9 @@
*/
public void setTime(long time)
{
- Calendar cal = getCalendar();
- synchronized (cal)
- {
- cal.setTimeInMillis(time);
- dostime = (cal.get(Calendar.YEAR) - 1980 & 0x7f) << 25
- | (cal.get(Calendar.MONTH) + 1) << 21
- | (cal.get(Calendar.DAY_OF_MONTH)) << 16
- | (cal.get(Calendar.HOUR_OF_DAY)) << 11
- | (cal.get(Calendar.MINUTE)) << 5
- | (cal.get(Calendar.SECOND)) >> 1;
- }
+ this.time = time;
this.known |= KNOWN_TIME;
+ this.known &= ~KNOWN_DOSTIME;
}
/**
@@ -192,39 +223,34 @@
// The extra bytes might contain the time (posix/unix extension)
parseExtra();
- if ((known & KNOWN_TIME) == 0)
- return -1;
-
- int sec = 2 * (dostime & 0x1f);
- int min = (dostime >> 5) & 0x3f;
- int hrs = (dostime >> 11) & 0x1f;
- int day = (dostime >> 16) & 0x1f;
- int mon = ((dostime >> 21) & 0xf) - 1;
- int year = ((dostime >> 25) & 0x7f) + 1980; /* since 1900 */
-
- try
- {
- cal = getCalendar();
- synchronized (cal)
- {
- cal.set(year, mon, day, hrs, min, sec);
- return cal.getTimeInMillis();
- }
- }
- catch (RuntimeException ex)
+ if ((known & KNOWN_TIME) != 0)
+ return time;
+ else if ((known & KNOWN_DOSTIME) != 0)
{
- /* Ignore illegal time stamp */
- known &= ~KNOWN_TIME;
- return -1;
+ int sec = 2 * (dostime & 0x1f);
+ int min = (dostime >> 5) & 0x3f;
+ int hrs = (dostime >> 11) & 0x1f;
+ int day = (dostime >> 16) & 0x1f;
+ int mon = ((dostime >> 21) & 0xf) - 1;
+ int year = ((dostime >> 25) & 0x7f) + 1980; /* since 1900 */
+
+ try
+ {
+ Calendar cal = Calendar.getInstance();
+ cal.set(year, mon, day, hrs, min, sec);
+ time = cal.getTimeInMillis();
+ known |= KNOWN_TIME;
+ return time;
+ }
+ catch (RuntimeException ex)
+ {
+ /* Ignore illegal time stamp */
+ known &= ~KNOWN_TIME;
+ return -1;
+ }
}
- }
-
- private static synchronized Calendar getCalendar()
- {
- if (cal == null)
- cal = Calendar.getInstance();
-
- return cal;
+ else
+ return -1;
}
/**
@@ -298,7 +324,7 @@
if (method != ZipOutputStream.STORED
&& method != ZipOutputStream.DEFLATED)
throw new IllegalArgumentException();
- this.method = (short) method;
+ this.method = (byte) method;
}
/**
