On Thu, 3 Jun 2021 17:29:14 GMT, Markus KARG
<github.com+1701815+mk...@openjdk.org> 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
