<br><br><div class="gmail_quote">On Sun, Jun 28, 2009 at 22:47, Xueming Shen <span dir="ltr"><<a href="mailto:Xueming.Shen@sun.com">Xueming.Shen@sun.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;">
<br>
Looks fine.<br>
<br>
(1) "import java.nio.file.Paths" is not used by anyone.</blockquote><div><br>Let's make sure to start using nio2 then...<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
(2)nio2 APIs are good, but now I can't not just copy/paste the jar Main to 6.x:-)<br>
</blockquote><div><br>That *is* a problem. Fortunately, it's a small change.<br><br>Is there anything else stopping the jdk6 version from being identical to jdk7?<br><br>Martin<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
sherman<br>
<br>
<br>
Martin Buchholz wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div class="h5">
<br>
<br>
On Fri, Jun 26, 2009 at 10:33, Xueming Shen <<a href="mailto:Xueming.Shen@sun.com" target="_blank">Xueming.Shen@sun.com</a> <mailto:<a href="mailto:Xueming.Shen@sun.com" target="_blank">Xueming.Shen@sun.com</a>>> wrote:<br>
<br>
The latest version looks good. 2 nit comments<br>
<br>
(1) it's reasonable to have createTempFileInSamDirectoryAs<br>
separate out, but I would<br>
keep the directoryOf within it. Yes, it's "clearer":-)<br>
<br>
<br>
Done.<br>
<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)<br>
throws IOException {<br>
- return File.createTempFile("jartmp", null, directoryOf(file));<br>
+ File dir = file.getParentFile();<br>
+ if (dir == null)<br>
+ dir = new File(".");<br>
+ return File.createTempFile("jartmp", null, dir);<br>
}<br>
private boolean ok;<br>
<br>
<br>
<br>
(2)the "updateOK" in dumpIndex really serves nobody but "clearer":-)<br>
<br>
<br>
I tried to improve dumpIndex by<br>
- removing updateOk variable.<br>
- using Path.moveTo is more likely to be atomic, so that<br>
a concurrent process is less likely to find the jar being updated missing.<br>
(Alan: is there a better way to do the common task of replacing a file<br>
with its transformed output?)<br>
<br>
*/<br>
void dumpIndex(String rootjar, JarIndex index) throws IOException {<br>
File jarFile = new File(rootjar);<br>
- File tmpFile = createTempFileInSameDirectoryAs(jarFile);<br>
+ Path jarPath = jarFile.toPath();<br>
+ Path tmpPath = createTempFileInSameDirectoryAs(jarFile).toPath();<br>
try {<br>
- boolean updateOk = update(new FileInputStream(jarFile),<br>
- new FileOutputStream(tmpFile),<br>
- null, index);<br>
- if (updateOk) {<br>
- jarFile.delete();<br>
- if (!tmpFile.renameTo(jarFile)) {<br>
- throw new IOException(getMsg("error.write.file"));<br>
+ if (update(jarPath.newInputStream(),<br>
+ tmpPath.newOutputStream(),<br>
+ null, index)) {<br>
+ try {<br>
+ tmpPath.moveTo(jarPath, REPLACE_EXISTING);<br>
+ } catch (IOException e) {<br>
+ throw new IOException(getMsg("error.write.file"), e);<br>
}<br>
}<br>
} finally {<br>
- tmpFile.delete();<br>
+ tmpPath.delete(false);<br>
}<br>
}<br>
<br>
webrev updated.<br>
Martin<br>
<br>
<br>
<br>
<br>
sherman<br>
<br>
btw, while you are here:-) do you have time to make the update()<br>
NOT to put jarIndex the<br>
first one when there is a manifest present? The JarInputStream<br>
assumes the manifest is always<br>
the first entry (or the second if the manifest dir is the first),<br>
which causes problem...I'm not saying<br>
to mix with this one:-) just in case you have time and interested<br>
while you are here:-)<br>
<br>
<br>
Martin Buchholz wrote:<br>
<br>
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 "."<br>
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<br>
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<br>
as the<br>
* specified file. A variant of File.createTempFile.<br>
*/<br>
private static File createTempFileInSameDirectoryAs(File<br>
file) throws IOException {<br>
return File.createTempFile("jartmp", null,<br>
directoryOf(file));<br>
}<br>
<br>
---<br>
<br>
While we're here, let's remove the double buffering on file<br>
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<br>
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<br>
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<br>
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<br>
make that<br>
private to CRC32OutputStream.<br>
+ private static class CRC32OutputStream extends<br>
java.io.OutputStream {<br>
+ final CRC32 crc = new CRC32();<br>
<br>
----<br>
<br>
We should add a try { finally } to delete the tempfile for jar i<br>
+ try {<br>
boolean updateOk = update(new FileInputStream(jarFile),<br>
- new<br>
FileOutputStream(scratchFile),<br>
<br>
+ new<br>
FileOutputStream(tmpFile),<br>
null, index);<br>
+ if (updateOk) {<br>
jarFile.delete();<br>
<br>
- if (!scratchFile.renameTo(jarFile)) {<br>
- scratchFile.delete();<br>
+ if (!tmpFile.renameTo(jarFile)) {<br>
<br>
throw new IOException(getMsg("error.write.file"));<br>
}<br>
- scratchFile.delete();<br>
+ }<br>
+ } finally {<br>
<br>
+ tmpFile.delete();<br>
+ }<br>
}<br>
----<br>
<br>
Webrev updated.<br>
<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/~martin/jar-misc/</a><br>
<<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<br>
<br>
There are now many small changes combined in this fix. Sorry<br>
about that.<br>
I'd be more inclined to separate them if creating new bugs was<br>
easier.<br>
<br>
I'm not planning on making any more changes. Really.<br>
<br>
Martin<br>
<br>
On Thu, Jun 25, 2009 at 12:00, Martin Buchholz<br>
<<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a> <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>><br></div></div><div><div></div><div class="h5">
<mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a> <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>>>> wrote:<br>
<br>
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.<br>
The second<br>
* argument must contain only upper-case ASCII characters.<br>
* We don't want case comparison to be locale-dependent<br>
(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<br>
method to<br>
STORED.<br>
<br>
/**<br>
* Updates a ZipEntry which describes the data read<br>
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<br>
required),<br>
which was not previously noticed because closeEntry was never<br>
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<br>
CRC32OutputStream(crc32);<br>
index.write(os);<br>
os.updateEntry(e);<br>
}<br>
zos.putNextEntry(e);<br>
index.write(zos);<br>
zos.closeEntry();<br>
<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>
<<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<br>
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>
<<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc.0/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc.0/</a>><br>
<<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc.0/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc.0/</a>><br>
<br>
<br>
Martin<br>
<br>
<br>
<br>
On Wed, Jun 24, 2009 at 19:34, Martin Buchholz<br>
<<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a> <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>><br></div></div><div><div></div><div class="h5">
<mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a> <mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>>>> wrote:<br>
<br>
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<br>
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<br>
mode.<br>
Update mode should be measurably faster.<br>
- make CRC32OutputStream a nested class.<br>
refactor crc32.reset and updating entry into<br>
CRC32OutputStream.<br>
- Fix apparently benign bug updating n in<br>
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>
<<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<<a href="http://cr.openjdk.java.net/%7Emartin/jar-misc/" target="_blank">http://cr.openjdk.java.net/%7Emartin/jar-misc/</a>><br>
<br>
Thanks,<br>
<br>
Martin<br>
<br>
<br>
<br>
<br>
<br>
</div></div></blockquote>
<br>
</blockquote></div><br>