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

[Markus KARG]

On Thu, 17 Jun 2021 20:08:08 GMT, Michael Bien <github.com+114367+mbien at openjdk.org> wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a possible solution for issue [JDK-8265891](https://bugs.openjdk.java.net/browse/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?
>
> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 216:
> 
>> 214:         }
>> 215: 
>> 216:         return super.transferTo(out);
> 
> (i am no reviewer)
> wouldn't this method be more intuitive if it wouldn't try to avoid using "else if" and "else"? If there are sequential if blocks with return in them, part of me is always trying to find the fall-through scenario, but in this case its all distinct code paths. Using branches would make that obvious + if there would be a fall through in future, the compiler would generate a "missing return" error right away. 
> 
> example:
> 
>         if (out instanceof ChannelOutputStream cos) {
>             
>             WritableByteChannel oc = cos.channel();
>             long i = 0L;
> 
>             if (ch instanceof FileChannel fc) {
>                 
>                 long pos = fc.position();
>                 long size = fc.size();
>                 try {
>                     for (long n = size - pos; i < n;)
>                         i += fc.transferTo(pos + i, Long.MAX_VALUE, oc);
>                     return i;
>                 } finally {
>                     fc.position(pos + i);
>                 }
>                 
>             } else if (oc instanceof FileChannel fc) {
>                 
>                 long fcpos = fc.position();
> 
>                 if (ch instanceof SeekableByteChannel sbc) {
>                     
>                     long pos = sbc.position();
>                     long size = sbc.size();
>                     try {
>                         for (long n = size - pos; i < n;)
>                             i += fc.transferFrom(ch, fcpos + i, Long.MAX_VALUE);
>                         return i;
>                     } finally {
>                         fc.position(fcpos + i);
>                     }
>                     
>                 } else {
>                     
>                     ByteBuffer bb = Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
>                     try {
>                         int r;
>                         do {
>                             i += fc.transferFrom(ch, fcpos + i, Long.MAX_VALUE);
>                             r = ch.read(bb); // detect end-of-stream
>                             if (r > -1) {
>                                 bb.flip();
>                                 while (bb.hasRemaining())
>                                     oc.write(bb);
>                                 bb.clear();
>                                 i += r;
>                             }
>                         } while (r > -1);
>                         return i;
>                     } finally {
>                         fc.position(fcpos + i);
>                         Util.releaseTemporaryDirectBuffer(bb);
>                     }
>                 }
> 
>             } else {
>                 
>                 ByteBuffer bb = Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
>                 try {
>                     for (int r = ch.read(bb); r > -1; r = ch.read(bb)) {
>                         bb.flip();
>                         while (bb.hasRemaining())
>                             oc.write(bb);
>                         bb.clear();
>                         i += r;
>                     }
>                     return i;
>                 } finally {
>                     Util.releaseTemporaryDirectBuffer(bb);
>                 }
>             }
>         } else {
>             return super.transferTo(out);
>         }

This is just about personal likes and dislikes. In fact, there is no "missing" case at all as this is not about "there must be THIS solution OR that solution", but this is just about doing "the standard case" vs. doing optimizations "if applicable" -- which is a different semantic. I personally dislike your proposal as it makes the code overly cluttered and hard to read. No other reviewer wanted it that way. So until a majority of reviewers actually wants me to clutter the code with else cases, I'd rather abstain from your proposal.

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

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