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

[mkarg]

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 #5179 and deliberately concentrates only on the case where both, source and target, are actually 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)).

---

Progress

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

Issue

• JDK-8273038: ChannelInputStream.transferTo() uses FileChannel.transferTo(FileChannel)

Reviewers

• Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5209

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

Using diff file

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

[bridgekeeper]

👋 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]

@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.

[mkarg]

@AlanBateman @bplb This PR is a spin-off for the single case of FileChannel->FileChannel only, as suggested by you. I kindly request your review. Thanks.

[mlbridge]

Webrevs

• 13: Full - Incremental (1f5a4567)
• 12: Full - Incremental (1804aa00)
• 11: Full - Incremental (cb2a8636)
• 10: Full - Incremental (90a176f3)
• 09: Full - Incremental (6b4c34c7)
• 08: Full - Incremental (852032d9)
• 07: Full - Incremental (98dc61e5)
• 06: Full - Incremental (b80bf8ea)
• 05: Full - Incremental (a043ae25)
• 04: Full - Incremental (e9163bf4)
• 03: Full - Incremental (205b9b21)
• 02: Full - Incremental (33bef3d6)
• 01: Full - Incremental (f35cd7d7)
• 00: Full (bed6daf8)

[pirateskipper]

Throwable thrown = null; try { thrower.run();

[mkarg]

Is that an OpenJDK best practice or why do you ask for this change? The code actually is a copy of existing OpenJDK code and not originally authored by me.

[AlanBateman]

There are 3 PRs open, I'll ignore #4263 and #5179 for now.

The moving of the OutputStream implementation from Channels to sun.nio.ch.ChannelOutputStream looks fine. In the class description it should be "accessible" as it visible. You can drop "but not be part of the java.base module API" too.

Changing the bounds check to use Objects.checkFromIndexSize looks fine.

It would be good if you could re-format 1-arg transferTo to something like this:

        if (out instanceof ChannelOutputStream cos
                && ch instanceof FileChannel fc 
                && cos.channel() instanceof FileChannel dst) {
            
        }

just to to avoid the really long line as that gets annoying when looking at side-by-side diffs.

The 2-arg transferTo methods look okay. An alternative, that avoids the need for "srcPos + bytesWritten" in 3 places is to just something like:

        long initialPos = src.position();
        long pos = initialPos;
        try {
            while (pos < src.size()) {
               pos += src.transferTo(pos, Long.MAX_VALUE, dst);
            }
        } finally {
            src.position(pos);
        }
        return pos - initialPos;

@bplb You looked at the test in the first PR, have you look at this one? I suspect it would be a lot cleaner/maintainable if it were written as a TestNG test. I assume the transfer loop never has more than one iteration because the file sizes are <2GB. If it were writing this test then I would probably use Files/Arrays.mismatch to compare the contents.

[mkarg]

There are 3 PRs open, I'll ignore #4263 and #5179 for now.

Yes please ignore them for now. The other two PRs are suspendend as they cover bigger pictures and will be resumed by me later.

The moving of the OutputStream implementation from Channels to sun.nio.ch.ChannelOutputStream looks fine. In the class description it should be "accessible" as it visible. You can drop "but not be part of the java.base module API" too.

Done in f35cd7d.

It would be good if you could re-format 1-arg transferTo to something like this:
...
just to to avoid the really long line as that gets annoying when looking at side-by-side diffs.

Never had this problem as I'm using inline-diff tools only, but done in 33bef3d.

An alternative, that avoids the need for "srcPos + bytesWritten" in 3 places is to just something like:
...

Does that mean that I must change it? As the method is mostly I/O bound I do not see what we actually would gain. Anyways, done in 205b9b2.

I suspect it would be a lot cleaner/maintainable if it were written as a TestNG test. I assume the transfer loop never has more than one iteration because the file sizes are <2GB. If it were writing this test then I would probably use Files/Arrays.mismatch to compare the contents.

Understood, but please note that this code actually was simply copied from the original tests that Brian provided as a blue-print, which neither uses TestNG nor Files/Arrays.mismatch nor tests larger files. So if you ask me to change that, in turn someone should change the original tests then to keep the code in sync. So can we keep the tests as-is or am I forced to use TestNG and File/Arrays.mismatch?

[bplb]

@AlanBateman It does not look like this test is substantively different than the one in the first PR aside from constraining the cases tested. One thing I think is insufficient is only dealing with streams at position zero. In the test I recently added, java/io/FileInputStream/TransferTo.java, I included random initial positions for both input and output. For random positions it is better to use jdk.test.lib.RandomFactory.getRandom() instead of new Random() as this will print the seed value which can be useful in reproducing any problems. (For data content alone, new Random() is fine because we don't care what those bytes are.)

[AlanBateman]

There are 3 PRs open, I'll ignore #4263 and #5179 for now.

Yes please ignore them for now. The other two PRs are suspendend as they cover bigger pictures and will be resumed by me later.

Okay. The implementation changes in the current patch (205b9b2) look good. I think we will need to iterate on the test.

[mkarg]

There are 3 PRs open, I'll ignore #4263 and #5179 for now.

Yes please ignore them for now. The other two PRs are suspendend as they cover bigger pictures and will be resumed by me later.

Okay. The implementation changes in the current patch (205b9b2) look good. I think we will need to iterate on the test.

Ok so I will do as Brian said and include a test case with random initial positions. Note that my test will do that with the default case, too, so maybe someone should do that for the original test of the default implementation, too, to stay in sync.

[bplb]

Why not just use java/io/FileInputStream/TransferTo.java as a starting point?

[mkarg]

Why not just use java/io/FileInputStream/TransferTo.java as a starting point?

Because I already invested considerable time into my test already and in fact do like my test more due to the flexibility it provides for my future PRs that will follow soon (adding more transfer cases), and very much like it as it is, so if only a small change is actually needed here then I will add that change and we're done with this PR and I will go on to re-enable other test cases.

Please don't get me wrong, but while I highly appreciate everybody's comments on actual risks or problems that I have to resolve before my PR can be merged, I really like to concentrate on actual risks and problems of my PR instead of proposing to throw it away effectively. Thanks.

[mkarg]

@bplb @AlanBateman As discussed before, I have changed the test in this PR to cover the following cases:

• Testing random buffer sizes
• Testing random initial source position
• Testing random initial target position

I deliberately did not test the following:

• 2GB+: IMHO makes not much sense to add such a heavy weight test as the expected benefit will not really outweigh the test complexity needed, will slow down test performance massively, and was not tested in the original implementation / in https://github.com/openjdk/jdk/blob/master/test/jdk/java/io/FileInputStream/TransferTo.java, too. There is no rule that we have to provide 100% test coverage in OpenJDK. Actually adding a test must be well justified and is not the only measure we have in the box to prevent the occurance of bugs in production: I suggest, instead of a 2GB+ test, we agree that the contributed implementation looks correct to each of us (= applying six-eyes principle). For such a simple code line, this should be enough QA, as in case it unexpectedly would break, it is very likely to get reported by beta testers long before GA (possibly by myself as many of my projects move around 4GB+ sized files in reality), which should be early enough to catch and fix.
• Same for testing position-after-transfer. This is not covered by the original test, and due to our six eyes it is highly unlikely that it is broken or will break in future. So I'd say KISS here.

Wherever it made sense (without sacrifying my intended modularity needed for other channel types tested later) I tried to follow the solutions found in https://github.com/openjdk/jdk/blob/master/test/jdk/java/io/FileInputStream/TransferTo.java. Thanks Bran, for this blue print. In fact I wonder if the introduction of 10 loops over random sizes actually will improve test qualify. My assumption is that it rather reduces test stability and test repeatability: In case there actually is a bug that happens only with a specific combination of buffer size and initial positions, ten iterations will be much too few to stably reproduce the bug at each execution of the test. More often that not, that ten loops will all pass without running into the problem given the huge max size. This will driver testers nuts! The improved entropy by introducing the JDK randomizer does not help much to improve this. Having said that, I copied the solution from Brian only because he came up with it, and I can live with the statistical unpredictability it induces, but just wanted to clearly say that I (spoiler: working for a company providing QA tools like FMEA and SPC software) would say introducing random here induces more harm due to non-repeatability than it actualy improves the detection rate of failures not detected by thorough six-eye review.

[AlanBateman]

• 2GB+: IMHO makes not much sense to add such a heavy weight test as the expected benefit will not really outweigh the test complexity needed,

I'll try to get time to look at what you have in the next few days but just to say that the suggestion for exercising files > 2GB is because it's beyond the limit of the underlying sendfile. You should be able to check this quickly by printing the iteration count in ChannelInputStream.transferTo, it is always 1?

Update with comments on commit 98dc61e:

The src/ implementation changes are fine, I think we agreed those previously.

It would be cleaner if this test had been created as TestNG test. That would allow us to get rid of Thrower and the assertThrowsXXX infrastructure. It would allow us to use data providers too. Maybe it can be converted and cleaned up in a future PR.

I think I mentioned this previously but the test can go into test/jdk/java/nio/channels/Channels so that it's in the same directory as the other tests for the Channels API/implementation.

ifOutIsNullThenNpeIsThrown is a strange method name, can we rename it to something testNullPointerException or something more obvious?

"contents" is also a bit strange for the method name. This where the real test is so let's rename that to something more obvious too. There's a typo in one of the comments here it says "starting point", should be "starting position".

You've got reasonable coverage for random initial positions in both source and target files, good! One other case that could be tested is where the initial position is beyond EOF. If the initial position of the source file is beyond EOF then 0 bytes will be transferred. If the initial position of the target file is beyond EOF then it will be extended. At some point we should look at adding a test for files > 2Gb as otherwise we aren't testing the loop (as we discussed).

A few minor nits: The copyright date says "2014", I assume copied from another test. There are several unused imports to remove. The static modifiers on interfaces aren't needed. There are few really long lines, one is 150+ that wraps and will be annoying for side-by-side diffs when changed in the future.

[mkarg]

I'll try to get time to look at what you have in the next few days but just to say that the suggestion for exercising files > 2GB is because it's beyond the limit of the underlying sendfile. You should be able to check this quickly by printing the iteration count in ChannelInputStream.transferTo, it is always 1?

I understand your concerns and can dispel them. I added a counter that prints the number of iterations and yes, just as you expected, due to the Long.MAX_VALUE there is only a single iteration. But then I simply reduced Long.MAX_VALUE to 1024 and the counter reported thousands of iterations -- just as expected. Even better, the test confirmed that still the number of reported bytes is still correct just as the file contents are.

Hence in case sendfile limits the number of transferred bytes in production use, this quick check has proven that the code is correct and everything actually works fine.

[mkarg]

Is that an OpenJDK best practice or why do you ask for this change? The code actually is a copy of existing OpenJDK code and not originally authored by me.

[mkarg]

The src/ implementation changes are fine, I think we agreed those previously.

Agreed.

It would be cleaner if this test had been created as TestNG test. That would allow us to get rid of Thrower and the assertThrowsXXX infrastructure. It would allow us to use data providers too. Maybe it can be converted and cleaned up in a future PR.

I confirm that I will migrate to TestNG in a future PR.

I think I mentioned this previously but the test can go into test/jdk/java/nio/channels/Channels so that it's in the same directory as the other tests for the Channels API/implementation.

Done in c886fbc.

ifOutIsNullThenNpeIsThrown is a strange method name, can we rename it to something testNullPointerException or something more obvious?

Done in 90a176f.

"contents" is also a bit strange for the method name. This where the real test is so let's rename that to something more obvious too.

Done in 6b4c34c.

There's a typo in one of the comments here it says "starting point", should be "starting position".

Done in cb2a863.

You've got reasonable coverage for random initial positions in both source and target files, good! One other case that could be tested is where the initial position is beyond EOF. If the initial position of the source file is beyond EOF then 0 bytes will be transferred. If the initial position of the target file is beyond EOF then it will be extended.

Done in 852032d.

At some point we should look at adding a test for files > 2Gb as otherwise we aren't testing the loop (as we discussed).

Agreed that at some point (i. e. in some future PR) someone could provide such a test. As explained in #5209 (comment) there is no 100% path coverage rule in OpenJDK, so while this might be a good idea, it seems it is definitively not a mandatory constraint to test that loop at all.

A few minor nits: The copyright date says "2014", I assume copied from another test.

It actually reads "2014, 2021" which means that the original code was first published in 2014 and was last modified in 2021. Oracle once asked me to always do it exactly that way when touching any Oracle source code (which I did here, as the test originated from existing Oracle source code). If OpenJDK is an exceptional case here, then please confirm that I am officially allowed to remove the year 2014 from that list. Until then I have to keep the line exactly the way it is to not get into any troubles with Oracle's legal department.

There are several unused imports to remove.

Done in c3dbc33.

The static modifiers on interfaces aren't needed.

Done in f3a6551.

There are few really long lines, one is 150+ that wraps and will be annoying for side-by-side diffs when changed in the future.

Done in 412fab1. The longest code line now is 115 characters which should be fine for side-by-side views as my screen shows more than 250 characters per line.

I hope that with my recent changes this PR now is in a shape which allows it to get merged. As we agreed on multiple small PRs instead of a single big one, our target should be to contain only the essential changes per each PR but add more PRs ontop for non-essential improvements (which I will be happy to provide).

[AlanBateman]

Agreed that at some point (i. e. in some future PR) someone could provide such a test. As explained in #5209 (comment) there is no 100% path coverage rule in OpenJDK, so while this might be a good idea, it seems it is definitively not a mandatory constraint to test that loop at all.

The override of InputStream::transferTo is calling FileChannel::transferTo in a loop so I don't think it is unreasonable to ask if we have test that has more than one iteration. I'm okay if we defer this to a further step but I do think we should have test for this case.

It actually reads "2014, 2021" which means that the original code was first published in 2014 and was last modified in 2021. Oracle once asked me to always do it exactly that way when touching any Oracle source code (which I did here, as the test originated from existing Oracle source code). If OpenJDK is an exceptional case here, then please confirm that I am officially allowed to remove the year 2014 from that list. Until then I have to keep the line exactly the way it is to not get into any troubles with Oracle's legal department.

This is a new test, the date should be 2021.

[mkarg]

The override of InputStream::transferTo is calling FileChannel::transferTo in a loop so I don't think it is unreasonable to ask if we have test that has more than one iteration. I'm okay if we defer this to a further step but I do think we should have test for this case.

I will look into testing the 2 GB+ iterations after this PR is merged and I'm done with my TestNG PR.

It actually reads "2014, 2021" which means that the original code was first published in 2014 and was last modified in 2021. Oracle once asked me to always do it exactly that way when touching any Oracle source code (which I did here, as the test originated from existing Oracle source code). If OpenJDK is an exceptional case here, then please confirm that I am officially allowed to remove the year 2014 from that list. Until then I have to keep the line exactly the way it is to not get into any troubles with Oracle's legal department.
This is a new test, the date should be 2021.

Done with 1804aa0.

So this PR is fine for merging now? :-)

[AlanBateman]

Done with 1804aa0.

Nearly there, it needs to be "2021," to avoid falling foul of scripts that check headers.

[mkarg]

Done with 1804aa0.

Nearly there, it needs to be "2021," to avoid falling foul of scripts that check headers.

Done with 1f5a456.

[AlanBateman]

I've tested the patch against the current main line (the PR is against a repo that is 4 or 5 months behind the main line). All okay. The expectation is that the test will be improved and iterated on in subsequent PRs.

[openjdk]

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

[openjdk]

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

8273038: ChannelInputStream.transferTo() uses FileChannel.transferTo(FileChannel)

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

• 6750c34: 8270533: AArch64: size_fits_all_mem_uses should return false if its output is a CAS
• a66629a: 8254167: G1: Record regions where evacuation failed to provide targeted iteration
• 286a1f6: 8273440: Zero: Disable runtime/Unsafe/InternalErrorTest.java
• 7d24a33: 8273318: Some containers/docker/TestJFREvents.java configs are running out of memory
• 1513dc7: 8271603: Unnecessary Vector usage in java.desktop
• ea4907a: 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out
• 4eacdb3: 8273104: Refactoring option parser for UL
• 8884d2f: 8273462: ProblemList vmTestbase/vm/mlvm/anonloader/stress/oome/heap/Test.java in -Xcomp mode
• d6d6c06: 8273246: Amend the test java/nio/channels/DatagramChannel/ManySourcesAndTargets.java to execute in othervm mode
• 708407e: 8273168: Remove superfluous use of boxing in java.desktop
• ... and 1258 more: https://git.openjdk.java.net/jdk/compare/66274320251f492b0bba79cdfef88ad5b0d104fa...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 (@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).

[mkarg]

/integrate

[openjdk]

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

[AlanBateman]

/sponsor

[openjdk]

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

• 6750c34: 8270533: AArch64: size_fits_all_mem_uses should return false if its output is a CAS
• a66629a: 8254167: G1: Record regions where evacuation failed to provide targeted iteration
• 286a1f6: 8273440: Zero: Disable runtime/Unsafe/InternalErrorTest.java
• 7d24a33: 8273318: Some containers/docker/TestJFREvents.java configs are running out of memory
• 1513dc7: 8271603: Unnecessary Vector usage in java.desktop
• ea4907a: 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out
• 4eacdb3: 8273104: Refactoring option parser for UL
• 8884d2f: 8273462: ProblemList vmTestbase/vm/mlvm/anonloader/stress/oome/heap/Test.java in -Xcomp mode
• d6d6c06: 8273246: Amend the test java/nio/channels/DatagramChannel/ManySourcesAndTargets.java to execute in othervm mode
• 708407e: 8273168: Remove superfluous use of boxing in java.desktop
• ... and 1258 more: https://git.openjdk.java.net/jdk/compare/66274320251f492b0bba79cdfef88ad5b0d104fa...master

Your commit was automatically rebased without conflicts.

[openjdk]

@AlanBateman @mkarg Pushed as commit 1855574.

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