RFR: 8305744: (ch) InputStream returned by Channels.newInputStream should have fast path for ReadbleByteChannel/WritableByteChannel

[Alan Bateman]

On Fri, 7 Apr 2023 11:45:42 GMT, Markus KARG <duke at openjdk.org> wrote:

> This sub-issue defines the work to be done to implement JDK-8265891 solely for the particular case of ReadableByteChannel-to-WritableByteChannel transfers, where neither Channel is a FileChannel, but including special treatment of SelectableByteChannel.
> 
> *Without* this optimization, the JRE applies a loop where a temporary 16k byte array on the Java heap is used to transport the data between both channels. This implies that data is first transported from the source channel into the Java heap and then from the Java heap to the target channel. For most Channels, as these are NIO citizen, this implies two transfers between Java heap and OS resources, which is rather time consuming. As the data is in no way modified by transferTo(), the temporary allocation of valuable heap memory is just as unnecessary as both transfers to and from the heap. It makes sense to prevent the use of Java heap memory.
> 
> This optimization omits the byte array on the Java heap, effectively omitting the transfers to and from that array in turn. Instead, the transfer happens directly from Channel to Channel reusing a ByteBuffer taken from the JRE's internal buffer management utility which also has a size of 16k. Both measures make the transfer more efficient, as just the very essential resources are used, but not more.
> 
> Using JMH on an in-memory data transfer using custom Channels (to prevent interception by other optimizations already existing in transferTo()) it was possible to measure a 2,5% performance gain. While this does not sound much, just as the spared 16k of Java heap doesn't, we need to consider that this also means sparing GC pressure, power consumption and CO2 footprint, and we need to consider that it could be higher when using different (possibly third-party) kinds of custom Channels (as "real I/O" between heap and OS-resources is way slower than the RAM-to-RAM benchmark applied by me). As the change does not induce new, extraordinary risks compared to the existing source code, I think it is worth gaining that 2,5+ percent. Possibly there might exist some kind of scalable Java-implemented cloud service that is bound to exactly this loop and that transports very heavy traffic, those particular 2,5+ percent could be a huge amount of kWh or CO2 in absolute numbers. Even if not now, th
 ere might be in future.

It would be helpful if there were some benchmarks or something to that shows which cases this helps. The current description isn't very clear on which case there is 2.5% benefit.

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 268:

> 266:         }
> 267: 
> 268:         // ReableByteChannel -> WritableByteChannel

Typo, should be "ReadbleByteChannel"

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 269:

> 267: 
> 268:         // ReableByteChannel -> WritableByteChannel
> 269:         if (out instanceof ChannelOutputStream cos) {

I think it might be better to change the code at L254 to:


        if (out instanceof ChannelOutputStream cos) {
            ReadableByteChannel rbc = ch;
            WritableByteChannel wbc = cos.channel();

            // FileChannel -> WritableByteChannel

            // ...
        }


That puts the special casing for an output stream backed by a ChannelOutputStream in one place.

src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 281:

> 279:                             if (!wsc.isBlocking())
> 280:                                 throw new IllegalBlockingModeException();
> 281:                             return transfer(rbc, wbc);

This introduces the potential for deadlock as tock ordering is not specified for the transferXXX methods. Note that this same issue is problematic for some of the optimisations at the lower level in FileChannel too. We'll need to think about this topic.

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

PR Review: https://git.openjdk.org/jdk/pull/13387#pullrequestreview-1376727746
PR Review Comment: https://git.openjdk.org/jdk/pull/13387#discussion_r1161073194
PR Review Comment: https://git.openjdk.org/jdk/pull/13387#discussion_r1161073814
PR Review Comment: https://git.openjdk.org/jdk/pull/13387#discussion_r1161073156