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>