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
