I did some benchmarking, <br>and found that my changes "only" make jar 10-20% faster.<br>Disappointing - we expect an order of magnitude for every commit!<br><br>While benchmarking, I discovered to my horror that the simple<br>
<br>jar cf /tmp/t1 ...<br>jar i /tmp/t1 <br><br>fails, because it tries to create the replacement jar in "." and then<br>rename() it into place.  Oops... Better refactor out the code that<br>puts the replacement temp file in the same directory.<br>
Better write some tests for that, too.<br><br>    /**<br>     * A variant of File.getParentFile that always returns a valid<br>     * directory (i.e. returns new File(".") where File.getParentFile<br>     * would return null).<br>
     */<br>    private static File directoryOf(File file) {<br>        File dir = file.getParentFile();<br>        return (dir != null) ? dir : new File(".");<br>    }<br><br>    /**<br>     * Creates a new empty temporary file in the same directory as the<br>
     * specified file.  A variant of File.createTempFile.<br>     */<br>    private static File createTempFileInSameDirectoryAs(File file) throws IOException {<br>        return File.createTempFile("jartmp", null, directoryOf(file));<br>
    }<br><br>---<br><br>While we're here,  let's remove the double buffering on file copy operations.<br>And the repeated allocations of big buffers.<br><br>    /**<br>     * A buffer for use only by copy(InputStream, OutputStream).<br>
     * Not as clean as allocating a new buffer as needed by copy,<br>     * but significantly more efficient.<br>     */<br>    private byte[] copyBuf = new byte[8192];<br><br>    /**<br>     * Copies all bytes from the input stream to the output stream.<br>
     * Does not close or flush either stream.<br>     *<br>     * @param from the input stream to read from<br>     * @param to the output stream to write to<br>     * @throws IOException if an I/O error occurs<br>     */<br>
    private void copy(InputStream from, OutputStream to) throws IOException {<br>        int n;<br>        while ((n = from.read(copyBuf)) != -1)<br>            to.write(copyBuf, 0, n);<br>    }<br><br>    /**<br>     * Copies all bytes from the input file to the output stream.<br>
     * Does not close or flush the output stream.<br>     *<br>     * @param from the input file to read from<br>     * @param to the output stream to write to<br>     * @throws IOException if an I/O error occurs<br>     */<br>
    private void copy(File from, OutputStream to) throws IOException {<br>        InputStream in = new FileInputStream(from);<br>        try {<br>            copy(in, to);<br>        } finally {<br>            in.close();<br>
        }<br>    }<br><br>    /**<br>     * Copies all bytes from the input stream to the output file.<br>     * Does not close the input stream.<br>     *<br>     * @param from the input stream to read from<br>     * @param to the output file to write to<br>
     * @throws IOException if an I/O error occurs<br>     */<br>    private void copy(InputStream from, File to) throws IOException {<br>        OutputStream out = new FileOutputStream(to);<br>        try {<br>            copy(from, out);<br>
        } finally {<br>            out.close();<br>        }<br>    }<br><br>----<br><br>On the other hand, allocating a CRC32 is very cheap, so let's make that<br>
private to CRC32OutputStream.<br>
<pre><span class="new">+    private static class CRC32OutputStream extends java.io.OutputStream {</span><br><span class="new">+        final CRC32 crc = new CRC32();</span><br><span class="new"></span></pre>
<br>
----<br><br>We should add a try { finally } to delete the tempfile for jar i<br><pre><span class="new">+        try {</span><br>         boolean updateOk = update(new FileInputStream(jarFile),<br><span class="removed">-                                  new FileOutputStream(scratchFile),</span><br>
<span class="new">+                                      new FileOutputStream(tmpFile),</span><br>                                   null, index);<br><span class="new">+            if (updateOk) {</span><br>         jarFile.delete();<br>
<span class="removed">-        if (!scratchFile.renameTo(jarFile)) {</span><br><span class="removed">-            scratchFile.delete();</span><br><span class="new">+                if (!tmpFile.renameTo(jarFile)) {</span><br>
             throw new IOException(getMsg("error.write.file"));<br>         }<br><span class="removed">-        scratchFile.delete();</span><br><span class="new">+            }</span><br><span class="new">+        } finally {</span><br>
<span class="new">+            tmpFile.delete();</span><br><span class="new">+        }</span><br>     }<br></pre><br>

----<br>
<br>Webrev updated.<br><a href="http://cr.openjdk.java.net/~martin/jar-misc/">http://cr.openjdk.java.net/~martin/jar-misc/</a><br><br>There are now many small changes combined in this fix.  Sorry about that.<br>I'd be more inclined to separate them if creating new bugs was easier.<br>
<br>I'm not planning on making any more changes.  Really.<br><br>Martin<br><br>
<div class="gmail_quote">On Thu, Jun 25, 2009 at 12:00, Martin Buchholz <span dir="ltr"><<a href="mailto:martinrb@google.com">martinrb@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
I have an updated version of this fix, with these changes:<br><br>- Documented the turkish i problem<br><br>    /**<br>     * Compares two strings for equality, ignoring case.  The second<br>     * argument must contain only upper-case ASCII characters.<br>

     * We don't want case comparison to be locale-dependent (else we<br>     * have the notorious "turkish i bug").<br>     */<br>    private boolean equalsIgnoreCase(String s, String upper) {<br><br>- Refactored code so that updateEntry now also sets the method to STORED.<br>

<br>        /**<br>         * Updates a ZipEntry which describes the data read by this<br>         * output stream, in STORED mode.<br>         */<br>        public void updateEntry(ZipEntry e) {<br>            e.setMethod(ZipEntry.STORED);<br>

            e.setSize(n);<br>            e.setCrc(crc.getValue());<br>        }<br><br>- addIndex was never updating the size in the ZipEntry (as required),<br>  which was not previously noticed because closeEntry was never called.<br>

<br>    private void addIndex(JarIndex index, ZipOutputStream zos)<br>        throws IOException<br>    {<br>        ZipEntry e = new ZipEntry(INDEX_NAME);<br>        e.setTime(System.currentTimeMillis());<br>        if (flag0) {<br>

            CRC32OutputStream os = new CRC32OutputStream(crc32);<br>            index.write(os);<br>            os.updateEntry(e);<br>        }<br>        zos.putNextEntry(e);<br>        index.write(zos);<br>        zos.closeEntry();<div class="im">
<br>
    }<br><br><a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/~martin/jar-misc/</a><br></div>Previous webrev:<br><a href="http://cr.openjdk.java.net/%7Emartin/jar-misc.0/" target="_blank">http://cr.openjdk.java.net/~martin/jar-misc.0/</a><br>
<font color="#888888">
<br>Martin</font><div><div></div><div class="h5"><br><br><br><div class="gmail_quote">On Wed, Jun 24, 2009 at 19:34, Martin Buchholz <span dir="ltr"><<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Hi jar team,<br><br>I have a bunch of minor improvements to <br> src/share/classes/sun/tools/jar/Main.java<br><br>Toby and Xueming, please review.<br><br>Warning: the index code has not been maintained for many years.<br>

<br>
Xueming, please file a bug.<br><br>Synopsis: Miscellaneous improvements to "jar".<br>Description:<br>- Use standard jdk coding style for javadoc<br>- Don't create a temp file for jar index in STORED mode.<br>


- Don't use synchronized collections.<br>- Fix javac warnings.<br>- Don't define new names for things like INDEX_NAME;<br>  use static import instead.<br>- more efficiently compare special file names in update mode.<br>


  Update mode should be measurably faster.<br>- make CRC32OutputStream a nested class.<br>  refactor crc32.reset and updating entry into CRC32OutputStream.<br>- Fix apparently benign bug updating n in CRC32OutputStream.write(byte[], int, int)<br>


<br>Evaluation: Yep.<br><br><a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/~martin/jar-misc/</a><br><br>Thanks,<br><font color="#888888"><br>Martin<br>
</font></blockquote></div><br>
</div></div></blockquote></div><br>