Re: [cp-patches] RFC: tweaks to java.util.zip.ZipEntry

2008-02-07 Thread Ian Rogers

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 -   1.20
+++ java/util/zip/ZipEntry.java 7 Feb 2008 10:09:38 -
@@ -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
+  

[cp-patches] RFC: tweaks to java.util.zip.ZipEntry

2008-02-05 Thread Ian Rogers

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: tweaks to java.util.zip.ZipEntry

2008-02-05 Thread Jeroen Frijters
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

2008-02-05 Thread Tom Tromey
 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

2008-02-05 Thread Mark Wielaard

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