On Sun, 30 May 2021 17:30:56 GMT, Markus KARG <github.com+1701815+mk...@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. 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. 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. 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/4263
