8281962: Avoid unnecessary native calls in InflaterInputStream #7492

simonis · 2022-02-16T09:30:46Z

Currently, InflaterInputStream::read() first does a native call to the underlying zlib inflate() function and only afterwards checks if the inflater requires input (i.e. Inflater::needsInput()) or has finished inflating (Inflater::finished()). This leads to an unnecessary native call to inflate() when InflaterInputStream::read() is invoked for the very first time because Inflater::fill() hasn't been called and another unnecessary native call to detect the end of the stream. For small streams/files which completely fit into the output buffer passed to InflaterInputStream::read() we can therefore save two out of three native calls. The attached micro benchmark shows that this results in a 5%-10% performance improvement for zip files of sizes between 4096 to 256 bytes (notice that in common jars like Tomcat, Spring-Boot, Maven, Jackson, etc. about 60-80% of the classes are smaller than 4096 bytes).

before JDK-8281962
------------------
Benchmark                                     (size)  Mode  Cnt  Score   Error  Units
InflaterInputStreams.inflaterInputStreamRead     256  avgt    5  2.571 ± 0.120  us/op
InflaterInputStreams.inflaterInputStreamRead     512  avgt    5  2.861 ± 0.064  us/op
InflaterInputStreams.inflaterInputStreamRead    4096  avgt    5  5.110 ± 0.278  us/op

after JDK-8281962
-----------------
Benchmark                                     (size)  Mode  Cnt  Score   Error  Units
InflaterInputStreams.inflaterInputStreamRead     256  avgt    5  2.332 ± 0.081  us/op
InflaterInputStreams.inflaterInputStreamRead     512  avgt    5  2.691 ± 0.293  us/op
InflaterInputStreams.inflaterInputStreamRead    4096  avgt    5  4.812 ± 1.038  us/op

Tested with the JTreg zip/jar/zipfs and the JCK zip/jar tests.

As a side effect, this change also fixes an issue with alternative zlib implementations like zlib-Chromium or zlib-Cloudflare which pad the inflated bytes with a specif byte pattern at the end of the output for debugging purpose. This breaks code patterns like the following:

int readcount = 0;
while ((bytesRead = inflaterInputStream.read(data, 0, bufferSize)) != -1) {
    outputStream.write(data, 0, bytesRead);
    readCount++;
}
if (readCount == 1) {
    return data;         //  <---- first bytes might be overwritten
}
return outputStream.toByteArray();

Even if the whole data fits into the data array during the first call to inflaterInputStream.read(), we still need a second call to inflaterInputStream.read() in order to detect the end of the inflater stream. If this second call calls into the native inflate() function of Cloudflare/Chromium's zlib version, this will still write some padding bytes at the beginning of the data array and overwrite the previously read data. This issue has been reported in Spring [1] and ASM [2] when using these libraries with Cloudflares zlib version (by setting LD_LIBRARY_PATH accordingly).

[1] spring-projects/spring-framework#27429
[2] https://gitlab.ow2.org/asm/asm/-/issues/317955

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8281962: Avoid unnecessary native calls in InflaterInputStream

Reviewers

Christoph Langer (@RealCLanger - Reviewer) ⚠️ Review applies to 8314380
Claes Redestad (@cl4es - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)
Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7492/head:pull/7492
$ git checkout pull/7492

Update a local copy of the PR:
$ git checkout pull/7492
$ git pull https://git.openjdk.java.net/jdk pull/7492/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7492

View PR using the GUI difftool:
$ git pr show -t 7492

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7492.diff

bridgekeeper · 2022-02-16T09:31:12Z

👋 Welcome back simonis! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-02-16T09:33:04Z

@simonis The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-02-16T09:36:36Z

Webrevs

01: Full - Incremental (c393b584)
00: Full (8314380e)

RealCLanger

Makes sense to me. Benchmark numbers look good.

openjdk · 2022-02-17T09:37:33Z

@simonis This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8281962: Avoid unnecessary native calls in InflaterInputStream

Reviewed-by: clanger, redestad, alanb, lancea

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 99 new commits pushed to the master branch:

2557ef8: 8282276: Problem list failing two Robot Screen Capture tests
6445ee4: 5041655: (ch) FileLock: negative param and overflow issues
7feabee: 8261407: ReflectionFactory.checkInitted() is not thread-safe
58e1882: 8282042: [testbug] FileEncodingTest.java depends on default encoding
3cb3867: 8281315: Unicode, (?i) flag and backreference throwing IndexOutOfBounds Exception
957dae0: 8280958: G1/Parallel: Unify marking code structure
e44d067: 8244593: Clean up GNM/NM after JEP 381
41355e2: 8276686: Malformed Javadoc inline tags in JDK source in /java/util/regex/Pattern.java
022d807: 8271008: appcds/*/MethodHandlesAsCollectorTest.java tests time out because of excessive GC (CodeCache GC Threshold) in loom
ab6d8e6: 8260328: Drop redundant CSS properties from java.desktop HTML files
... and 89 more: https://git.openjdk.java.net/jdk/compare/95f198b2b1b2d5437515dc837cc160e4224c0ff3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

AlanBateman · 2022-02-17T09:59:08Z

This change is probably okay but will require study to see if there are any side effects (sadly, this area has a history of side effects being reported months and years after a change). Would you mind holding off integrating this change until it has been reviewed by someone that works in the area?

cl4es

Code changes look good to me.

(Shouldn't the copyright header have a year?)

test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java

…s from comments

simonis · 2022-02-17T15:56:34Z

Makes sense to me. Benchmark numbers look good.

Thanks Christoph!

simonis · 2022-02-17T15:59:11Z

This change is probably okay

Hi Alan,

thanks for looking at this change.

but will require study to see if there are any side effects (sadly, this area has a history of side effects being reported months and years after a change). Would you mind holding off integrating this change until it has been reviewed by someone that works in the area?

Sure, no problem. But can you please be a little more specific on who you consider to qualify as "works in the area" :)

LanceAndersen · 2022-02-17T20:58:54Z

The change looks innocuous so it is probably OK. I would like to kick of our Mach5 runs to see if it shakes out any potential issues.

From reading the 3rd party problem reports, it appears that the problem is with the 3rd party zlib implementations. Hopefully this change will not mask other issues

simonis · 2022-02-18T08:46:51Z

The change looks innocuous so it is probably OK. I would like to kick of our Mach5 runs to see if it shakes out any potential issues.

Thanks Lance! Much appreciated.

From reading the 3rd party problem reports, it appears that the problem is with the 3rd party zlib implementations. Hopefully this change will not mask other issues

The problem arises from a difference in the specification of zlib's inflate function and Java's Input stream. Basically, both take a buffer and the length of that buffer as input and return the number of bytes (i.e. payload) written into that buffer (which can be smaller than length). However, zlib doesn't specify that bytes between the returned length and the the buffer length can't be modified while Java does.

Mark Adler's original zlib version never wrote more bytes into the buffer than it returned as length value and users of his implementation started to more or less rely on this implementation detail. But newer and improved versions of zlib might write more bytes into the output buffer than they return as length value (e.g. because they use vector instructions for writes). According to zlib's inflate specification this is fine as long as they don't overwrite the end of the buffer and as long as they return the correct length of inflated bytes.

In order to make this behavior more evident, Chromium's zlib version puts some extra padding bytes after the last inflated byte if there's enough space left in the buffer (and this happens even if zero bytes were inflated). This behavior becomes particularly harmful if Java's InflaterInputStream unnecessarily calls the native inflate() function just to find out that there's no data left to inflate. With Chromium's zlib this will still write the padding bytes to the beginning of the output buffer and potentially overwrite the inflated output from the last call to InflaterInputStream::read.

So to cut a long story short, there's no problem with Chromium's zlib implementation. It behaves correctly according to the zlib specification. This change (besides the performance improvements) helps using other zlib implementations which behave correctly but slightly different from the original zlib implementation into Java.

simonis · 2022-02-21T10:06:40Z

The change looks innocuous so it is probably OK. I would like to kick of our Mach5 runs to see if it shakes out any potential issues.

@LanceAndersen , did you manage to get any Mach5 results? Did you find any issues?

AlanBateman

I think this change is okay. We also have to be super cautious when touching InflaterXXX and friends but I think one is okay.

cl4es

Thanks for fixing the microbenchmark to not have hardcoded limitations!

LanceAndersen · 2022-02-21T18:05:08Z

The change looks innocuous so it is probably OK. I would like to kick of our Mach5 runs to see if it shakes out any potential issues.

@LanceAndersen , did you manage to get any Mach5 results? Did you find any issues?

Tests are still running so no final results yet (please note that today is also a US holiday)

simonis · 2022-02-22T08:56:36Z

The change looks innocuous so it is probably OK. I would like to kick of our Mach5 runs to see if it shakes out any potential issues.

@LanceAndersen , did you manage to get any Mach5 results? Did you find any issues?

Tests are still running so no final results yet (please note that today is also a US holiday)

No problem. Just let me know once the tests have finished.

And thanks @AlanBateman, @cl4es for the reviews!

LanceAndersen

Mach5 runs did not raise any issues, so you should be OK to move forward.

simonis · 2022-02-23T08:34:29Z

Thanks @LanceAndersen!

simonis · 2022-02-23T08:34:52Z

/integrate

openjdk · 2022-02-23T08:36:26Z

Going to push as commit 378fa50.
Since your change was applied there have been 101 commits pushed to the master branch:

e1060be: 8281615: Deadlock caused by jdwp agent
6f882de: 8280964: [Linux aarch64] : drawImage dithers TYPE_BYTE_INDEXED images incorrectly
2557ef8: 8282276: Problem list failing two Robot Screen Capture tests
6445ee4: 5041655: (ch) FileLock: negative param and overflow issues
7feabee: 8261407: ReflectionFactory.checkInitted() is not thread-safe
58e1882: 8282042: [testbug] FileEncodingTest.java depends on default encoding
3cb3867: 8281315: Unicode, (?i) flag and backreference throwing IndexOutOfBounds Exception
957dae0: 8280958: G1/Parallel: Unify marking code structure
e44d067: 8244593: Clean up GNM/NM after JEP 381
41355e2: 8276686: Malformed Javadoc inline tags in JDK source in /java/util/regex/Pattern.java
... and 91 more: https://git.openjdk.java.net/jdk/compare/95f198b2b1b2d5437515dc837cc160e4224c0ff3...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-23T08:36:41Z

@simonis Pushed as commit 378fa50.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8281962: Avoid unnecessary native calls in InflaterInputStream

8314380

openjdk bot added the rfr Pull request is ready for review label Feb 16, 2022

openjdk bot added the core-libs core-libs-dev@openjdk.org label Feb 16, 2022

RealCLanger approved these changes Feb 17, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 17, 2022

cl4es suggested changes Feb 17, 2022

View reviewed changes

test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java Outdated Show resolved Hide resolved

Changed hardcoded constant to JMH parmater and removed non-ASCII char…

c393b58

…s from comments

AlanBateman approved these changes Feb 21, 2022

View reviewed changes

cl4es approved these changes Feb 21, 2022

View reviewed changes

LanceAndersen approved these changes Feb 22, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 23, 2022

openjdk bot closed this Feb 23, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8281962: Avoid unnecessary native calls in InflaterInputStream #7492

8281962: Avoid unnecessary native calls in InflaterInputStream #7492

simonis commented Feb 16, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 16, 2022

openjdk bot commented Feb 16, 2022

mlbridge bot commented Feb 16, 2022 •

edited

Loading

RealCLanger left a comment

openjdk bot commented Feb 17, 2022 •

edited

Loading

AlanBateman commented Feb 17, 2022

cl4es left a comment

simonis commented Feb 17, 2022

simonis commented Feb 17, 2022

LanceAndersen commented Feb 17, 2022

simonis commented Feb 18, 2022

simonis commented Feb 21, 2022

AlanBateman left a comment

cl4es left a comment

LanceAndersen commented Feb 21, 2022

simonis commented Feb 22, 2022

LanceAndersen left a comment

simonis commented Feb 23, 2022

simonis commented Feb 23, 2022

openjdk bot commented Feb 23, 2022

openjdk bot commented Feb 23, 2022

8281962: Avoid unnecessary native calls in InflaterInputStream #7492

8281962: Avoid unnecessary native calls in InflaterInputStream #7492

Conversation

simonis commented Feb 16, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 16, 2022

openjdk bot commented Feb 16, 2022

mlbridge bot commented Feb 16, 2022 • edited Loading

Webrevs

RealCLanger left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 17, 2022 • edited Loading

AlanBateman commented Feb 17, 2022

cl4es left a comment

Choose a reason for hiding this comment

simonis commented Feb 17, 2022

simonis commented Feb 17, 2022

LanceAndersen commented Feb 17, 2022

simonis commented Feb 18, 2022

simonis commented Feb 21, 2022

AlanBateman left a comment

Choose a reason for hiding this comment

cl4es left a comment

Choose a reason for hiding this comment

LanceAndersen commented Feb 21, 2022

simonis commented Feb 22, 2022

LanceAndersen left a comment

Choose a reason for hiding this comment

simonis commented Feb 23, 2022

simonis commented Feb 23, 2022

openjdk bot commented Feb 23, 2022

openjdk bot commented Feb 23, 2022

simonis commented Feb 16, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 16, 2022 •

edited

Loading

openjdk bot commented Feb 17, 2022 •

edited

Loading