Where to discuss class proposals?
Dear Core Library Developers, >From discussing with other coders for 25+ years, since the early days of Java, and from having contributed to several Java EE subprojects (like JAX-RS and others) in the past, and from the feedback I got to several of my blog postings on java.net, there turned out a need for some new interfaces / classes in the core libraries (namely Ordinal, Range and Sequence to handle non-integer ordinals, non-number ranges, and content-less lists defined from ranges and step sizes). I (and others, like Martin FOWLER's proposal) actually wrote such interfaces and classes, but they don't make sense unless many JRE core classes adopt them (they have to be part of the JRE, not part of the application). The question is: Where is to discuss such ideas? I tried it by positing feature requests using the Java Bug Database mechanism, but those had been unanswered since months. So even that is mentioned in OpenJDK's new members guide as the preferred way, it seemed it actually is not the way the core library team actually is living. So please tell me: Where and in what form to post ideas for the core libs to get in touch with you in shorter time than months and to actively discuss JRE needed changes lively? Or is it just to the sole grace of Oracle to decide what goes into the JRE? Or what is ACTUAL way to get things into the JRE? Obviously, the Java Bug Database is not. This is not to offend anybody, we all are in heavy stress these days, I just really want to know how to move on with this issue. Thanks Markus KARG Head Crashing Informatics http://www.headcrashing.eu http://www.java.net/blogs/mkarg
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 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? @AlanBateman I'm done with the changes you requested and kindly like to ask where to go from here. Great ideas, will do as you say, stay tuned! Didn't know that using `var` is a no-go, sorry for that. Pattern matching is definitively a great idea! Replaced `var` by actual type in https://github.com/openjdk/jdk/pull/4263/commits/b3c62b0951124550a686386b9419f7246c214dc6. Replaced casting by pattern matching for `instanceof` in https://github.com/openjdk/jdk/pull/4263/commits/ac62cdb9acffc133bfaaa97ee77d5c7b3b704994. There had been no more comments since one week. How to proceed? As there had been no more change requests within one week, I assume this means you all rate this PR as read-to-review, so hereby I change its state and explicitly invite everybody to review this proposal. :-) - PR: https://git.openjdk.java.net/jdk/pull/4263
RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
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? - Commit messages: - Draft: Correctly positioning channel in case of exception - Draft: Corrected try-catch block - Draft: Moving I/O ops into the for statement block - Draft: Replaced custom bounds check by Objects.checkFromIndexSize() - Draft: Replaced casting by pattern matching for instanceof - Draft: Replacing var by actual type - Draft: Hiding ChannelOutputStream from the API - Draft: Removing final everywhere but kept only for constants - Draft: Using Thread-Local Buffer Cache - Draft: Increased DEFAULT_BUFFER_SIZE - ... and 1 more: https://git.openjdk.java.net/jdk/compare/66274320...ed49098a Changes: https://git.openjdk.java.net/jdk/pull/4263/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265891 Stats: 286 lines in 4 files changed: 205 ins; 76 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Tue, 8 Jun 2021 11:49:55 GMT, Alan Bateman wrote: >> @AlanBateman I'm done with the changes you requested and kindly like to ask >> where to go from here. > >> @AlanBateman I'm done with the changes you requested and kindly like to ask >> where to go from here. > > Moving ChannelOutputStream to sun.nio.ch looks right. The implementation of > transferTo will need a few rounds of cleanup, it's a look very messy right > now. Most usages of var need to be replaced as it is impossible for the > reader to know what the types are. Pattern matching for instanceof can be > used to avoid the casts. @AlanBateman Pushed the requested changes. More change requests? :-) - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Wed, 2 Jun 2021 09:03:03 GMT, Alan Bateman 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
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Sat, 5 Jun 2021 07:25:44 GMT, Markus KARG wrote: >> 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. > > Will move the class (and the needed utility methods) to the `sun` package to > prevent addition to the API. Stay tuned. :-) `ChannelOutputStream` isn't part of the API anymore as I moved it to the `sun` package by commit https://github.com/openjdk/jdk/pull/4263/commits/b6dc6a7eadb5168531459aa032f169b344cadb47. Thankyou for pointing this out! >> Moved I/O operations into the `for` statement by >> https://github.com/openjdk/jdk/pull/4263/commits/562b1023e62c4ff0a36e55ebc119cee6fb69809c. > > Correctly positioning channel in case of exception by > https://github.com/openjdk/jdk/pull/4263/commits/ed49098a4712bf23cb6d218c27695717ba3312c2. Honestly, for me the code looks perfectly comprehensible now, so I actually do not know what to simplify any further (besides adjusting to personal code style likes or dislikes). If there actually *is* something hard to read or understand, please don't hesitate to express your detailed concerns and/or propose an actual change. I will be happy to adopt it in this PR. >> Uhm... Maybe this is a dumb beginner's question, but: What is "the TL buffer >> cache"? > > I think I found what you mean and will use it: > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/nio/ch/Util.java#L221 Done in https://github.com/openjdk/jdk/pull/4263/commits/8f589867154f8bc22ca6f6bb7a26d7ae841fe714. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Thu, 3 Jun 2021 17:29:14 GMT, Markus KARG
wrote:
>> 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.
Will move the class (and the needed utility methods) to the `sun` package to
prevent addition to the API. Stay tuned. :-)
>> 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?
Checked the existing code and removed most `final`s but kept it only for
immitable members and real constants in
https://github.com/openjdk/jdk/pull/4263/commits/df1a57a12cc81bbdcb36d3caf66a7aea7cc542cb.
>> 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.
Correctly positioning channel in case of exception by
https://github.com/openjdk/jdk/pull/4263/commits/ed49098a4712bf23cb6d218c27695717ba3312c2.
>> 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"?
I think I found what you mean and will use it:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/nio/ch/Util.java#L221
-
PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Tue, 8 Jun 2021 20:34:15 GMT, Brian Burkhalter wrote: >> I'd like to abstain from changes in ChannelOutputStream, as I did not write >> that code at all. It is simply moved from being an inner class. Please let's >> concentrate on the code I actually wrote in this PR. Thanks. > > That's fine. Sometimes we make small changes like that while we are in nearby > code. I see. Changed by https://github.com/openjdk/jdk/pull/4263/commits/599181bd5289a1c31e0a3bb596c979a9e6491ccd. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Tue, 8 Jun 2021 19:26:14 GMT, Brian Burkhalter 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/sun/nio/ch/ChannelOutputStream.java line 113:
>
>> 111: if ((off < 0) || (off > bs.length) || (len < 0) ||
>> 112: ((off + len) > bs.length) || ((off + len) < 0)) {
>> 113: throw new IndexOutOfBoundsException();
>
> Could the bounds checking be done as follows?
>
> `Objects.checkFromIndexSize(off, len, bs.length);`
>
> Same comment applies to lines 146 and 219 in `java.nio.channels.Channels`.
I'd like to abstain from changes in ChannelOutputStream, as I did not write
that code at all. It is simply moved from being an inner class. Please let's
concentrate on the code I actually wrote in this PR. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Thu, 17 Jun 2021 20:08:08 GMT, Michael Bien
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/sun/nio/ch/ChannelInputStream.java line 216:
>
>> 214: }
>> 215:
>> 216: return super.transferTo(out);
>
> (i am no reviewer)
> wouldn't this method be more intuitive if it wouldn't try to avoid using
> "else if" and "else"? If there are sequential if blocks with return in them,
> part of me is always trying to find the fall-through scenario, but in this
> case its all distinct code paths. Using branches would make that obvious + if
> there would be a fall through in future, the compiler would generate a
> "missing return" error right away.
>
> example:
>
> if (out instanceof ChannelOutputStream cos) {
>
> WritableByteChannel oc = cos.channel();
> long i = 0L;
>
> if (ch instanceof FileChannel fc) {
>
> long pos = fc.position();
> long size = fc.size();
> try {
> for (long n = size - pos; i < n;)
> i += fc.transferTo(pos + i, Long.MAX_VALUE, oc);
> return i;
> } finally {
> fc.position(pos + i);
> }
>
> } else if (oc instanceof FileChannel fc) {
>
> long fcpos = fc.position();
>
> if (ch instanceof SeekableByteChannel sbc) {
>
> long pos = sbc.position();
> long size = sbc.size();
> try {
> for (long n = size - pos; i < n;)
> i += fc.transferFrom(ch, fcpos + i,
> Long.MAX_VALUE);
> return i;
> } finally {
> fc.position(fcpos + i);
> }
>
> } else {
>
> ByteBuffer bb =
> Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
> try {
> int r;
> do {
> i += fc.transferFrom(ch, fcpos + i,
> Long.MAX_VALUE);
> r = ch.read(bb); // detect end-of-stream
> if (r > -1) {
> bb.flip();
> while (bb.hasRemaining())
> oc.write(bb);
> bb.clear();
> i += r;
> }
> } while (r > -1);
> return i;
> } finally {
> fc.position(fcpos + i);
> Util.releaseTemporaryDirectBuffer(bb);
> }
> }
>
> } else {
>
> ByteBuffer bb = Util.getTemporaryDirectBuffer(TRANSFER_SIZE);
> try {
> for (int r = ch.read(bb); r > -1; r = ch.read(bb)) {
> bb.flip();
> while (bb.hasRemaining())
> oc.write(bb);
> bb.clear();
> i += r;
> }
> return i;
> } finally {
> Util.releaseTemporaryDirectBuffer(bb);
> }
> }
> } else {
> return su
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo
On Sun, 30 May 2021 17:30:56 GMT, Markus KARG 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? I am a bit lost currently. So what actually do I have to do next? You want *me* to write these tests, or shall I just refactor my implementation and *you* will provide these tests (I would prefer that, actually)? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v2]
> 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 one additional commit since the last revision: Draft: Renaming i and separating code into several methods Signed-off-by: Markus Karg - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/ed49098a..7d18ca62 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=00-01 Stats: 108 lines in 1 file changed: 57 ins; 39 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]
> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: Draft: Renaming i and separating code into several methods Signed-off-by: Markus Karg - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/7d18ca62..47ee00a2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v2]
On Thu, 1 Jul 2021 21:59:33 GMT, Markus KARG 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 one additional > commit since the last revision: > > Draft: Renaming i and separating code into several methods > > Signed-off-by: Markus Karg I have renamed `i` (and other ambiguous variables) and separated the implementation of `transferTo` into multiple methods. I will look into the test examples next to learn what I can reuse when writing tests for all those implementations. Thank you for all the kind input and the good ideas. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]
On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG 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 refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. I cloned the `InputStream/TransferTo` test in a way which uses providers, but when started to implement the providers I noticed that a huge amount of *empty* source code is needed just to make the compiler happy: `FileChannel` already has dozens of abstract methods I have to override but they never will be used by any of the tests... Having that said, it would be good if I would be allowed to spare us thousands of useless codelines: * May I just implement the `content()` test or do you insist on me really implementing *all* the tests found in `InputStream/TransferTo` for *each* of the overloaded `transferTo` implementations? * May I use a mocking framework to actually just provide *partial* implementations of the channels, or do you insist on me actually overloading *all* methods of *all* channels in actual Java source code just for the sake of making the compiler happy? While I understand the necessity of tests, and while I am convinced that the performance benefit outweighs the weeks of work this would imply just for adding all those test code, I actually like to propose that I only cover the `content()` tests plus using Mockito, so we would have 80% of the benefit for 20% of the coding costs. WDYT? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]
On Wed, 14 Jul 2021 18:35:15 GMT, Brian Burkhalter wrote:
> Have you looked into other tests which have implemented Providers?
I have not found any test code containing the word `new FileChannel() {...}` or
`extends FileChannel`, so I apparently there exists no mock of `FileChannel`.
Unfortunately, as long as it is mandatory to copy *all* the tests of
`InputStream/TransferTo` this means that I *must* implement *all* methods of
`FileChannel` (even if they are empty) just for the sake of throwing an
exception after one, two, or three transferred bytes (which, BTW, is not part
of the API but looks like just a personal decision of the original test
author). It is not really helpful that OpenJDK *neither* uses mocking *nor*
lets me reduce the number of tests to the *essential* minimum, as this means, I
wrote approx. 100 LoC for the optimized implementations and now I have to write
approx. 1000 LoC just to perform *exactly the same tests* (to be clear: I have
no problem with testing `content()` which certainly *has* to be tested, but
which does *not* imply writing a `FileChannel` mock).
-
PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]
On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG 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 refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Draft: Renaming i and separating code into several methods > > Signed-off-by: Markus Karg Understood. Work is in progress. Stay tuned. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v4]
> 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 four additional commits since the last revision: - Draft: Test for ChannelInputStream::transferTo - Draft: Using Channels API to invoke sun.nio.ch classes - Draft: MUST check params before ANY other operation - Draft: ChannelInputStream::transferTo Test - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/47ee00a2..b431fcd6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=02-03 Stats: 229 lines in 2 files changed: 229 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]
On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG 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 refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Sorry for the delay. I now have pushed a first draft of the test. IIUC then five implementation have to be tested just for the actualy API, i. e. whether they throws NPE when `out` is `null`, and whether they transfer small, single turn, and multiple turn loads correctly. The tests are taken from `InputStream/TransferTo.java` as you proposed, and I am not using mocking but instead use `Channels.new*` as the existing tests you proposed as a blueprint. I hope I understood your requests and proposals correctly, if not please tell me. The tests pass well and proof that the new implementations work as expected and API-compliant. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v5]
> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: Draft: Test for ChannelInputStream::transferTo - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/b431fcd6..bfc699a0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=03-04 Stats: 227 lines in 1 file changed: 0 ins; 0 del; 227 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: Draft: Test for ChannelInputStream::transferTo - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/bfc699a0..1f9dba3d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=04-05 Stats: 173 lines in 1 file changed: 6 ins; 0 del; 167 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Tue, 27 Jul 2021 00:57:02 GMT, Brian Burkhalter wrote: >> Markus KARG has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. > > src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 85: > >> 83: private byte[] bs; // Invoker's previous array >> 84: private byte[] b1; >> 85: > > It might be better to put the field declarations at the beginning of the > class before the static methods. This is a question of style and personal likes. Code maintenance is much easier if variables are defined *near* to its first use, not *far away* (as in the Pascal or C language). If the reviewers want me to change it, I will do it. > test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 67: > >> 65: test(readableByteChannelInput(), defaultOutput()); >> 66: } >> 67: > > This test looks like it's doing what it's supposed to do. I'm not asking to > change it, but I think using TestNG might have given a simpler result with > less work. For example, there could be one `DataProvider` supplying inputs > which feed a `@Test` which expects an NPE, and another `DataProvider` > supplying input-output pairs which feeds a `@Test` which validates the > functionality. No doubt, using modern tools would have spared me work, and indeed I would have chosen JUnit or TestNG if there would be a clear commitment to that tool *upfront*. For now, I see now use in reworking the code afterwards. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Mon, 26 Jul 2021 23:59:05 GMT, Brian Burkhalter wrote: >> Markus KARG has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. > > src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 179: > >> 177: for (long n = srcSize - srcPos; bytesWritten < n;) >> 178: bytesWritten += src.transferTo(srcPos + bytesWritten, >> Long.MAX_VALUE, dest); >> 179: return bytesWritten; > > If `src` is extended at an inconvenient point in time, for example between > the return from a call to `src.transferTo()` that makes `bytesWritten < n` > false and the evaluation of that terminating condition, then it appears that > not all the content of `src` will be transferred to `dest`. I am not however > sure that this violates any contract but it could be a behavioral change from > the existing implementation. The JavaDocs in `InputStream::transferTo` *cleary* tell the caller that there is **no** guarantee of *any* specific behavior in that particular case: >The behavior for the case where the input and/or output stream is >asynchronously closed, or the thread interrupted during the transfer, is >highly input and output stream specific, and therefore not specified. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Thu, 29 Jul 2021 16:42:35 GMT, Brian Burkhalter wrote: >> The JavaDocs in `InputStream::transferTo` *cleary* tell the caller that >> there is **no** guarantee of *any* specific behavior in that particular >> case: >>>The behavior for the case where the input and/or output stream is >>>asynchronously closed, or the thread interrupted during the transfer, is >>>highly input and output stream specific, and therefore not specified. > > Good point. :-) >> This is a question of style and personal likes. Code maintenance is much >> easier if variables are defined *near* to its first use, not *far away* (as >> in the Pascal or C language). If the reviewers want me to change it, I will >> do it. > > It's actually a matter of convention but I think it can remain as it is. Ok for me, otherwise just clearly tell me and I do change it. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Fri, 30 Jul 2021 12:09:30 GMT, Alan Bateman wrote: > Just on naming, the existing channel implementations use "dst" for the > destination and "wbc" (not "oc") for writable byte channels. Just mentioning > it so that the new code can be kept consistent where possible. I have renamed `dest` to `dst` and `oc` to `wbc` in https://github.com/openjdk/jdk/pull/4263/commits/f91d5422c41e25410a81aad54a45b2d0e171305a. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v7]
> 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 one additional commit since the last revision: Draft: Renamed oc to wbc and dest to dst - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/1f9dba3d..f91d5422 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=05-06 Stats: 17 lines in 1 file changed: 0 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v8]
> 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: Renamed r to bytesRead - Draft: Prevent infinite loop after truncation - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/f91d5422..f3e37fe1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=06-07 Stats: 16 lines in 1 file changed: 2 ins; 2 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v9]
> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains two new commits since the last revision: - Draft: Renamed r to bytesRead - Draft: Prevent infinite loop after truncation - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/f3e37fe1..e0724718 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=07-08 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Sat, 31 Jul 2021 13:12:54 GMT, Markus KARG wrote: >> Is this loop correct for the case that the channel gets truncated? In that >> case transferTo will return 0 as no bytes will be transferred and I'm >> concerned this code will go into an infinite loop. >> >> Also can you can check that IllegalBlockingModeException is thrown for the >> case ch is a SelectableChannel configured non-blocking and the destination >> is a FileChannel? >> >> Just on naming, the existing channel implementations use "dst" for the >> destination and "wbc" (not "oc") for writable byte channels. Just mentioning >> it so that the new code can be kept consistent where possible. > >> Just on naming, the existing channel implementations use "dst" for the >> destination and "wbc" (not "oc") for writable byte channels. Just mentioning >> it so that the new code can be kept consistent where possible. > > I have renamed `dest` to `dst` and `oc` to `wbc` in > https://github.com/openjdk/jdk/pull/4263/commits/f91d5422c41e25410a81aad54a45b2d0e171305a. > Is this loop correct for the case that the channel gets truncated? In that > case transferTo will return 0 as no bytes will be transferred and I'm > concerned this code will go into an infinite loop. Good catch, indeed missed this particular situation! The modified code found in https://github.com/openjdk/jdk/pull/4263/commits/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20 should be safe from infinite loops, as it checks the actual file length in each iteration (even if this costs us one more `synchronized` per iteration). This will also improve the situation with concurrent *extension* of the file as outlined in: >If src is extended at an inconvenient point in time, for example between the >return from a call to src.transferTo() that makes bytesWritten < n false and >the evaluation of that terminating condition, then it appears that not all the >content of src will be transferred to dest. I am not however sure that this >violates any contract but it could be a behavioral change from the existing >implementation. As `size()` is called in each iteration, it will pick up the appendend bytes. Still the situation exists that some bytes are *replaced* in the source file -- but that is actually out-of-scope of this PR...! ;-) - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v10]
> 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 one additional commit since the last revision: Draft: Throwing IllegalBlockingModeException if src is SelectableChannel and dst is FileChannel - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/e0724718..8e2889fd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=08-09 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Sat, 31 Jul 2021 14:39:02 GMT, Markus KARG wrote: >>> Just on naming, the existing channel implementations use "dst" for the >>> destination and "wbc" (not "oc") for writable byte channels. Just >>> mentioning it so that the new code can be kept consistent where possible. >> >> I have renamed `dest` to `dst` and `oc` to `wbc` in >> https://github.com/openjdk/jdk/pull/4263/commits/f91d5422c41e25410a81aad54a45b2d0e171305a. > >> Is this loop correct for the case that the channel gets truncated? In that >> case transferTo will return 0 as no bytes will be transferred and I'm >> concerned this code will go into an infinite loop. > > Good catch, indeed missed this particular situation! > > The modified code found in > https://github.com/openjdk/jdk/pull/4263/commits/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20 > should be safe from infinite loops, as it checks the actual file length in > each iteration (even if this costs us one more `synchronized` per iteration). > > This will also improve the situation with concurrent *extension* of the file > as outlined in: > >>If src is extended at an inconvenient point in time, for example between the >>return from a call to src.transferTo() that makes bytesWritten < n false and >>the evaluation of that terminating condition, then it appears that not all >>the content of src will be transferred to dest. I am not however sure that >>this violates any contract but it could be a behavioral change from the >>existing implementation. > > As `size()` is called in each iteration, it will pick up the appendend bytes. > Still the situation exists that some bytes are *replaced* in the source file > -- but that is actually out-of-scope of this PR...! ;-) > Also can you can check that IllegalBlockingModeException is thrown for the > case ch is a SelectableChannel configured non-blocking and the destination is > a FileChannel? Done in https://github.com/openjdk/jdk/pull/4263/commits/8e2889fd6138d8aa8e308a1cd2aea3546a7c43a7, but honestly I'd kindly like to ask for a brief explanation why this has to be done. What actual bad effect would happen if I do not throw? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Sat, 31 Jul 2021 17:33:50 GMT, Alan Bateman wrote: > I need to look at it closely but I suspect this introduces a potential > overflow. Also if output stream is backed by a SocketChannel configured > non-blocking then FC::transferTo may return 0 so I assume there is a > potential infinite loop there too. I suspect the eventually patch will need > have to make use of the blockingLock to prevent the underlying channels from > being changed to non-blocking during the transfer. I need to confess that my NIO knowledge is not deep enough to follow you closely, so I trust on your advice how to go on from here. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v11]
> 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 one additional commit since the last revision: [Draft] Using blocking lock to prevent concurrently switching into non-blocking mode - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/8e2889fd..f4485d5b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=09-10 Stats: 16 lines in 1 file changed: 8 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Sat, 31 Jul 2021 21:20:28 GMT, Markus KARG wrote: > I suspect the eventually patch will need have to make use of the blockingLock > to prevent the underlying channels from being changed to non-blocking during > the transfer. The blocking lock now is used since https://github.com/openjdk/jdk/pull/4263/commits/f4485d5b6a3b5c471feff5642dfef0fc747d3fac to prevent this. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Sun, 1 Aug 2021 07:46:36 GMT, Markus KARG wrote: >>> I need to look at it closely but I suspect this introduces a potential >>> overflow. Also if output stream is backed by a SocketChannel configured >>> non-blocking then FC::transferTo may return 0 so I assume there is a >>> potential infinite loop there too. I suspect the eventually patch will need >>> have to make use of the blockingLock to prevent the underlying channels >>> from being changed to non-blocking during the transfer. >> >> I need to confess that my NIO knowledge is not deep enough to follow you >> closely, so I trust on your advice how to go on from here. > >> I suspect the eventually patch will need have to make use of the >> blockingLock to prevent the underlying channels from being changed to >> non-blocking during the transfer. > > The blocking lock now is used since > https://github.com/openjdk/jdk/pull/4263/commits/f4485d5b6a3b5c471feff5642dfef0fc747d3fac > to prevent this. > Also can you can check that IllegalBlockingModeException is thrown for the > case ch is a SelectableChannel configured non-blocking and the destination is > a FileChannel? Do we really only have to check this in the particular case of `FileChannel` destinations? And don't we have to assert blocking mode for *destination* channels, too (just like `ChannelOutputStream::writeFully` does)? As this results in an 2:2 matrix of possibilities (src is selectable nor not, dst is selectable or not) it might be easier to do *only the blocking checks* in `transferTo` but let it call something like `transferFromSelectableToNonSelectable` in turn *or* to wrap *each* implementation of `transferTo` in a wrapper like `executeInBlockingMode((src, dst) -> transferTo(src, dst))`...? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Sat, 31 Jul 2021 17:33:50 GMT, Alan Bateman wrote: >>> Also can you can check that IllegalBlockingModeException is thrown for the >>> case ch is a SelectableChannel configured non-blocking and the destination >>> is a FileChannel? >> >> Done in >> https://github.com/openjdk/jdk/pull/4263/commits/8e2889fd6138d8aa8e308a1cd2aea3546a7c43a7, >> but honestly I'd kindly like to ask for a brief explanation why this has to >> be done. What actual bad effect would happen if I do not throw? > >> The modified code found in >> [4b501b2](https://github.com/openjdk/jdk/commit/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20) >> should be safe from infinite loops, as it checks the actual file length in >> each iteration (even if this costs us one more `synchronized` per iteration). > > I need to look at it closely but I suspect this introduces a potential > overflow. Also if output stream is backed by a SocketChannel configured > non-blocking then FC::transferTo may return 0 so I assume there is a > potential infinite loop there too. I suspect the eventually patch will need > have to make use of the blockingLock to prevent the underlying channels from > being changed to non-blocking during the transfer. @AlanBateman Did I recap the sum your comments correctly, is the above conclusion what you wanted to tell me? Shall I proceed with one of the two solutions outlined in the "...2:2 matrix..." section of my answer *or* shall I wait until you had a deeper look? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v12]
> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: [Draft] Using blocking lock to prevent concurrently switching into non-blocking mode - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/f4485d5b..55c96880 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=10-11 Stats: 16 lines in 1 file changed: 8 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/55c96880..4a0d7cf7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=11-12 Stats: 43 lines in 1 file changed: 21 ins; 8 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Sun, 1 Aug 2021 08:24:13 GMT, Markus KARG wrote: >>> The modified code found in >>> [4b501b2](https://github.com/openjdk/jdk/commit/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20) >>> should be safe from infinite loops, as it checks the actual file length in >>> each iteration (even if this costs us one more `synchronized` per >>> iteration). >> >> I need to look at it closely but I suspect this introduces a potential >> overflow. Also if output stream is backed by a SocketChannel configured >> non-blocking then FC::transferTo may return 0 so I assume there is a >> potential infinite loop there too. I suspect the eventually patch will need >> have to make use of the blockingLock to prevent the underlying channels from >> being changed to non-blocking during the transfer. > > @AlanBateman Did I recap the sum your comments correctly, is the above > conclusion what you wanted to tell me? Shall I proceed with one of the two > solutions outlined in the "...2:2 matrix..." section of my answer *or* shall > I wait until you had a deeper look? Asserting blocking mode for *both* sides (source and target) in https://github.com/openjdk/jdk/pull/4263/commits/fc38eae44de9e16468d33bd2ebab6502c92b4860. Eliminated the resulting duplicate code in https://github.com/openjdk/jdk/pull/4263/commits/4a0d7cf74ee7e35aa0448df4b5ea4c5e3113ece6. Do you see more problems we need to solve? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 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 I think I fixed all requested changes. Anymore comments on this PR? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Wed, 11 Aug 2021 11:27:53 GMT, Alan Bateman wrote: > I've looked through the latest revision. Is there any way that we could drop > most of the changes to ChannelInputStream and focus on one or two specific > cases? I'm asking because there are several issues, inconsistencies, and it > is trying to cover many scenarios that aren't covered by the test. > If the original motivation was file -> file then it could be simplified down > to a FileChannel -> FileChannel transfer as the default provider uses file > channels. We could even push some support into FileChannelImpl so that it is > done while holding the position lock. I am a bit disappointed actually about that destructive answer at that late point in time, now that I worked for months on all the requested changes and tests. To prevent exactly this situation, I deliberately had the discussion started in JIRA only, and I deliberately had the original code being just a draft in the first place, and I deliberately did nearly *everything* I was asked to (including even the most irrelevant minor code style issues). And you come up with the request to drop the code **now**? Certainly we could reduce the PR to just file channels, but in fact, now that I spent all the time in the non-file-channels, I wonder why I shall throw away all that work and go just with file channels actually? What is not covered that was originally covered, and what is that lots of issues you talk about? Actually I cannot see the actual problem unless you name it. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Thu, 12 Aug 2021 08:40:34 GMT, Alan Bateman wrote: >> 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 > >> I am a bit disappointed actually about that destructive answer at that late >> point in time, now that I worked for months on all the requested changes and >> tests. To prevent exactly this situation, I deliberately had the discussion >> started in JIRA only, and I deliberately had the original code being just a >> draft in the first place, and I deliberately did nearly _everything_ I was >> asked to (including even the most irrelevant minor code style issues). And >> you come up with the request to drop the code **now**? >> >> Certainly we could reduce the PR to just file channels, but in fact, now >> that I spent all the time in the non-file-channels, I wonder why I shall >> throw away all that work and go just with file channels actually? What is >> not covered that was originally covered, and what is that lots of issues you >> talk about? Actually I cannot see the actual problem unless you name it. > > There are 78 comments on this PR so far. We've tried to point out the bugs > and issues at each iteration. We asked for tests because the changes > introduce several code paths and implementations that would not be exercised > by existing tests. There are several scenarios still missing and the patch > doesn't yet have the microbenchmarks to demonstrate the improvements. > > I assume this is your first contribution so there will be learning curve and > maybe some frustration. I think you have a better chance of success if you > split this up and reduce the scope of this PR down to something manageable. > Keeping the selectable channels out of this PR and focusing on the case where > the input and output streams wrap file channels should make it simpler and > may lead to a better solution. Reducing the scope will also reduce the burden > on reviewers. > I do not know exactly what @AlanBateman had in mind, but I think there is > general concern about ensuring that all combinations of channel types and all > execution paths are exercised. @AlanBateman @bplb Does it make **any** real sense to answer your recent questions, provide the proofs, tests and benchmark results (I actually would love to *if* it makes sense) *or* will the outcome be that I *must* drop everything besides file channels *anyways* (In that case it is in vain)? As my time is just as precious as yours I really need to know that **before** I spend more weeks into code paths that you possibly decided to never accept ever. Don't get me wrong, if you see a chance to keep the code once I provided the answers I will do that, but if you do not see that chance, please frankly and unambiguously tell me **now**. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Fri, 13 Aug 2021 12:54:55 GMT, Alan Bateman wrote: > > Does it make **any** real sense to answer your recent questions, provide > > the proofs, tests and benchmark results (I actually would love to _if_ it > > makes sense) _or_ will the outcome be that I _must_ drop everything besides > > file channels _anyways_ (In that case it is in vain)? As my time is just as > > precious as yours I really need to know that **before** I spend more weeks > > into code paths that you possibly decided to never accept ever. Don't get > > me wrong, if you see a chance to keep the code once I provided the answers > > I will do that, but if you do not see that chance, please frankly and > > unambiguously tell me **now**. Thanks. > > I think the best course of action is to reduce the scope of this PR to the > file channel cases. There is no reason why future PRs can't build on this and > add implementations for other channel types. Agreed, I will split this PR into several PRs, so we can start with the low-hanging fruits first and later dive into the more complex cases. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 15 Aug 2021 13:37:23 GMT, Markus KARG wrote: > I think the best course of action is to reduce the scope of this PR to the > file channel cases. There is no reason why future PRs can't build on this and > add implementations for other channel types. I have split up this PR so that only the lowest hanging fruit is covered and kindly request review: https://github.com/openjdk/jdk/pull/5179. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Thu, 12 Aug 2021 01:10:15 GMT, Brian Burkhalter wrote:
>> 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
>
> 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?
Done in https://github.com/openjdk/jdk/pull/5209, will rebase on that PR once
merged.
> 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.
Done in https://github.com/openjdk/jdk/pull/5209, will rebase on that PR once
merged.
-
PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 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 Work on this draft will be continued as soon as depenency PRs are resolved. Please keep this issue open. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 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 Please keep this PR open. More commits will follow on the next weeks. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 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 Please keep this PR open. More commits will follow on the next weeks. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Mon, 6 Dec 2021 07:07:03 GMT, Markus KARG wrote: >> 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 > > Please keep this PR open. More commits will follow on the next weeks. > Looks promising, keep moving, @mkarg ! Thanks a lot, Laurent! I really appreciate! In fact, most of the work is going on in spin-off PRs, and some of it already is merged, as I explained on [Twitter](https://twitter.com/mkarg/status/1467064781978931206?s=20) and [Youtube](https://youtu.be/9Rjz3zRXoD4). :-) - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Thu, 12 Aug 2021 01:04:29 GMT, Brian Burkhalter wrote:
>> 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 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()`?
Measured it today, here are the actual results:
Sandbox.transferTo thrpt 25 0,091 ± 0,011 ops/s -- using byte[] (original
implementation)
Sandbox.transferTo thrpt 25 0,095 ± 0,012 ops/s -- using utility temporary
buffer (my contribution)
Sandbox.transferTo thrpt 23 0,099 ± 0,012 ops/s --
FileChannel.transferTo(FileChannel)
The improvement of this method actually is approx. 4% on my Windows laptop,
which I would say is worth to do it.
-
PR: https://git.openjdk.java.net/jdk/pull/4263
RFR: 8279283 - (ch) BufferedInputStream should override transferTo
Implementation of JDK-8279283 - Commit messages: - BufferedInputStream::transferTo Changes: https://git.openjdk.java.net/jdk/pull/6935/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279283 Stats: 15 lines in 1 file changed: 15 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6935.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6935/head:pull/6935 PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8279283 - (ch) BufferedInputStream should override transferTo [v2]
> Implementation of JDK-8279283 Markus KARG has updated the pull request incrementally with one additional commit since the last revision: synchronized BufferedInputStream::transferTo - Changes: - all: https://git.openjdk.java.net/jdk/pull/6935/files - new: https://git.openjdk.java.net/jdk/pull/6935/files/2831898e..8dac240d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6935.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6935/head:pull/6935 PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8279283 - (ch) BufferedInputStream should override transferTo
On Mon, 27 Dec 2021 08:50:26 GMT, Alan Bateman wrote: > BIS is not specified to be thread safe but the existing read/write operations > are. If transferTo is overridden then this is an area that will require close > attention. In analogy to the other read/write operations I now have synchronized transferTo in https://github.com/openjdk/jdk/pull/6935/commits/8dac240d2716a9d14c997ea62b7f96637acc8d66 to be on the safe side. > Have you surveyed the existing tests to see if transferTo is invoked on a > BIS? New tests may be needed. Did not find an existing test for BIS.transferTo, so I will write a new test for this. - PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8279283 - (ch) BufferedInputStream should override transferTo [v3]
> Implementation of JDK-8279283 Markus KARG has updated the pull request incrementally with one additional commit since the last revision: test for BufferedInputStream.transferTo - Changes: - all: https://git.openjdk.java.net/jdk/pull/6935/files - new: https://git.openjdk.java.net/jdk/pull/6935/files/8dac240d..fbc5def7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=01-02 Stats: 210 lines in 1 file changed: 210 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6935.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6935/head:pull/6935 PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8279283 - (ch) BufferedInputStream should override transferTo
On Mon, 27 Dec 2021 09:30:25 GMT, Markus KARG wrote: > Have you surveyed the existing tests to see if transferTo is invoked on a > BIS? New tests may be needed. I have provided a test for BIS.transferTo in https://github.com/openjdk/jdk/pull/6935/commits/fbc5def7af6634b630c0bf423d8c7a2715b25cf4. - PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8279283 - (ch) BufferedInputStream should override transferTo [v4]
> Implementation of JDK-8279283 Markus KARG has updated the pull request incrementally with one additional commit since the last revision: removed unused code - Changes: - all: https://git.openjdk.java.net/jdk/pull/6935/files - new: https://git.openjdk.java.net/jdk/pull/6935/files/fbc5def7..304a4aa4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=02-03 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6935.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6935/head:pull/6935 PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8279283 - (ch) BufferedInputStream should override transferTo [v5]
> Implementation of JDK-8279283 Markus KARG has updated the pull request incrementally with one additional commit since the last revision: fixed missing BufferedInputStream - Changes: - all: https://git.openjdk.java.net/jdk/pull/6935/files - new: https://git.openjdk.java.net/jdk/pull/6935/files/304a4aa4..0d3528ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6935&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6935.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6935/head:pull/6935 PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8279283 - BufferedInputStream should override transferTo [v5]
On Mon, 27 Dec 2021 13:43:12 GMT, Markus KARG wrote: >> Implementation of JDK-8279283 > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > fixed missing BufferedInputStream Good catches, I will look into your comments! - PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 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 Please keep this PR open as I am working on several sub-issues currently. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8279283 - BufferedInputStream should override transferTo [v5]
On Mon, 27 Dec 2021 13:43:12 GMT, Markus KARG wrote: >> Implementation of JDK-8279283 > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > fixed missing BufferedInputStream Please keep open, still working on it. - PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 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 Please keep this PR open. I am still working on it. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8279283 - BufferedInputStream should override transferTo [v5]
On Mon, 27 Dec 2021 13:43:12 GMT, Markus KARG wrote: >> Implementation of JDK-8279283 > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > fixed missing BufferedInputStream Please keep this PR open; I will continune work on it soon. - PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 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 Please keep this PR open; I will continune work on it soon. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8279283 - BufferedInputStream should override transferTo [v5]
On Mon, 27 Dec 2021 13:43:12 GMT, Markus KARG wrote: >> Implementation of JDK-8279283 > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > fixed missing BufferedInputStream Please keep this PR open; I will continune work on it soon. - PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG 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 Please keep this PR open; I will continune work on it soon. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8279283 - BufferedInputStream should override transferTo [v5]
On Mon, 27 Dec 2021 13:43:12 GMT, Markus KARG wrote: >> Implementation of JDK-8279283 > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > fixed missing BufferedInputStream Please keep this PR open; I will continune work on it soon. - PR: https://git.openjdk.java.net/jdk/pull/6935
Re: RFR: 8279283 - BufferedInputStream should override transferTo [v5]
On Mon, 27 Dec 2021 13:43:12 GMT, Markus KARG wrote: >> Implementation of JDK-8279283 > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > fixed missing BufferedInputStream I think we should turn it back to draft. - PR: https://git.openjdk.java.net/jdk/pull/6935
