RFR: 8265891: ChannelInputStream.transferTo() uses FileChannel.transferTo(FileChannel)

[Brian Burkhalter]

On Tue, 24 Aug 2021 12:17:18 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> 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 https://github.com/openjdk/jdk/pull/4263 and https://github.com/openjdk/jdk/pull/5179 and deliberately concentrates **only** on the case where both, source **and** target, are actually `FileChannel`s. 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 https://github.com/openjdk/jdk/pull/5097 which was reported by even provide 5x performance (https://github.com/openjdk/jdk/pull/5097#issuecomment-897271997).
>
> 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.

@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](https://github.com/openjdk/jdk/blob/master/test/jdk/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.)

-------------

PR: https://git.openjdk.java.net/jdk/pull/5209