RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Xueming Shen]

Steve,

I would assume the difference of the webrev.04 "old" iterator and the 
webrev.06 "new" iterator
that might trigger a performance is how you create the JarFileEntry. The 
one parameter constructor
invokes isMultiRelease(), which might be relatively expensive, when the 
"mr" is enabled. There
are couple "if" checks involved.

For the "new" iterator is slower than the current jdk9 one. It might be 
desired to have two JarEntryIterator
classes defined, one for "notVersioned", one for the "versioned". Of 
course keep the two parameter
constructor for the "notVersioned". This might bring performance back 
for the normal "notVersioned"
usage, which I would assume is the majority.

There is also a "bug" in the new iterator. The iterator/Enumeration 
returned from ZipFile.entries()
checks "ensureOpen() in both hasNext() and next(). So close() the 
ZipFile after itr.hasNext() will
fails the next next() invocation in the old implementation. The latest 
itr will returns the cached
"ze". Not a big issue for sure, but a kind of "regression". Defining two 
iterator classes as suggested
above will also workaround this issue, as the "notVersioned" one will 
work just as expected, no
regression.

-Sherman

On 2/9/16 5:04 PM, Steve Drach wrote:
> Hi,
>
> Yet another webrev, 
> http://cr.openjdk.java.net/~sdrach/8132734/webrev.06/ 
> <http://cr.openjdk.java.net/%7Esdrach/8132734/webrev.06/>, with a 
> change to JarEntryIterator to fix a problem discovered by performance 
> tests — calling hasNext() twice as often as needed.  I also removed 
> the @since 9 tags from the methods entries() and stream(), and added 
> an additional sentence to the spec for each of those methods that 
> clarifies what a base entry is (actually is not).
>
>> I think having stream and entries do this is right although I would 
>> like to see some performance data if possible.
>
> See 
> http://cr.openjdk.java.net/~sdrach/8132734/JarFile%20Performance.pdf 
> <http://cr.openjdk.java.net/%7Esdrach/8132734/JarFile%20Performance.pdf>
>
> I used JMH to run the benchmark.  See 
> http://cr.openjdk.java.net/~sdrach/8132734/MyBenchmark.java 
> <http://cr.openjdk.java.net/%7Esdrach/8132734/MyBenchmark.java>.  I 
> used two jar files, the rt.jar file from JDK 8 that has 20653 entries 
> and the multi-release.jar found in the test directory with 14 entries. 
>  Obviously rt.jar is not a multi-release jar file.
>
> The first two tables (1 and 2) are comparable and the second two 
> tables are somewhat comparable (3 and 4).
>
> Tables 1 and 2 have 4 sections that show the results of tests on the 
> two jar files in 4 configurations of JarFile.  The tests were done 
> with a JarFile object constructed without the Release object argument, 
> essentially the legacy constructor.  The section labeled "JDK 8 
> JarFile” was done with JDK 8u66.  The section labeled “JDK 9 JarFile” 
> was done with the latest build of openjdk/jdk9/dev without any changes 
> in my 8132734 changeset.  I chose this section as the reference, so 
> the last column shows the values normalized to 1 micro/milli second 
> per operation (rt.jar times are in milliseconds and multi-release.jar 
> times are in microseconds).  It should be obvious that JDK 9 is much 
> faster than JDK 8, somewhere on the order of 5-6 times faster.  I 
> think that is because ZipFile doesn’t use JNI in JDK 9.
>
> Of the two remaining sections in Tables 1 and 2, the section labeled 
> “MultiRelease JarFile” differs from the section labeled “MultiRelease 
> JarFile, new iterator” only in the JarEntryIterator class.  The first 
> one uses the original iterator in JarFile.java that can be seen 
> starting with line 551 of webrev.04 
> <http://cr.openjdk.java.net/%7Esdrach/8132734/webrev.04/>, and the new 
> iterator starts with line 553 of webrev.06 
> <http://cr.openjdk.java.net/%7Esdrach/8132734/webrev.06/>.  The 
> results are strange.  The new, more complex, iterator appears to be 
> faster than the old, simpler, iterator.  I double, and triple, checked 
> it, but it was always faster.  I used jitwatch to look at the hotspot 
> logs generated during compilation and neither method was compiled.  I 
> suppose I could dig into it further but decided not to.  Consider it 
> good news.  The results do show that the multi-release enhancement 
> slows JarFile entries/stream down by 2-18% depending on the size of 
> the jar file.  But they are still much better than the JDK 8 values.
>
>> Also I would expect that if a JAR file is not mult-release but the 
>> library opens it with Release.RUNTIME to perform the same as opening 
>> the JAR file with the Release-less constructors.
>
> The results in Table 3 attempts to answer this question since rt.jar 
> is not a multi-release jar file.  This tells me that if one opens the 
> JarFile with Release.RUNTIME, that there is a performance penalty of 
> 2-6% on this very large jar file.
>
> Finally, the results in Table 4 tell me that processing a true 
> multi-release jar file takes about 80% more time per entries() or 
> stream() operation.  I’ve looked at this in a profiler and there is no 
> particular area that stands out to me, it’s just more complicated to 
> process a multi-release jar file, as would be expected.
>
>> I think the javadoc will need to also need to make it clear whether 
>> entries with names starting with META-INF/versions/ are returned.
>
> Fixed.
>
>>
>> I see you've added @since 9 to the existing methods, I assume you 
>> didn't mean to do this.
>
> Fixed.
>
>>
>> At some point then we need to discuss how RUNTIME_VERSION is 
>> computed. Iris (via Mandy) has pushed jdk.Version to jdk9/dev but 
>> having it exported by java.base conflicts with the design principles 
>> in JEP 200. Moving it to another module means that code in java.base 
>> cannot use it and thus the JAR file can't use it.
>
> Left as is.
>