RFR: 8374644: Regression in GZIPInputStream performance after JDK-7036144 [v2]

[Lance Andersen]

On Thu, 8 Jan 2026 12:10:36 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 82:
>> 
>>> 80:         usesDefaultInflater = true;
>>> 81:         try {
>>> 82:             readHeader(in, true);
>> 
>> Any consideration given to using an enum or constant to make it a bit clearer on what the true represents realizing there is a comment in the readHeader method.  Not a big deal either way
>
> Hello Lance, I hadn't considered those options. Given that this is a private method being invoked only in 2 places, I picked the the trivial way to pass around the boolean. If it's OK I'll let it stay in this manner for this PR.

Hi jai,

‘keeping as is is fine. Perhaps if you are making additional updates just add a comment to what make it clear as to why true is passed here

>> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 219:
>> 
>>> 217:         } else {
>>> 218:             magic = readUShort(in);
>>> 219:         }
>> 
>> Understand what you are doing but perhaps consider adding more clarity to the comments as you are now  cobbling together the int which should contain the GZIP header magic number
>
> Do you mean additional comments for the enclosing `if` block? That piece of code is the same as what's currently present in the implementation of `readUShort(...)` except that we won't throw a `EOFException`. Would an additional comment like this be good:
> 
> 
> // read an unsigned short value representing the GZIP magic header.
> // this is the same as calling readUShort(in), except that here,
> // when reading the first byte, we don't raise an EOFException
> // if the stream has already reached EOF.

Yes that is fine, just helps to,clarify given the code is outside readUShort which will make things clearer for others later on

>> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 260:
>> 
>>> 258:         }
>>> 259:         crc.reset();
>>> 260:         assert n > 0 : "incorrect number of header bytes: " + n;
>> 
>> No problem with this but wondering if we should add this later and look at other places of interest
>
> The new place where we return the -1 (representing EOF) is a bit far away from here, so I wanted to be sure that we only return positive values here. It's not too important though, and I just noticed that there aren't any other asserts in this file. So yes, I'll remove this from the PR and we can think of introducing it later if necessary.

Sounds good

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

PR Review Comment: https://git.openjdk.org/jdk/pull/29092#discussion_r2672162403
PR Review Comment: https://git.openjdk.org/jdk/pull/29092#discussion_r2672170548
PR Review Comment: https://git.openjdk.org/jdk/pull/29092#discussion_r2672163374