RFR: 8341597: ZipFileInflaterInputStream input buffer size uses uncompressed size

[David Schlosnagle]

On Sun, 6 Oct 2024 19:17:24 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> Please review this PR which proposes to change the input buffer size of `ZipFileInflaterInputStream` to be based on the _compressed_ size of a ZIP entry instead of the _uncompressed_ size. This saves allocation since buffers will no longer be oversized:
>> 
>> * The `size` parameter passed to the `ZipFileInflaterInputStream` constructor is passed on to the superclass `InflaterInputStream` where it determines the size of the input buffer. This buffer is used to read compressed data and pass it on to the `Inflater`.
>> * `ZipFile:getInputStream` currently looks at the _uncompressed_ size when determining the input buffer size. It should instead use the _compressed_ size, since this buffer is used for compressed, not uncompressed data.
>> * The current implementation somewhat mysteriously adds 2 to the uncompressed size. My guess is that this is to allow fitting a two-byte DEFLATE header for empty files (where the uncompressed size is 0 and the compressed size is 2).
>> * There is a check for `size <= 0`. This condition is unreachable in the current code and in the PR as well, since the compressed size will always be `>= 2`. I propose we remove this check.
>> 
>> Performance: A benchmark which measures the cost of opening and closing input streams using `ZipFile::getInputStream` shows a modest improvement of ~5%, consistent with less allocation of unused buffer space.
>> 
>> Testing: No tests are added in this PR. The `noreg-cleanup` label is added in JBS. GHA testing is currently pending.
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 417:
> 
>> 415:                     if (size > 65536) {
>> 416:                         size = 8192;
>> 417:                     }
> 
> Not sure if this clamping makes sense? We clamp the size at 8192, but only when size is larger than 65536?
> 
> Wondering if we should simply clamp to 8192 instead:
> 
> 
> Suggestion:
> 
>                    int size = Math.clamp(CENSIZ(zsrc.cen, pos), 2, 8192);

I'm curious if it would be beneficial to increase the max clamp size to 16384 bytes similar to https://bugs.openjdk.org/browse/JDK-8299336 / https://github.com/openjdk/jdk/pull/11783

Suggestion:

                    int size = Math.clamp(CENSIZ(zsrc.can, pos), 2, 16384);

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21379#discussion_r1789364170