On Sun, 1 Aug 2021 22:01:33 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? > > Markus KARG has updated the pull request incrementally with two additional > commits since the last revision: > > - Draft: Eliminated duplicate code using lambda expressions > - Draft: Use blocking mode also for target channel src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 175: > 173: throw new IllegalBlockingModeException(); > 174: } > 175: return tls.supply(); It does not look like the `SelectableChannel` branch is tested, including whether an `IllegalBlockingModeException` is thrown when it should be. src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 249: > 247: } > 248: > 249: private static long transfer(ReadableByteChannel src, > WritableByteChannel dst) throws IOException { Does this method have a measurable improvement in performance over `InputStream.transferTo()`? test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 93: > 91: checkTransferredContents(inputStreamProvider, > outputStreamProvider, createRandomBytes(1024, 4096)); > 92: // to span through several batches > 93: checkTransferredContents(inputStreamProvider, > outputStreamProvider, createRandomBytes(16384, 16384)); Should some random-sized buffers be tested? (Use `jdk.test.lib.RandomFactory` factory for this, not `j.u.Random`. The existing use of `Random` is fine.) Should some testing be done of streams with non-zero initial position? test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 101: > 99: try (InputStream in = inputStreamProvider.input(inBytes); > 100: OutputStream out = > outputStreamProvider.output(recorder::set)) { > 101: in.transferTo(out); The return value of `transferTo()` is not checked. ------------- PR: https://git.openjdk.java.net/jdk/pull/4263
