RFR: 8265448: (zipfs): Reduce read contention in ZipFileSystem [v4]

[Jason Zaugg]

On Thu, 6 May 2021 08:05:12 GMT, Jason Zaugg <jzaugg at openjdk.org> wrote:

>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1235:
>> 
>>> 1233:             synchronized(ch) {
>>> 1234:                 return ch.position(pos).read(bb);
>>> 1235:             }
>> 
>> I think it's okay to include the update to EntryInputStream, that part looks fine, as does the directly use of the FileChannel positional read.
>> 
>> I'm still mulling over the case where ch is not a FileChannel as I would expected it to capture the existing position and restore it after the read. I think this is the degenerative case when the zip file is located in a custom file system that doesn't support FileChannel. In that case, positional read has to be implemented on the most basic SeekableByteChannel. It would only be observed when mixing positional read ops with other ops that depend on the current position.
>
> Here are all the references to `ch`.
> 
> 
> this.ch = Files.newByteChannel(zfpath, READ);
> ...
> this.ch.close();
> ...
> ch.close();              // close the ch just in case no update
> ...
> if (ch instanceof FileChannel fch) {
>     return fch.read(bb, pos);
> } else {
>     synchronized(ch) {
>         return ch.position(pos).read(bb);
>     }
> }
> ...
> long ziplen = ch.size();
> ...
> ch.close();
> 
> 
> It appears the only position-dependent operation called `read(ByteBuffer)`. This is performed together with the `pos` call within the `synchronized(ch)` lock.

I have confirmed that the non-`FileChannel` code path is exercised by existing tests.

test/jdk/jdk/nio/zipfs/ZipFSTester.java includes a test that forms a file system based on a JAR that is itself an entry within another `ZipFileSystem`.

Sample stacks:


java.lang.Throwable: readFullyAt. ch.getClass=class jdk.nio.zipfs.ByteArrayChannel
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.readFullyAt(ZipFileSystem.java:1234)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.readFullyAt(ZipFileSystem.java:1226)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$EntryInputStream.initDataPos(ZipFileSystem.java:2259)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$EntryInputStream.read(ZipFileSystem.java:2201)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$2.fill(ZipFileSystem.java:2151)
	at java.base/java.util.zip.InflaterInputStream.read(InflaterInputStream.java:158)
	at ZipFSTester.checkEqual(ZipFSTester.java:858)
	at ZipFSTester.test1(ZipFSTester.java:259)



java.lang.Throwable: readFullyAt. ch.getClass=class jdk.nio.zipfs.ByteArrayChannel
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem.readFullyAt(ZipFileSystem.java:1234)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$EntryInputStream.read(ZipFileSystem.java:2214)
	at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$2.fill(ZipFileSystem.java:2151)
	at java.base/java.util.zip.InflaterInputStream.read(InflaterInputStream.java:158)
	at ZipFSTester.checkEqual(ZipFSTester.java:858)
	at ZipFSTester.test1(ZipFSTester.java:259)


This use case is not covered by the `ZipFSTester.test2`, a multi-threaded test.

While looking at the test I noticed false warnings in the output: `read()/position() failed`. This did not actually fail the test. I investigated this and a) fixed the condition to deal with the edge case of zero-length entries and b) throw an "check failed" exception when the assertion fails.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3853