Hi Christoph, I have cleaned up, re-vamped and added more coverage to the test. As part of this I started to lay the foundation for removing some of the duplicate code as part of continued clean up and enhanced coverage going forward The revised webrev can be found at: http://cr.openjdk.java.net/~lancea/8211917/webrev.00/index.html <http://cr.openjdk.java.net/~lancea/8211917/webrev.00/index.html> Best Lance
On Feb 19, 2020, at 3:06 PM, Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> wrote:
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@sap.com <mailto:christoph.langer@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@oracle.com <mailto:lance.andersen@oracle.com>> Sent: Mittwoch, 19. Februar 2020 02:42 To: Langer, Christoph <christoph.langer@sap.com <mailto:christoph.langer@sap.com>> Cc: Jaikiran Pai <jai.forums2013@gmail.com <mailto:jai.forums2013@gmail.com>>; nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@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@sap.com <mailto:christoph.langer@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@openjdk.java.net <mailto:nio-dev-bounces@openjdk.java.net>> On Behalf Of Jaikiran Pai Sent: Montag, 17. Februar 2020 03:56 To: Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> Cc: nio-dev@openjdk.java.net <mailto:nio-dev@openjdk.java.net>; core-libs-dev@openjdk.java.net <mailto:core-libs-dev@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@oracle.com <mailto:Lance.Andersen@oracle.com>
<oracle_sig_logo.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> <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@oracle.com <mailto:Lance.Andersen@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@oracle.com <mailto:Lance.Andersen@oracle.com>