RFR: JDK-8145260: To bring j.u.z.ZipFile's native implementation to Java to remove the expensive jni cost and mmap crash risk [2]

[Seán Coffey]

Sherman,

Changes look fine. One suggestion in ZipFile around line 956. Would you 
be better off managing the RandomAccessFile via try-with-resources. That 
would clean up your exception handling.

Regards,
Sean.

On 12/12/2015 18:43, Xueming Shen wrote:
> Hi,
>
> The changeset for JDK-8142508 had been backout because it triggers 
> jtreg fails in -avm mode
> with testng tests. Here is the updated version that fixes the problem. 
> I'm using a different issue
> to trace this update change.
>
> issue: https://bugs.openjdk.java.net/browse/JDK-8145260
> webrev: http://cr.openjdk.java.net/~sherman/8145260/webrev/
> diff: http://cr.openjdk.java.net/~sherman/8145260/diff  (diff to the 
> latest 8142508 version)
>
> The root cause/direct trigger that causes the jtreg failure is the 
> overlook in the new implementation.
> The flag "locsig" was implemented as
>
>  this.locsig = (LOCSIG(buf) != LOCSIG);
>
> while it really should be
>
>  this.locsig = (LOCSIG(buf) == LOCSIG);
>
> This is the flag that sun.misc.URLClassPath uses to check if a jar 
> file stars with the correct
> LOC (throws away if not) in certain circumstance (security manager + 
> !disable_jar_checking).
>
> This has been fixed in the new webrev, and I also renamed the field to 
> be more straightforward
> (locsig -> startsWithLoc)
>
> Other than that, there are two small updates
>
> (1) wrap the zip file initialization code with a try{} block and close 
> the raw file if any exception
> (2) reorder of "len <=0" check in ZipFileInputStream.read()
>
> All tests seem to pass now.
>
> Thanks,
> Sherman
>
> On 11/11/15 12:22 PM, Xueming Shen wrote:
>> Hi
>>
>> Please help review the changes for JDK-8142508
>>
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8142508
>> webrev: http://cr.openjdk.java.net/~sherman/8142508/webrev
>>
>> Mainly to address the issues in current j.u.z.ZipFile implementation 
>> as listed
>> below
>>
>> (1) The ZIP file format support code is in native C (shared with the 
>> VM via
>>      ZipFile.c -> zip_util.c). Any entry lookup, creation operation 
>> requires multiple
>>      round-trips of expensive jni calls.
>>
>> (2) The native C implementation which uses mmap to map in the central 
>> directory
>>      table appears to be a big risk of vm crash when the underlying 
>> jar file gets
>>      overwritten with new contents while it is still being used by 
>> other ZipFile. The
>>      crash reports keep coming in even after we have introduced in 
>> system flag
>>      to disable it (sun.zip.disableMemoryMapping).
>>
>> (3) The use of "filename + lastModified()" cache (zip_util.c) appears 
>> to be broken
>>      if the timestamp is in low resolution/precision 
>> (File.getModified() for example,
>>      might only have "second" ersolution on solaris/linux), and/or 
>> the file is being
>>      overwritten.
>>
>> The clean solution appears to bring the ZIP format support code 
>> completely from
>> native to Java to remove the jni invocation cost and the mmap risk. 
>> Also to use
>> the fileKey and lastModified from 
>> java.nio.file.attribute.BasicFileAttributes to have
>> better/correct cache matching.
>>
>> Benchmark:
>>
>> This simple jmh measurement suggests a big boost of the performance of
>> ZipFile.getEntry()/entries()/getStream() which are basically entry 
>> related
>> accesses (the "open only" has some regression, which is expected as we
>> switched from the mmap to simply read the cen table in into a byte[])
>>
>> http://cr.openjdk.java.net/~sherman/8142508/MyBenchmark.java
>>
>> # JDK9 base
>>
>> Benchmark                  Mode  Cnt    Score    Error  Units
>> MyBenchmark.testEntries    avgt   50   13.671 ±  0.385  ms/op
>> MyBenchmark.testGetEntry   avgt   50   17.414 ±  0.803  ms/op
>> MyBenchmark.testGetStream  avgt   50   42.398 ± 10.136  ms/op
>> MyBenchmark.testOpen       avgt   50    3.049 ±  0.094  ms/op
>> MyBenchmark.testRead       avgt   50  215.179 ±  9.926  ms/op
>> MyBenchmark.testReadAll    avgt   50  244.422 ± 19.375  ms/op
>> -------------------------------------------------------------------------------------- 
>>
>> # JDK9 ZipFile without jni invocation
>>
>> Benchmark                  Mode  Cnt    Score    Error  Units
>> MyBenchmark.testEntries    avgt   50    6.436 ±  0.422  ms/op
>> MyBenchmark.testGetEntry   avgt   50   10.021 ±  0.699  ms/op
>> MyBenchmark.testGetStream  avgt   50   38.713 ± 16.687  ms/op
>> MyBenchmark.testOpen       avgt   50    3.288 ±  0.119  ms/op
>> MyBenchmark.testRead       avgt   50  220.653 ±  8.529  ms/op
>> MyBenchmark.testReadAll    avgt   50  249.798 ± 18.438  ms/op
>> --------------------------------------------------------------------------------------- 
>>
>>
>> Test:
>> http://cr.openjdk.java.net/~sherman/8142508/webrev/test/java/util/zip/ZipFile/TestZipFile.java.html 
>>
>>
>> Verified the new ZipFile runs as expected when the underlying jar/zip 
>> file get
>> deleted/overwritten when the zip still be used. The "old" ZipFile 
>> fails to continue
>> to work but no crash, and the "new" one works correctly on updated 
>> zip file
>> without problem (The test runs a little long, have not decided if it 
>> should be
>> checked in as unit test).
>>
>> -Sherman
>>
>