Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into from

Conversation

mkarg
Copy link
Contributor

@mkarg mkarg commented Aug 19, 2021

As proposed in JDK-8265891, this PR provides an implementation for Channels.newInputStream().transferTo() which provide superior performance compared to the current implementation. This PR is a spin-off from #4263 and deliberately concentrates only on FileChannels. Other types of channels will be discussed in future PRs.

  • 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 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. A rather similar approach in different use casse was recently implemented by #5097 which was reported by even provide 5x performance (#5097 (comment)).

Update: This sub-issue only defines the work to be done to implement JDK-8265891 solely for the particular case of FileChannel.transferTo(WriteableByteChannel), including special treatment of SelectableByteChannel, as the master branch already contains code to handle the specific case of FileChannel.transferTo(FileChannel).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8276779: (ch) InputStream returned by Channels.newInputStream should have fast path for SelectableChannels
  • JDK-8276968: (ch) InputStream returned by Channels.newInputStream should have fast path for SelectableChannels (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5179

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 19, 2021

👋 Welcome back mkarg! 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 openjdk bot added the rfr label Aug 19, 2021
@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Aug 19, 2021

@AlanBateman @bplb This PR is a spin-off for FileChannels only, as suggested by you. I kindly request your review. Thanks.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 19, 2021

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

  • nio

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.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Aug 19, 2021

Thanks for the update. I see you've kept the code for the file -> socket scenario but there aren't any tests for this. If you do want to include this scenario then we'll need at least a test for the case where the output stream wraps a socket channel that is configured blocking. The test/jdk/java/nio/channels/Channels directory would be a good location for the new test because it is testing the input stream returned by the Channels API.

As regards ChannelInputStream then I had hoped that ThrowingLongSupplier would go away. If you keep your 2-arg transfer method then we should be able to implementation transferTo very simply, something like

        public long transferTo(OutputStream out) throws IOException {
            if (ch instanceof FileChannel fc
                    && out instanceof ChannelOutputStream cos) {
                WritableByteChannel target = cos.channel();
                if (target instanceof SelectableChannel sc) {
                    synchronized (sc.blockingLock()) {
                        if (!sc.isBlocking())
                            throw new IllegalBlockingModeException();
                        return transfer(fc, target);
                    }
                }
                return transfer(fc, target);
            }
            return super.transferTo(out);
        }

@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Aug 19, 2021

Thanks for the update. I see you've kept the code for the file -> socket scenario but there aren't any tests for this.

Seems I misunderstood your last week's request of separating the PRs! I simply split up the features into several PRs. So you actually wanted to start with just the FC->FC use case only (which, actually, was not an explicit execution path of the original PR)? I will further narrow this PR then, no problem, just tell me!

And do I understand correctly, you do not just want to have some test for each PR that just proofs the API is correctly implemented (= Blackbox), but you actually want to have one test for each existing implementation of WriteableByteChannel (= Whitebox)? I mean, as it is an interface which can be implemented by infinite classes by anybody, such a request is inherently unresolvable. Or do you just want to have tests for specific implementations, then, besides SocketChannel, which ones do you want to have tested? To be more precise, my problem is that I do not understand why you want me to test explicitly SocketChannels, as (a) my code not even contains the word "Socket", (b) anybody could come up with a new implementation of "WritableByteChannel" that needs special treatment or testing in future, and (c) the original test of the original InputStream::transferTo() did not test SocketChannels at all.

@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Aug 19, 2021

I had hoped that ThrowingLongSupplier would go away. If you keep your 2-arg transfer method then we should be able to implementation transferTo very simply, something like

        public long transferTo(OutputStream out) throws IOException {
            if (ch instanceof FileChannel fc
                    && out instanceof ChannelOutputStream cos) {
                WritableByteChannel target = cos.channel();
                if (target instanceof SelectableChannel sc) {
                    synchronized (sc.blockingLock()) {
                        if (!sc.isBlocking())
                            throw new IllegalBlockingModeException();
                        return transfer(fc, target);
                    }
                }
                return transfer(fc, target);
            }
            return super.transferTo(out);
        }

This is not true. It will only check the blocking mode for the target. But technically it is possible that someone writes an implementation of FileChannel which implements SelectableChannel. In that case it is needed to check the blocking mode on the source, too. So either we (a) ignore this possibility, (b) write a very complex code which calls transfer(fc, target) in FOUR code paths not just two, or (c) we keep the ThrowingLongSupplier as an elegant way to perform the 2x2 matrix.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Aug 19, 2021

This is not true. It will only check the blocking mode for the target. But technically it is possible that someone writes an implementation of FileChannel which implements SelectableChannel. In that case it is needed to check the blocking mode on the source, too. So either we (a) ignore this possibility, (b) write a very complex code which calls transfer(fc, target) in FOUR code paths not just two, or (c) we keep the ThrowingLongSupplier as an elegant way to perform the 2x2 matrix.

SelectableChannel is an abstract class, not an interface so you can't extend both FileChannel and SelectableChannel. So when the source channel is a FileChannel then it should only be concerned with whether the target channel is a SelectableChannel or not. I think we can keep it very simple, along the lines of the method I pasted in above.

If you choose to include the case where the target is a SelectableChannel (and I have no objection to doing that) then the test needs to exercise that code. This means the test needs an input stream created on a source file/FileChannel, and an output stream created an channel that is WritableByteChannel & SelectableChannel. A Pipe.SinkChannel or a connected SocketChannel will do fine. If you choose to leave out selectable channels for the first PR then file -> file is okay, which I think is what your current test is doing.

@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Aug 20, 2021

SelectableChannel is an abstract class, not an interface so you can't extend both FileChannel and SelectableChannel. So when the source channel is a FileChannel then it should only be concerned with whether the target channel is a SelectableChannel or not. I think we can keep it very simple, along the lines of the method I pasted in above.

Silly me, you are right, it is an abstract class! Totally missed this!

If you choose to include the case where the target is a SelectableChannel (and I have no objection to doing that) then the test needs to exercise that code. This means the test needs an input stream created on a source file/FileChannel, and an output stream created an channel that is WritableByteChannel & SelectableChannel. A Pipe.SinkChannel or a connected SocketChannel will do fine. If you choose to leave out selectable channels for the first PR then file -> file is okay, which I think is what your current test is doing.

Thanks a lot for the detailed explanation! I now better understood your concerns and will change the PR accordingly!

@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Aug 21, 2021

@AlanBateman I have pushed another PR #5209 that handles only the case FileChannel->FileChannel, so we will have some progress there and once that one is merged I will come back to the FileChannel->SelectableChannel case later.

@mkarg mkarg force-pushed the JDK-8265891-FileChannel branch from 84a7b06 to f03ca97 Compare Aug 21, 2021
@mkarg mkarg force-pushed the JDK-8265891-FileChannel branch 2 times, most recently from 1e4c9de to 56f1693 Compare Aug 30, 2021
@openjdk openjdk bot removed the rfr label Aug 30, 2021
@mkarg mkarg force-pushed the JDK-8265891-FileChannel branch from 56f1693 to 041e02f Compare Aug 30, 2021
@openjdk openjdk bot added the rfr label Aug 30, 2021
@mkarg mkarg force-pushed the JDK-8265891-FileChannel branch from 041e02f to 4238790 Compare Aug 30, 2021
@mkarg mkarg marked this pull request as draft Aug 31, 2021
@openjdk openjdk bot removed the rfr label Aug 31, 2021
@mkarg mkarg force-pushed the JDK-8265891-FileChannel branch 2 times, most recently from 3887556 to a507fd7 Compare Aug 31, 2021
@mkarg mkarg force-pushed the JDK-8265891-FileChannel branch from a507fd7 to e76caa4 Compare Sep 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2021

@mkarg this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8265891-FileChannel
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict label Sep 8, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 6, 2021

@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
Copy link
Contributor Author

@mkarg mkarg commented Oct 8, 2021

Please keep this draft open, it will be continued after other PRs are merged.

@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Nov 27, 2021

The current version of this test is missing the previous updates that were pushed to address the 2GB files being left after the test run. Please update your PR so that reviews are based off of what is in the workspace.

I just rebased this PR on master so now it does contain Lance`s 2GB-temp-file-removal.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 28, 2021

Thanks for the update and re-basing. I don't have any more comments, let's give Lance a day or two to see if he has any more comments on the temporary file location/naming that he suggested.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

We are getting closer, the methods fileChannelInput/Output should also create the temp files in the working directory and include a prefix for name construction.

@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Dec 1, 2021

We are getting closer, the methods fileChannelInput/Output should also create the temp files in the working directory and include a prefix for name construction.

This is exactly what #6379 is good for. Please review that PR first. Once it got merged, I will rebase this PR ontop.

@LanceAndersen
Copy link
Contributor

@LanceAndersen LanceAndersen commented Dec 1, 2021

JDK-8276994 was resolved previously so I am not sure why you still have a PR open for that issue as you would not be able to integrate without a new bug.

Please integrate any remaining changes into the test and we can finalize the review for you

@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Dec 2, 2021

JDK-8276994 was resolved previously so I am not sure why you still have a PR open for that issue as you would not be able to integrate without a new bug.

Understood. I did not find a public rule that says that there cannot be multiple PRs referencing the same JIRA issue.

Please integrate any remaining changes into the test and we can finalize the review for you

Ok so I will merge both PRs into one.

Signed-off-by: Markus Karg <markus@headcrashing.eu>
@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Dec 2, 2021

@LanceAndersen This PR now contains the remaining changes and I have closed #6379 in turn.

test/jdk/java/nio/channels/Channels/TransferTo.java Outdated Show resolved Hide resolved
@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2021

⚠️ @mkarg 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 JDK-8265891-FileChannel
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2021

@mkarg 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:

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

Reviewed-by: lancea, alanb

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 122 new commits pushed to the master branch:

  • 02ee337: 8278175: Enable all doclint warnings for build of java.desktop
  • 24e16ac: 8277617: Adjust AVX3Threshold for copy/fill stubs
  • 2b87c2b: 8277793: Support vector F2I and D2L cast operations for X86
  • e1cde19: 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException
  • a729a70: 8225181: KeyStore should have a getAttributes method
  • 38f525e: 8275821: Optimize random number generators developed in JDK-8248862 using Math.unsignedMultiplyHigh()
  • 780b8b1: 8278179: Enable all doclint warnings for build of java.naming
  • 678ac58: 8278240: ProblemList containers/docker/TestJcmd.java on linux-aarch64
  • 01cb2b9: 8277529: SIGSEGV in C2 CompilerThread Node::rematerialize() compiling Packet::readUnsignedTrint
  • 660f21a: 8278119: ProblemList few headful test failing in macosx12-aarch64 system
  • ... and 112 more: https://git.openjdk.java.net/jdk/compare/e9b36a83160d3c1fa79841692e9fadf336bf7954...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 (@LanceAndersen, @AlanBateman) 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).

@openjdk openjdk bot added the ready label Dec 3, 2021
Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

The changes look OK. I will run the PR through Mach5 tier1 - tier3 prior to approving as a sanity check.

Signed-off-by: Markus Karg <markus@headcrashing.eu>
@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Dec 3, 2021

The changes look OK. I will run the PR through Mach5 tier1 - tier3 prior to approving as a sanity check.

Ok thank you, so I wait with the integration for your test result.

@LanceAndersen
Copy link
Contributor

@LanceAndersen LanceAndersen commented Dec 3, 2021

The changes look OK. I will run the PR through Mach5 tier1 - tier3 prior to approving as a sanity check.

Ok thank you, so I wait with the integration for your test result.

Mach5 tiers1 - 3 ran fine

@mkarg
Copy link
Contributor Author

@mkarg mkarg commented Dec 4, 2021

/integrate

@openjdk openjdk bot added the sponsor label Dec 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 4, 2021

@mkarg
Your change (at version 39c36cb) is now ready to be sponsored by a Committer.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Dec 4, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Dec 4, 2021

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

  • 02ee337: 8278175: Enable all doclint warnings for build of java.desktop
  • 24e16ac: 8277617: Adjust AVX3Threshold for copy/fill stubs
  • 2b87c2b: 8277793: Support vector F2I and D2L cast operations for X86
  • e1cde19: 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException
  • a729a70: 8225181: KeyStore should have a getAttributes method
  • 38f525e: 8275821: Optimize random number generators developed in JDK-8248862 using Math.unsignedMultiplyHigh()
  • 780b8b1: 8278179: Enable all doclint warnings for build of java.naming
  • 678ac58: 8278240: ProblemList containers/docker/TestJcmd.java on linux-aarch64
  • 01cb2b9: 8277529: SIGSEGV in C2 CompilerThread Node::rematerialize() compiling Packet::readUnsignedTrint
  • 660f21a: 8278119: ProblemList few headful test failing in macosx12-aarch64 system
  • ... and 112 more: https://git.openjdk.java.net/jdk/compare/e9b36a83160d3c1fa79841692e9fadf336bf7954...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 4, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Dec 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 4, 2021

@AlanBateman @mkarg Pushed as commit 9642629.

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

@mkarg mkarg deleted the JDK-8265891-FileChannel branch Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated nio
4 participants