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]

[Xueming Shen]

On 12/14/15 1:13 AM, Seán Coffey wrote:
> 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.
Hi Sean,

The "zfile" does not need to close if  "new RandomAccessFile) throws an 
exception here.
And we need to keep the "zfile" open/alive if everything works well, so 
it appears the
try-with-resourecs does not really help anything here.

Thanks,
Sherman

> 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
>>>
>>
>