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]

Ah - of course. That comment would been lack of coffee syndrome on my 
behalf earlier this morning then ;)

Regards,
Sean.

On 14/12/15 16:36, Xueming Shen wrote:
> 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
>>>>
>>>
>>
>