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

[Markus KARG]

On Wed, 2 Jun 2021 09:03:03 GMT, Alan Bateman <alanb 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/java/nio/channels/Channels.java line 145:
> 
>> 143:      * @since 18
>> 144:      */
>> 145:     public static class ChannelOutputStream extends OutputStream {
> 
> This adds Channels.ChannelOutputStream to the API, you will need to justify that.

You mean as a source comment or just here in this discussion thread?

In fact it might be better to not add it to a package with is part of the API, but to move it to the `sun` package, which is not, right?

The justification is that I need to refer to this class from `ChannelInputStream::transferTo()` to be able to get hold of this previously anonymous class's inner member "ch", which in turn is key to the whole story of this PR: Using NIO transfer between the channels instead of moving bytes through the JVM's memory.

> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 148:
> 
>> 146: 
>> 147:     @Override
>> 148:     public long transferTo(final OutputStream out) throws IOException {
> 
> Please try to keep to existing style, e.g. most/all "final" are noise and can be removed.

Sorry I am new do this project and didn't know that final is banned generally. To get it right: Is it banned only in parameters or also for variables and class members?

> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 158:
> 
>> 156:                 for (long n = size - pos; i < n;
>> 157:                   i += fc.transferTo(pos + i, Long.MAX_VALUE, oc));
>> 158:                 fc.position(size);
> 
> The patch is improving but needs cleanup so that it is easy to maintain. I think I would move the I/O ops out of the update code and into the for statement/block. Also this will need the update to the channel position to be a finally block so that the it is adjusted when there are partial transfers.

Moved I/O operations into the `for` statement by https://github.com/openjdk/jdk/pull/4263/commits/562b1023e62c4ff0a36e55ebc119cee6fb69809c.

> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 177:
> 
>> 175:                 }
>> 176: 
>> 177:                 final var bb = ByteBuffer.allocateDirect(TRANSFER_SIZE);
> 
> This will probably need to be changed to the use the TL buffer cache.

Uhm... Maybe this is a dumb beginner's question, but: What is "the TL buffer cache"?

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

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