On Sun, Jun 28, 2009 at 22:47, Xueming Shen <xueming.s...@sun.com> wrote:
>
> Looks fine.
>
> (1) "import java.nio.file.Paths" is not used by anyone.
Let's make sure to start using nio2 then...
>
> (2)nio2 APIs are good, but now I can't not just copy/paste the jar Main to
> 6.x:-)
>
That *is* a problem. Fortunately, it's a small change.
Is there anything else stopping the jdk6 version from being identical to
jdk7?
Martin
>
> sherman
>
>
> Martin Buchholz wrote:
>
>>
>>
>> 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/>
>> <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/>
>> <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/>
>> <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/>
>> <http://cr.openjdk.java.net/%7Emartin/jar-misc/>
>>
>> Thanks,
>>
>> Martin
>>
>>
>>
>>
>>
>>
>
