Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: pass (nike.apache.org: domain of t.p.ellison@gmail.com
 designates 209.85.220.218 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:user-agent:mime-version:to:subject:references
         :in-reply-to:x-enigmail-version:content-type
         :content-transfer-encoding;
        b=ajvw0FlEIzobLu/t2VoDVv8OB1JAPIvWvIbiEJsGdHxq/GtfnZBi+Go0FytyakgjSE
         dTHqJmz7M34cVonFV0CBH/iE2SBbIkUZysYfkoRqbUnXtGeqWXL8vFv4QGOYnT+Z0IcO
         Xx69aufKogkrZ6PnjvIfAn1WhhsrUhmhhaFj4=
Message-ID: <4ACCF982.7070006@gmail.com>
Date: Wed, 07 Oct 2009 21:26:42 +0100
From: Tim Ellison <t.p.ellison@gmail.com>
User-Agent: Thunderbird 2.0.0.23 (Windows/20090812)
MIME-Version: 1.0
To: dev@harmony.apache.org
Subject: [classlib][archive] Several archive bugfixes and optimizations
 (HARMONY-6346)
References: <1460895940.1254529943491.JavaMail.jira@brutus>
In-Reply-To: <1460895940.1254529943491.JavaMail.jira@brutus>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Thanks for the patch Jesse.  A specific comment below.

On 03/Oct/2009 01:32, Jesse Wilson (JIRA) wrote:
> Several archive bugfixes and optimizations 
> ------------------------------------------
> The attached patch includes several miscellaneous bugfixes,
> optimizations and simplifications that we created for Android's copy
> of the archive module.  Here's an overview of what's changed:
> 
> InflaterInputStream Removes a bogus magic number check. Previously I
> submitted a patch to get this check to work; but the whole premise is
> flawed. To demonstrate, attempt to inflate data that was deflated
> using non-default settings.
> 
> InflaterInputStream, ZipInputStream, GZIPInputStream These now make
> sure that available() returns 0 when the end of the stream is
> reached. Previously available() could return 1 even if read() was
> guaranteed to return -1.
> 
> GZIPOutputStream Slight performance fix: cast from long to int only
> once
> 
> JarFile Verifies the entry when the last byte is returned. This is
> similar to the available() problem in ZipInputStream etc. Move
> getMetaEntriesImpl() from native to Java.
> 
> ZipFile Limit the amount of memory used while reading files.
> Previously we would create arbitrarily large buffers. Move several
> methods from native to Java.
> 
> MsgHelp Keep resource bundles in memory with soft references Use the
> system classloader always.
> 
> Tests New tests to demonstrate problems above, recovery on broken
> manifests, verification of empty entries

How about we change this new method in JarFile

private byte[] getAllBytesFromStreamAndClose(InputStream is) throws
IOException {
    ByteArrayOutputStream bs = new ByteArrayOutputStream();
    try {
        byte[] buf = new byte[1024];
        while (is.available() > 0) {
            int iRead = is.read(buf, 0, buf.length);
            if (iRead > 0)
                bs.write(buf, 0, iRead);
        }
    } finally {
        is.close();
    }
    return bs.toByteArray();
}

I know that the InputStream here can only be a RAFStream or
InflaterInputStream, and both of them return available() as 1 so long as
the stream is not at the end, but in general I guess it is better to
read to the -1 eof marker rather than check available().

Then it uses a ByteArrayOutputStream every time, which by default has a
32-byte backing array, that grows aggressively, so putting anything near
1K bytes in will result in a ~2K backing array.

So how about we fast track the case (esp. RAFStream) where we get all
the bytes in one read...?

  // Initial read
  byte[] buffer = new byte[1024];
  int count = is.read(buffer);
  int nextByte = is.read();

  // Did we get it all in one read?
  if (nextByte == -1) {
    byte[] dest = new byte[count];
    System.arraycopy(buffer, 0, dest, 0, count);
    return dest;
  }

  // Requires additional reads
  ByteArrayOutputStream baos = new ByteArrayOutputStream(count * 2);
  baos.write(buffer, 0, count);
  baos.write(nextByte);
  while (true) {
    count = is.read(buffer);
    if (count == -1) {
        return baos.toByteArray();
    }
    baos.write(buffer, 0, count);
  }


The same holds true for Manifest#readFully(InputStream).  Even more so
since if we get an eof we don't have to scan for a line ending.

WDYT?

Regards,
Tim