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

[Alan Bateman]

On Sat, 21 Aug 2021 14:33:43 GMT, Markus KARG <github.com+1701815+mkarg 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.

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

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