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
