Miscellaneous improvements to "jar".
Xueming Shen
Xueming.Shen at Sun.COM
Mon Jun 29 05:47:10 UTC 2009
Looks fine.
(1) "import java.nio.file.Paths" is not used by anyone.
(2)nio2 APIs are good, but now I can't not just copy/paste the jar Main
to 6.x:-)
sherman
Martin Buchholz wrote:
>
>
> On Fri, Jun 26, 2009 at 10:33, Xueming Shen <Xueming.Shen at sun.com
> <mailto:Xueming.Shen at 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
> <martinrb at google.com <mailto:martinrb at google.com>
> <mailto:martinrb at google.com <mailto:martinrb at 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
> <martinrb at google.com <mailto:martinrb at google.com>
> <mailto:martinrb at google.com <mailto:martinrb at 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
>
>
>
>
>
More information about the core-libs-dev
mailing list