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

Wed Feb 19 20:06:08 UTC 2020

Hi Christoph

I think your changes to ZipFileSystem look fine.  I am running it through mach5 now.  I am still planning to clean up the test a bit and will create a web rev when I do.

> On Feb 19, 2020, at 4:55 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
> 
> 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).
Understand  I did not come up with anything  but if we do we can always update it later.
>  
> 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/ <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. ��

All good
>  
> 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
Lance
>  
> Best regards
> Christoph
>  
>  
> From: Lance Andersen <lance.andersen at oracle.com <mailto:lance.andersen at oracle.com>> 
> Sent: Mittwoch, 19. Februar 2020 02:42
> To: Langer, Christoph <christoph.langer at sap.com <mailto:christoph.langer at sap.com>>
> Cc: Jaikiran Pai <jai.forums2013 at gmail.com <mailto:jai.forums2013 at gmail.com>>; 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
>  
> 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
>  
>  
> <image001.gif> <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>
>  
> 
> 
>  

 <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif> <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>