Miscellaneous improvements to "jar".
Xueming Shen
Xueming.Shen at Sun.COM
Fri Jun 26 17:33:10 UTC 2009
The latest version looks good. 2 nit comments
(1) it's reasonable to have createTempFileInSamDirectoryAs separate out,
but I would
keep the directoryOf within it. Yes, it's "clearer":-)
(2)the "updateOK" in dumpIndex really serves nobody but "clearer":-)
sherman
btw, while you are here:-) do you have time to make the update() NOT to
put jarIndex the
first one when there is a manifest present? The JarInputStream assumes
the manifest is always
the first entry (or the second if the manifest dir is the first), which
causes problem...I'm not saying
to mix with this one:-) just in case you have time and interested while
you are here:-)
Martin Buchholz wrote:
> I did some benchmarking,
> and found that my changes "only" make jar 10-20% faster.
> Disappointing - we expect an order of magnitude for every commit!
>
> While benchmarking, I discovered to my horror that the simple
>
> jar cf /tmp/t1 ...
> jar i /tmp/t1
>
> fails, because it tries to create the replacement jar in "." and then
> rename() it into place. Oops... Better refactor out the code that
> puts the replacement temp file in the same directory.
> Better write some tests for that, too.
>
> /**
> * A variant of File.getParentFile that always returns a valid
> * directory (i.e. returns new File(".") where File.getParentFile
> * would return null).
> */
> private static File directoryOf(File file) {
> File dir = file.getParentFile();
> return (dir != null) ? dir : new File(".");
> }
>
> /**
> * Creates a new empty temporary file in the same directory as the
> * specified file. A variant of File.createTempFile.
> */
> private static File createTempFileInSameDirectoryAs(File file)
> throws IOException {
> return File.createTempFile("jartmp", null, directoryOf(file));
> }
>
> ---
>
> While we're here, let's remove the double buffering on file copy
> operations.
> And the repeated allocations of big buffers.
>
> /**
> * A buffer for use only by copy(InputStream, OutputStream).
> * Not as clean as allocating a new buffer as needed by copy,
> * but significantly more efficient.
> */
> private byte[] copyBuf = new byte[8192];
>
> /**
> * Copies all bytes from the input stream to the output stream.
> * Does not close or flush either stream.
> *
> * @param from the input stream to read from
> * @param to the output stream to write to
> * @throws IOException if an I/O error occurs
> */
> private void copy(InputStream from, OutputStream to) throws
> IOException {
> int n;
> while ((n = from.read(copyBuf)) != -1)
> to.write(copyBuf, 0, n);
> }
>
> /**
> * Copies all bytes from the input file to the output stream.
> * Does not close or flush the output stream.
> *
> * @param from the input file to read from
> * @param to the output stream to write to
> * @throws IOException if an I/O error occurs
> */
> private void copy(File from, OutputStream to) throws IOException {
> InputStream in = new FileInputStream(from);
> try {
> copy(in, to);
> } finally {
> in.close();
> }
> }
>
> /**
> * Copies all bytes from the input stream to the output file.
> * Does not close the input stream.
> *
> * @param from the input stream to read from
> * @param to the output file to write to
> * @throws IOException if an I/O error occurs
> */
> private void copy(InputStream from, File to) throws IOException {
> OutputStream out = new FileOutputStream(to);
> try {
> copy(from, out);
> } finally {
> out.close();
> }
> }
>
> ----
>
> On the other hand, allocating a CRC32 is very cheap, so let's make that
> private to CRC32OutputStream.
> + private static class CRC32OutputStream extends java.io.OutputStream {
> + final CRC32 crc = new CRC32();
>
> ----
>
> We should add a try { finally } to delete the tempfile for jar i
> + try {
> boolean updateOk = update(new FileInputStream(jarFile),
> - new FileOutputStream(scratchFile),
>
> + new FileOutputStream(tmpFile),
> null, index);
> + if (updateOk) {
> jarFile.delete();
>
> - if (!scratchFile.renameTo(jarFile)) {
> - scratchFile.delete();
> + if (!tmpFile.renameTo(jarFile)) {
>
> throw new IOException(getMsg("error.write.file"));
> }
> - scratchFile.delete();
> + }
> + } finally {
>
> + tmpFile.delete();
> + }
> }
>
>
> ----
>
> Webrev updated.
> http://cr.openjdk.java.net/~martin/jar-misc/
> <http://cr.openjdk.java.net/%7Emartin/jar-misc/>
>
> There are now many small changes combined in this fix. Sorry about that.
> I'd be more inclined to separate them if creating new bugs was easier.
>
> I'm not planning on making any more changes. Really.
>
> Martin
>
> On Thu, Jun 25, 2009 at 12:00, Martin Buchholz <martinrb at google.com
> <mailto:martinrb at google.com>> wrote:
>
> I have an updated version of this fix, with these changes:
>
> - Documented the turkish i problem
>
> /**
> * Compares two strings for equality, ignoring case. The second
> * argument must contain only upper-case ASCII characters.
> * We don't want case comparison to be locale-dependent (else we
> * have the notorious "turkish i bug").
> */
> private boolean equalsIgnoreCase(String s, String upper) {
>
> - Refactored code so that updateEntry now also sets the method to
> STORED.
>
> /**
> * Updates a ZipEntry which describes the data read by this
> * output stream, in STORED mode.
> */
> public void updateEntry(ZipEntry e) {
> e.setMethod(ZipEntry.STORED);
> e.setSize(n);
> e.setCrc(crc.getValue());
> }
>
> - addIndex was never updating the size in the ZipEntry (as required),
> which was not previously noticed because closeEntry was never
> called.
>
> private void addIndex(JarIndex index, ZipOutputStream zos)
> throws IOException
> {
> ZipEntry e = new ZipEntry(INDEX_NAME);
> e.setTime(System.currentTimeMillis());
> if (flag0) {
> CRC32OutputStream os = new CRC32OutputStream(crc32);
> index.write(os);
> os.updateEntry(e);
> }
> zos.putNextEntry(e);
> index.write(zos);
> zos.closeEntry();
>
> }
>
> http://cr.openjdk.java.net/~martin/jar-misc/
> <http://cr.openjdk.java.net/%7Emartin/jar-misc/>
> Previous webrev:
> http://cr.openjdk.java.net/~martin/jar-misc.0/
> <http://cr.openjdk.java.net/%7Emartin/jar-misc.0/>
>
> Martin
>
>
>
> On Wed, Jun 24, 2009 at 19:34, Martin Buchholz
> <martinrb at google.com <mailto:martinrb at google.com>> wrote:
>
> Hi jar team,
>
> I have a bunch of minor improvements to
> src/share/classes/sun/tools/jar/Main.java
>
> Toby and Xueming, please review.
>
> Warning: the index code has not been maintained for many years.
>
> Xueming, please file a bug.
>
> Synopsis: Miscellaneous improvements to "jar".
> Description:
> - Use standard jdk coding style for javadoc
> - Don't create a temp file for jar index in STORED mode.
> - Don't use synchronized collections.
> - Fix javac warnings.
> - Don't define new names for things like INDEX_NAME;
> use static import instead.
> - more efficiently compare special file names in update mode.
> Update mode should be measurably faster.
> - make CRC32OutputStream a nested class.
> refactor crc32.reset and updating entry into CRC32OutputStream.
> - Fix apparently benign bug updating n in
> CRC32OutputStream.write(byte[], int, int)
>
> Evaluation: Yep.
>
> http://cr.openjdk.java.net/~martin/jar-misc/
> <http://cr.openjdk.java.net/%7Emartin/jar-misc/>
>
> Thanks,
>
> Martin
>
>
>
More information about the core-libs-dev
mailing list