On Fri, Jun 26, 2009 at 10:33, Xueming Shen <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>> 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>> 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
>>
>>
>>
>>
>
