On Wed, 2 Jun 2021 09:03:03 GMT, Alan Bateman <al...@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
