8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo #4263

mkarg · 2021-05-30T17:30:56Z

This PR-draft is work in progress and an invitation to discuss a possible solution for issue JDK-8265891. It is not yet intended for a final review.

As proposed in JDK-8265891, this PR provides an implementation for Channels.newInputStream().transferTo() which provide superior performance compared to the current implementation. The changes are:

Prevents transfers through the JVM heap as much as possibly by offloading to deeper levels via NIO, hence allowing the operating system to optimize the transfer.
Using more JRE heap in the fallback case when no NIO is possible (still only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern hardware / fast I/O devides.

Using JMH I have benchmarked both, the original implementation and this implementation, and (depending on the used hardware and use case) performance change was approx. doubled performance. So this PoC proofs that it makes sense to finalize this work and turn it into an actual OpenJDK contribution.

I encourage everybody to discuss this draft:

Are there valid arguments for not doing this change?
Is there a better way to improve performance of Channels.newInputStream().transferTo()?
How to go on from here: What is missing to get this ready for an actual review?

Progress

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

Issue

JDK-8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4263

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

Using diff file

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

Preventing transfer through the JVM heap by using NIO and potentially performing intra-OS transport to improve performance where possible. Signed-off-by: Markus Karg <markus@headcrashing.eu>

These days I/O devides, particularly SSDs, are much faster than in the days this original code was written. Due to that, the transfer time is less I/O bound. Performance can be increased significantly by spending 16x 8 KiB instead of the original 1x 8 KiB heap memory (approx. doubles performance, clearly depending on the used hardware). Signed-off-by: Markus Karg <markus@headcrashing.eu>

bridgekeeper · 2021-05-30T17:31:42Z

Hi @mkarg, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user mkarg" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2021-05-30T17:33:50Z

@mkarg The following labels will be automatically applied to this pull request:

core-libs
nio

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

mkarg · 2021-05-30T17:49:29Z

/signed

bridgekeeper · 2021-05-30T17:49:50Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

src/java.base/share/classes/java/nio/channels/Channels.java

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java

Signed-off-by: Markus Karg <markus@headcrashing.eu>

mkarg · 2021-06-05T15:11:55Z

@AlanBateman I'm done with the changes you requested and kindly like to ask where to go from here.

AlanBateman · 2021-06-08T11:49:55Z

@AlanBateman I'm done with the changes you requested and kindly like to ask where to go from here.

Moving ChannelOutputStream to sun.nio.ch looks right. The implementation of transferTo will need a few rounds of cleanup, it's a look very messy right now. You are over using var and is impossible for the reader to know what the types are. Pattern matching for instanceof can be used to avoid the casts.

mkarg · 2021-06-08T17:18:24Z

Great ideas, will do as you say, stay tuned! Didn't know that using var is a no-go, sorry for that. Pattern matching is definitively a great idea!

Signed-off-by: Markus Karg <markus@headcrashing.eu>

mkarg · 2021-06-08T18:09:28Z

Replaced var by actual type in b3c62b0.

Signed-off-by: Markus Karg <markus@headcrashing.eu>

mkarg · 2021-06-08T18:17:55Z

Replaced casting by pattern matching for instanceof in ac62cdb.

mkarg · 2021-06-08T18:18:14Z

@AlanBateman Pushed the requested changes. More change requests? :-)

src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java

Signed-off-by: Markus Karg <markus@headcrashing.eu>

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java

Signed-off-by: Markus Karg <markus@headcrashing.eu>

mkarg · 2021-08-15T13:37:23Z

Does it make any real sense to answer your recent questions, provide the proofs, tests and benchmark results (I actually would love to if it makes sense) or will the outcome be that I must drop everything besides file channels anyways (In that case it is in vain)? As my time is just as precious as yours I really need to know that before I spend more weeks into code paths that you possibly decided to never accept ever. Don't get me wrong, if you see a chance to keep the code once I provided the answers I will do that, but if you do not see that chance, please frankly and unambiguously tell me now. Thanks.

I think the best course of action is to reduce the scope of this PR to the file channel cases. There is no reason why future PRs can't build on this and add implementations for other channel types.

Agreed, I will split this PR into several PRs, so we can start with the low-hanging fruits first and later dive into the more complex cases.

mkarg · 2021-08-19T06:21:06Z

I think the best course of action is to reduce the scope of this PR to the file channel cases. There is no reason why future PRs can't build on this and add implementations for other channel types.

I have split up this PR so that only the lowest hanging fruit is covered and kindly request review: #5179.

bridgekeeper · 2021-09-26T12:51:50Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2021-10-10T08:55:10Z

Work on this draft will be continued as soon as depenency PRs are resolved. Please keep this issue open.

bridgekeeper · 2021-11-07T12:50:52Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2021-11-07T16:51:50Z

Please keep this PR open. More commits will follow on the next weeks.

bridgekeeper · 2021-12-05T18:33:46Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2021-12-06T07:07:03Z

Please keep this PR open. More commits will follow on the next weeks.

bourgesl · 2021-12-06T08:42:30Z

Looks promising, keep moving, @mkarg !

mkarg · 2021-12-12T14:43:23Z

Looks promising, keep moving, @mkarg !

Thanks a lot, Laurent! I really appreciate! In fact, most of the work is going on in spin-off PRs, and some of it already is merged, as I explained on Twitter and Youtube. :-)

bridgekeeper · 2022-01-14T17:40:12Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-01-15T09:33:01Z

Please keep this PR open as I am working on several sub-issues currently.

bridgekeeper · 2022-02-12T14:35:28Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-02-13T07:54:31Z

Please keep this PR open. I am still working on it.

bridgekeeper · 2022-03-13T09:06:49Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-03-13T09:44:57Z

Please keep this PR open; I will continune work on it soon.

bridgekeeper · 2022-05-08T18:02:55Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-05-13T17:38:31Z

Please keep this PR open; I will continune work on it soon.

bridgekeeper · 2022-07-08T19:38:57Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-07-08T19:40:18Z

Please keep this PR open; I will continune work on it soon.

bridgekeeper · 2022-09-03T05:49:54Z

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2022-10-29T06:29:01Z

@mkarg This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

laeubi · 2022-10-30T11:36:23Z

Hi @laeubi, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user laeubi for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

mkarg · 2022-10-31T08:44:18Z

@laeubi The performance was optimized by several PRs (works well for file based streams now), but there are still cases that could (possibly should) get optimized. These cases are still on my agenda. But before I continue with those cases, I focus on enabling that optimization (BufferedInputStream prevents the benefit in most cases, but #10525 will fix that, so I can step on soon).

laeubi · 2022-10-31T09:38:50Z

Hi @laeubi, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user laeubi for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

mkarg added 2 commits May 30, 2021 17:46

Draft: Offloading transfer to deeper layers

54c5764

Preventing transfer through the JVM heap by using NIO and potentially performing intra-OS transport to improve performance where possible. Signed-off-by: Markus Karg <markus@headcrashing.eu>

bridgekeeper bot added the oca Needs verification of OCA signatory status label May 30, 2021

openjdk bot added nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels May 30, 2021

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label May 30, 2021

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Jun 1, 2021

AlanBateman reviewed Jun 2, 2021

View reviewed changes

src/java.base/share/classes/java/nio/channels/Channels.java Outdated Show resolved Hide resolved

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java Outdated Show resolved Hide resolved

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java Outdated Show resolved Hide resolved

mkarg added 2 commits June 4, 2021 19:03

Draft: Using Thread-Local Buffer Cache

8f58986

Signed-off-by: Markus Karg <markus@headcrashing.eu>

Draft: Removing final everywhere but kept only for constants

df1a57a

Signed-off-by: Markus Karg <markus@headcrashing.eu>

mkarg force-pushed the JDK-8265891 branch from 8dfe997 to 885eab9 Compare June 5, 2021 14:24

Draft: Hiding ChannelOutputStream from the API

b6dc6a7

Signed-off-by: Markus Karg <markus@headcrashing.eu>

mkarg force-pushed the JDK-8265891 branch from 885eab9 to b6dc6a7 Compare June 5, 2021 14:29

Draft: Replacing var by actual type

b3c62b0

Signed-off-by: Markus Karg <markus@headcrashing.eu>

Draft: Replaced casting by pattern matching for instanceof

ac62cdb

Signed-off-by: Markus Karg <markus@headcrashing.eu>

bplb reviewed Jun 8, 2021

View reviewed changes

src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java Outdated Show resolved Hide resolved

Draft: Replaced custom bounds check by Objects.checkFromIndexSize()

599181b

Signed-off-by: Markus Karg <markus@headcrashing.eu>

AlanBateman reviewed Jun 9, 2021

View reviewed changes

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java Outdated Show resolved Hide resolved

Draft: Moving I/O ops into the for statement block

562b102

Signed-off-by: Markus Karg <markus@headcrashing.eu>

mkarg mentioned this pull request Aug 19, 2021

8276779: (ch) InputStream returned by Channels.newInputStream should have fast path for SelectableChannels #5179

Closed

4 tasks

mkarg mentioned this pull request Aug 21, 2021

JDK-8273038: ChannelInputStream.transferTo() uses FileChannel.transferTo(FileChannel) #5209

Closed

3 tasks

mkarg marked this pull request as draft August 31, 2021 06:30

openjdk bot removed the rfr Pull request is ready for review label Aug 31, 2021

bridgekeeper bot closed this Oct 29, 2022

8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo #4263

8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo #4263

Uh oh!

Conversation

mkarg commented May 30, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented May 30, 2021

Uh oh!

openjdk bot commented May 30, 2021

Uh oh!

mkarg commented May 30, 2021

Uh oh!

bridgekeeper bot commented May 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkarg commented Jun 5, 2021

Uh oh!

AlanBateman commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkarg commented Jun 8, 2021

Uh oh!

mkarg commented Jun 8, 2021

Uh oh!

mkarg commented Jun 8, 2021

Uh oh!

mkarg commented Jun 8, 2021

Uh oh!

Uh oh!

Uh oh!

mkarg commented Aug 15, 2021

Uh oh!

mkarg commented Aug 19, 2021

Uh oh!

bridgekeeper bot commented Sep 26, 2021

Uh oh!

mkarg commented Oct 10, 2021

Uh oh!

bridgekeeper bot commented Nov 7, 2021

Uh oh!

mkarg commented Nov 7, 2021

Uh oh!

bridgekeeper bot commented Dec 5, 2021

Uh oh!

mkarg commented Dec 6, 2021

Uh oh!

bourgesl commented Dec 6, 2021

Uh oh!

mkarg commented Dec 12, 2021

Uh oh!

bridgekeeper bot commented Jan 14, 2022

Uh oh!

mkarg commented Jan 15, 2022

Uh oh!

bridgekeeper bot commented Feb 12, 2022

Uh oh!

mkarg commented Feb 13, 2022

Uh oh!

bridgekeeper bot commented Mar 13, 2022

Uh oh!

mkarg commented Mar 13, 2022

Uh oh!

bridgekeeper bot commented May 8, 2022

Uh oh!

mkarg commented May 13, 2022

Uh oh!

bridgekeeper bot commented Jul 8, 2022

Uh oh!

mkarg commented Jul 8, 2022

Uh oh!

bridgekeeper bot commented Sep 3, 2022

Uh oh!

bridgekeeper bot commented Oct 29, 2022

Uh oh!

mkarg commented May 30, 2021 •

edited by openjdk bot

Loading

AlanBateman commented Jun 8, 2021 •

edited

Loading

laeubi commented Oct 30, 2022 •

edited by bridgekeeper bot

Loading

laeubi commented Oct 31, 2022 •

edited by bridgekeeper bot

Loading