RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Langer, Christoph]

Hi Lance, Jaikiran,

you’re probably right. Both, the gut feeling that using streams at all here will bring some performance penalties as well as the potential double traversal of the value map speak against the suggestion I made. I was actually not really convinced myself – however, I was looking for a way to write the coding in a more concise manner (without performance penalties, of course).

So, going back to Jaikiran’s latest proposal, I only have a few minor performance tweaks. Please look at this webrev I generated: http://cr.openjdk.java.net/~clanger/webrevs/8211917.0/

First, the manifest node should be looked up by the statement “inodes.get(IndexNode.keyOf(getBytes("/META-INF/MANIFEST.MF")))”. Using “getInode(getBytes("/META-INF/MANIFEST.MF"))” would incur an unnecessary Object.requireNonNull() check and would also call the “entryLookup” function that is needed for mapped entries in MR jars. But even in MR jars, the manifest would not be mapped to some other versioned node.

And then within the loop body I restructured the if statements such that in the case of no manifest (probably the most common case), just one “if (manifestInode == null)” check is performed and not another “if (inode == manifestInode)” after the call to inodeIterator.next().

So, just minor tweaks but probably still worthwile doing. ��

Note that I also added a space after ‘while’ to be consistent with the rest of the file.

Other than that I’m good with the change. The testcase looks comprising and zipfs tests are passing.

Best regards
Christoph


From: Lance Andersen <lance.andersen at oracle.com>
Sent: Mittwoch, 19. Februar 2020 02:42
To: Langer, Christoph <christoph.langer at sap.com>
Cc: Jaikiran Pai <jai.forums2013 at gmail.com>; nio-dev at openjdk.java.net; core-libs-dev at openjdk.java.net
Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

Hi Christoph


On Feb 17, 2020, at 11:26 AM, Langer, Christoph <christoph.langer at sap.com<mailto:christoph.langer at sap.com>> wrote:

Hi Jaikiran, Lance,

I'm a bit late to the game. It occurred to me, whether we could maybe use the streams API to process the inodes. Something like this:

diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
@@ -56,6 +56,7 @@
import java.util.jar.Attributes;
import java.util.jar.Manifest;
import java.util.regex.Pattern;
+import java.util.stream.Stream;
import java.util.zip.CRC32;
import java.util.zip.Deflater;
import java.util.zip.DeflaterOutputStream;
@@ -1730,7 +1731,17 @@
            Entry e;

            // write loc
-            for (IndexNode inode : inodes.values()) {
+
+            // write the manifest inode (if any) first in the loc so that the
+            // java.util.jar.JarInputStream can find it, since it expects it to be
+            // the first or second entry in the jar
+            final IndexNode manifestInode = getInode(getBytes("/META-INF/MANIFEST.MF"));
+            final Stream<IndexNode> inodeStream = manifestInode == null ?
+                inodes.values().stream() :
+                Stream.concat(Stream.of(manifestInode),
+                    inodes.values().stream().filter(node -> !manifestInode.equals(node)));
+
+            for (IndexNode inode : (Iterable<IndexNode>)inodeStream::iterator) {
                if (inode instanceof Entry) {    // an updated inode
                    e = (Entry)inode;
                    try {

What do you think?

I am not sure the use of Streams will help us too much here as in your proposed example I believe you would end out traversing the entire list of IndexNodes twice in the case of a Manifest existing.  Once when building the Stream and the other when you are walking through the IndexNodes to write them out.

Best
Lance


I didn't have time yet to have a closer look to the testcase. Will do tomorrow.

Cheers
Christoph


-----Original Message-----
From: nio-dev <nio-dev-bounces at openjdk.java.net<mailto:nio-dev-bounces at openjdk.java.net>> On Behalf Of Jaikiran
Pai
Sent: Montag, 17. Februar 2020 03:56
To: Lance Andersen <lance.andersen at oracle.com<mailto:lance.andersen at oracle.com>>
Cc: nio-dev at openjdk.java.net<mailto:nio-dev at openjdk.java.net>; core-libs-dev at openjdk.java.net<mailto:core-libs-dev at openjdk.java.net>
Subject: Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system
should put the MANIFEST.MF at the start

Hello Lance,

On 15/02/20 2:27 am, Lance Andersen wrote:

Hi Jaikiran,

I think the changes to ZipFileSystem are OK.

The test overall is good.  I am going to streamline it a bit and
remove the long lines (we try to keep lines to around 80 characters).
I'll keep that in mind for future changes. Thank you for taking care of
this.

-Jaikiran


[cid:image001.gif at 01D5E713.0B83F590]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com<mailto:Lance.Andersen at oracle.com>