On Fri, Jun 26, 2009 at 10:33, Xueming Shen <xueming.s...@sun.com
<mailto:xueming.s...@sun.com>> wrote:
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":-)
Done.
}
/**
- * 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));
+ File dir = file.getParentFile();
+ if (dir == null)
+ dir = new File(".");
+ return File.createTempFile("jartmp", null, dir);
}
private boolean ok;
(2)the "updateOK" in dumpIndex really serves nobody but "clearer":-)
I tried to improve dumpIndex by
- removing updateOk variable.
- using Path.moveTo is more likely to be atomic, so that
a concurrent process is less likely to find the jar being updated
missing.
(Alan: is there a better way to do the common task of replacing a file
with its transformed output?)
*/
void dumpIndex(String rootjar, JarIndex index) throws IOException {
File jarFile = new File(rootjar);
- File tmpFile = createTempFileInSameDirectoryAs(jarFile);
+ Path jarPath = jarFile.toPath();
+ Path tmpPath = createTempFileInSameDirectoryAs(jarFile).toPath();
try {
- boolean updateOk = update(new FileInputStream(jarFile),
- new FileOutputStream(tmpFile),
- null, index);
- if (updateOk) {
- jarFile.delete();
- if (!tmpFile.renameTo(jarFile)) {
- throw new IOException(getMsg("error.write.file"));
+ if (update(jarPath.newInputStream(),
+ tmpPath.newOutputStream(),
+ null, index)) {
+ try {
+ tmpPath.moveTo(jarPath, REPLACE_EXISTING);
+ } catch (IOException e) {
+ throw new IOException(getMsg("error.write.file"), e);
}
}
} finally {
- tmpFile.delete();
+ tmpPath.delete(false);
}
}
webrev updated.
Martin
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/>
<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
<marti...@google.com <mailto:marti...@google.com>
<mailto:marti...@google.com <mailto:marti...@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/>
<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/>
<http://cr.openjdk.java.net/%7Emartin/jar-misc.0/>
Martin
On Wed, Jun 24, 2009 at 19:34, Martin Buchholz
<marti...@google.com <mailto:marti...@google.com>
<mailto:marti...@google.com <mailto:marti...@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/>
<http://cr.openjdk.java.net/%7Emartin/jar-misc/>
Thanks,
Martin
