openjdk
8292698: Improve performance of DataInputStream
I found out that reading from DataInputStream wrapping ByteArrayInputStream (as well as BufferedInputStream or any InputStream relying on byte[]) can be significantly improved by accessing volatile in field only once per operation.
Current implementation does it for each call of in.read(), i.e. in readInt() method we do it 4 times:
public final int readInt() throws IOException {
int ch1 = in.read();
int ch2 = in.read();
int ch3 = in.read();
int ch4 = in.read();
if ((ch1 | ch2 | ch3 | ch4) < 0)
throw new EOFException();
return ((ch1 << 24) + (ch2 << 16) + (ch3 << 8) + (ch4 << 0));
}
Apparently accessing volatile reference with underlying byte[] prevents runtime from doing some optimizations, so dereferencing local variable should be more efficient.
Benchmarking:
baseline:
Benchmark Mode Cnt Score Error Units
DataInputStreamTest.readChar avgt 20 22,889 ± 0,648 us/op
DataInputStreamTest.readInt avgt 20 21,804 ± 0,197 us/op
patch:
Benchmark Mode Cnt Score Error Units
DataInputStreamTest.readChar avgt 20 11,018 ± 0,089 us/op
DataInputStreamTest.readInt avgt 20 5,608 ± 0,087 us/op
Progress
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8292698: Improve performance of DataInputStream
Reviewers
- Daniel Fuchs (@dfuch - Reviewer)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9956/head:pull/9956
$ git checkout pull/9956
Update a local copy of the PR:
$ git checkout pull/9956
$ git pull https://git.openjdk.org/jdk pull/9956/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9956
View PR using the GUI difftool:
$ git pr show -t 9956
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9956.diff
:wave: Welcome back stsypanov! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@stsypanov 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.
Webrevs
- 03: Full - Incremental (3ea66641)
- 02: Full - Incremental (1535e925)
- 01: Full - Incremental (a2378ded)
- 00: Full (31a9c451)
'in' is a protected field so it requires thinking about subclasses that might change 'in', say when the input stream is asynchronously closed. BufferedInputStream is an example of a FilterInputStream that sets 'in' to null when asynchronously closed. There are other examples that change the underlying input stream to one that returns EOF when closed. So it might be that changing readChar/readInt/readLong is okay but it would have a bigger effect on readFully, skip and the other methods. So I think this is a case of proceeding with caution as there may be more gong on.
'in' is a protected field so it requires thinking about subclasses that might change 'in', say when the input stream is asynchronously closed. BufferedInputStream is an example of a FilterInputStream that sets 'in' to null when asynchronously closed. There are other examples that change the underlying input stream to one that returns EOF when closed. So it might be that changing readChar/readInt/readLong is okay but it would have a bigger effect on readFully, skip and the other methods. So I think this is a case of proceeding with caution as there may be more gong on.
On the other hand I don't see anything checking for in == null, so if in was null a NullPointerException exception would occur, which I doubt is the behavior that the subclass is expecting. I agree that caution is warranted, but if in is actually set to null by subclasses we should probably double check how/when they rely in their super class behavior: some bugs might be lurking.
I agree that caution is warranted
But assuming I've reverted the changes that are dubious, how could there be bugs caused by replacement of multiple reads with a single one? I was sure that such replacement improves thread safety as soon as we rid racy reads.
I agree that caution is warranted
But assuming I've reverted the changes that are dubious, how could there be bugs caused by replacement of multiple reads with a single one? I was sure that such replacement improves thread safety as soon as we rid racy reads.
Yes - that's what I was trying to say. I believe reading the volatile only once per method is much better for consistency. And I'm not sure I see why the other changes were dubious since these methods weren't checking for null either?
My point is that if some method in the subclass can set the value to null asynchronously when some other method in the super class is running then that's a NPE time-bomb (and IMO there's a bug lurking if that can happen).
But assuming I've reverted the changes that are dubious, how could there be bugs caused by replacement of multiple reads with a single one? I was sure that such replacement improves thread safety as soon as we rid racy reads.
The main thing is to think through the implications for async close where the close method replaces 'in'. There are a few examples in the JDK that extend FilterInputStream that do this. It was hard to reason about this scenario in the first iteration.
The main thing is to think through the implications for async close where the close method replaces 'in'.
Suppose we have a scenario where in is replaced asynchronously in one of implementations and the implementation is passed into constructor of DataInputStream. In this case the patched code is less NPE-vulnerable, isn't it?
Suppose we have a scenario where
inis replaced asynchronously in one of implementations and the implementation is passed into constructor ofDataInputStream. In this case the patched code is less NPE-vulnerable, isn't it?
I can't imagine a subclass of DataInputStream setting 'in' to null. If it handles async close then it's possible that it replaces it with an input stream where read throws IOException unconditionally. This is why the original patch was problematic for the methods that call in.read in a loop. The updated patch is probably okay, I'm just pointing out that we are forced to think of subclasses when touching this code.
I can't imagine a subclass of DataInputStream setting 'in' to null. If it handles async close then it's possible that it replaces it with an input stream where read throws IOException unconditionally. This is why the original patch was problematic for the methods that call in.read in a loop. The updated patch is probably okay, I'm just pointing out that we are forced to think of subclasses when touching this code.
Thanks Alan - that makes sense to me.
Btw, I've tried to reimplement readInt() in a way similar to readLong():
public final int readInt() throws IOException {
readFully(readBuffer, 0, 4);
return (readBuffer[0] & 0xff) << 24
+ (readBuffer[1] & 0xff) << 16
+ (readBuffer[2] & 0xff) << 8
+ (readBuffer[3] & 0xff);
}
but with this change the build fails with
Building target 'test' in configuration 'macosx-x86_64-server-release'
Compiling 3158 files for java.base
Updating support/src.zip
Optimizing the exploded image
Error occurred during initialization of boot layer
java.lang.module.FindException: Error reading module: /Users/stsypanov/IdeaProjects/jdk/build/macosx-x86_64-server-release/jdk/modules/java.management.rmi
Caused by: java.lang.module.InvalidModuleDescriptorException: Bad magic number
make[3]: *** [/Users/stsypanov/IdeaProjects/jdk/build/macosx-x86_64-server-release/jdk/_optimize_image_exec.marker] Error 1
make[2]: *** [exploded-image-optimize] Error 2
ERROR: Build failed for target 'test' in configuration 'macosx-x86_64-server-release' (exit code 2)
Stopping sjavac server
=== Output from failing command(s) repeated here ===
* For target jdk__optimize_image_exec:
Error occurred during initialization of boot layer
java.lang.module.FindException: Error reading module: /Users/stsypanov/IdeaProjects/jdk/build/macosx-x86_64-server-release/jdk/modules/java.management.rmi
Caused by: java.lang.module.InvalidModuleDescriptorException: Bad magic number
* All command lines available in /Users/stsypanov/IdeaProjects/jdk/build/macosx-x86_64-server-release/make-support/failure-logs.
=== End of repeated output ===
What is wrong with the change?
What is wrong with the change?
You'll need parentheses to make that work, changing it to use '|' would work too.
:warning: @stsypanov the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:
$ git checkout 8292698
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push
@stsypanov 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:
8292698: Improve performance of DataInputStream
Reviewed-by: dfuchs
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 580 new commits pushed to the master branch:
- 7e4868de7b0d3c20a35f06671ec0b68cfd441793: 8294847: Fix calculation of G1 effective scanned cards prediction
- 2f60675e06801b8ee495729d8bff2faec37ce509: 8294997: Improve ECC math operations
- 94caecbe574227b232e22d9f56361f8ecd507be6: 8294906: Memory leak in PKCS11 NSS TLS server
- 03e63a2b87e1bef6025722ec9a016312c55ebd81: 8295225: [JVMCI] codeStart should be cleared when entryPoint is cleared
- 26ac8366360685ef0cf3447ee7db16ba7a7fa1ec: 8291638: Keep-Alive timeout of 0 should close connection immediately
- 6ae7e4d4aad5712cf2fe6ab9f98dc424fa4170cb: 8293984: Unnecessary Vector usage in PropertyEditorSupport
- cd1357b0af0d4e3b459fcf88e67510502464bb90: 8295229: Try to verify gtest version
- 90fb9a085bbaa9d1928a1cec9f00098b80577721: 8295102: Always load @lambda-form-invoker lines from default classlist
- ac1941425bdb1a25aa3364eef9eb1092ee716761: 8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance
- 1961e81e02e707cd0c8241aa3af6ddabf7668589: 8289509: Improve test coverage for XPath Axes: descendant, descendant-or-self, following, following-sibling
- ... and 570 more: https://git.openjdk.org/jdk/compare/f91943c19fc0b060684a437d2c768461d54c088e...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.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@stsypanov Your change (at version 3ea66641a55894b087df6e3f5e4acf738086313b) is now ready to be sponsored by a Committer.
What is wrong with the change?
You'll need parentheses to make that work, changing it to use '|' would work too.
Does that mean there is no explicit test for DataInputStream endianess (if only accidential tests checking for magic numbers failed.. or where there other failures but you did not mention them).
btw the cleanup looks really good. (shame we have no easy way to stack allocate the scratch byte array)
Does that mean there is no explicit test for DataInputStream endianess
@ecki I think this is not strictly necessary as order of bytes is explicitly specified in JavaDoc of DataInput implemented by DataInputStream
Does that mean there is no explicit test for DataInputStream endianess
@ecki I think this is not strictly necessary as order of bytes is explicitly specified in JavaDoc of
DataInputimplemented byDataInputStream
Hm, maybe I was not clear, what I mean the endianess of DataInputStream is specified by the spec, so I would expect it to be tested. But it sounded like an accidential discovery that the impl was broken. Maybe that’s part of the tck tests and they not yet have been run?
The tests for endianess for DataInputStream are in the TCK and would have caught this case. Tests for endianess are not duplicated in the jtreg test suite.
Btw, the changes is quite simple, so could it be backported to JDK11 and JDK17 updates?
Anyone to sponsor, or should I get more approves?
I see that both Alan and Daniel have approved these changes and there hasn't been a commit in this PR, after the approval (which is a good thing). I'll run some internal tests and if all goes well will go ahead and sponsor this change.
The tier1, tier2, tier3 and the relevant JCK tests completed without any failures.