Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 15:09:27 GMT, Roger Riggs wrote: >>> I recommend moving `nr` declaration from the beginning of the method to >>> where it's actually used (here) >> >> @liach done. > > Sorry, I misunderstood your earlier comment: "Sounds reasonable and applied" > as concurring with not re-allocating to avoid the overhead of gc and wasting > the smaller buffer. > The code can be harder to understand because `size` and `skipBuffer.length` > may be different. @RogerRiggs Rethink of this, I kind of prefer the soft reference plan @liach shown > The code can be harder to understand because `size` and `skipBuffer.length` > may be different. yes... - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
If you consider doing benchmarks in detail maybe consider a static buffer, too? (Especially if it can be used in multiple implementations?) Gruss Bernd -- http://bernd.eckenfels.net Von: core-libs-dev im Auftrag von XenoAmess Gesendet: Wednesday, April 13, 2022 11:58:06 PM An: core-libs-dev@openjdk.java.net Betreff: Re: RFR: 8284638: store skip buffers in InputStream Object [v2] On Wed, 13 Apr 2022 16:02:10 GMT, Alan Bateman wrote: >>> @AlanBateman You are correct about this. But I wonder if this be a problem, >>> why Reader class can afford store a skip buffer for each Reader. >>> >>> Is there anything different in the situations about skipBuffer in Reader >>> and InputStream? >> >> Maybe the skip buffer in Reader should be looked at too, esp. as it >> couldpotentially grow to 16k bytes. The concern with changing >> InputStream.skip is that there may be a lot more input streams than readers >> in use, esp. if there is an input stream for every socket connection. > >> @AlanBateman Is the concern about holding more memory sufficient to retain >> the buffer using a SoftReference so it can be freed eagerly? >> These buffers are never read from, so are quite a waste, but at least they >> are only used when >> the underlying stream overrides skip. > > Maybe but I think it needs some benchmarks to know if caching a byte[] will > help or just bloat memory. If the InputStream is to a file or socket then > maybe skip is dominated by the I/O rather than the array allocation and > zero'ing. @AlanBateman jmh test added - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 16:02:10 GMT, Alan Bateman wrote: >>> @AlanBateman You are correct about this. But I wonder if this be a problem, >>> why Reader class can afford store a skip buffer for each Reader. >>> >>> Is there anything different in the situations about skipBuffer in Reader >>> and InputStream? >> >> Maybe the skip buffer in Reader should be looked at too, esp. as it >> couldpotentially grow to 16k bytes. The concern with changing >> InputStream.skip is that there may be a lot more input streams than readers >> in use, esp. if there is an input stream for every socket connection. > >> @AlanBateman Is the concern about holding more memory sufficient to retain >> the buffer using a SoftReference so it can be freed eagerly? >> These buffers are never read from, so are quite a waste, but at least they >> are only used when >> the underlying stream overrides skip. > > Maybe but I think it needs some benchmarks to know if caching a byte[] will > help or just bloat memory. If the InputStream is to a file or socket then > maybe skip is dominated by the I/O rather than the array allocation and > zero'ing. @AlanBateman jmh test added - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 15:03:22 GMT, Alan Bateman wrote: >> This change may be problematic for servers with a large number connections >> and an input stream for each connection. It could add up to 2k to the >> footprint of each connection when skip is used. > >> @AlanBateman You are correct about this. But I wonder if this be a problem, >> why Reader class can afford store a skip buffer for each Reader. >> >> Is there anything different in the situations about skipBuffer in Reader and >> InputStream? > > Maybe the skip buffer in Reader should be looked at too, esp. as it > couldpotentially grow to 16k bytes. The concern with changing > InputStream.skip is that there may be a lot more input streams than readers > in use, esp. if there is an input stream for every socket connection. > @AlanBateman Is the concern about holding more memory sufficient to retain > the buffer using a SoftReference so it can be freed eagerly? > These buffers are never read from, so are quite a waste, but at least they > are only used when > the underlying stream overrides skip. Maybe but I think it needs some benchmarks to know if caching a byte[] will help or just bloat memory. If the InputStream is to a file or socket then maybe skip is dominated by the I/O rather than the array allocation and zero'ing. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 15:03:22 GMT, Alan Bateman wrote: >> This change may be problematic for servers with a large number connections >> and an input stream for each connection. It could add up to 2k to the >> footprint of each connection when skip is used. > >> @AlanBateman You are correct about this. But I wonder if this be a problem, >> why Reader class can afford store a skip buffer for each Reader. >> >> Is there anything different in the situations about skipBuffer in Reader and >> InputStream? > > Maybe the skip buffer in Reader should be looked at too, esp. as it > couldpotentially grow to 16k bytes. The concern with changing > InputStream.skip is that there may be a lot more input streams than readers > in use, esp. if there is an input stream for every socket connection. @AlanBateman Is the concern about holding more memory sufficient to retain the buffer using a SoftReference so it can be freed eagerly? These buffers are never read from, so are quite a waste, but at least they are only used when the underlying stream overrides skip. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 15:03:22 GMT, Alan Bateman wrote: > > @AlanBateman You are correct about this. But I wonder if this be a problem, > > why Reader class can afford store a skip buffer for each Reader. > > Is there anything different in the situations about skipBuffer in Reader > > and InputStream? > > Maybe the skip buffer in Reader should be looked at too, esp. as it > couldpotentially grow to 16k bytes. The concern with changing > InputStream.skip is that there may be a lot more input streams than readers > in use, esp. if there is an input stream for every socket connection. Making a drop of storing skipBuffer in Reader sonds another solution that acceptable to me, as it makes the handling strategy same in Reader and InputStream classes. > The concern with changing InputStream.skip is that there may be a lot more > input streams than readers in use I don't think use frequency can influence this. In other words, I still think, if there be no special acceptable reason, we shall make the skipBuffer handle strategy same in Reader and InputStream classes. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:52:20 GMT, XenoAmess wrote: >> It indeed is reallocated when the existing one is not large enough. > >> I recommend moving `nr` declaration from the beginning of the method to >> where it's actually used (here) > > @liach done. Sorry, I misunderstood your earlier comment: "Sounds reasonable and applied" as concurring with not re-allocating to avoid the overhead of gc and wasting the smaller buffer. The code can be harder to understand because `size` and `skipBuffer.length` may be different. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:36:17 GMT, Alan Bateman wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add MIN_SKIP_BUFFER_SIZE > > This change may be problematic for servers with a large number connections > and an input stream for each connection. It could add up to 2k to the > footprint of each connection when skip is used. > @AlanBateman You are correct about this. But I wonder if this be a problem, > why Reader class can afford store a skip buffer for each Reader. > > Is there anything different in the situations about skipBuffer in Reader and > InputStream? Maybe the skip buffer in Reader should be looked at too, esp. as it couldpotentially grow to 16k bytes. The concern with changing InputStream.skip is that there may be a lot more input streams than readers in use, esp. if there is an input stream for every socket connection. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:51:34 GMT, liach wrote:
>> The check for `skipBuffer.length < size` makes it appear that the buffer can
>> be re-allocated.
>> If it is allocated once then only the `skipBuffer == null` is needed.
>>
>> The code may be simpler if the 'size' variable is removed.
>>
>> byte[] skipBuffer = this.skipBuffer;
>> if (skipBuffer == null) {
>> this.skipBuffer = skipBuffer =
>> new byte[(remaining < MIN_SKIP_BUFFER_SIZE) ?
>> MIN_SKIP_BUFFER_SIZE : MAX_SKIP_BUFFER_SIZE];
>> }
>> while (remaining > 0) {
>> int nr = read(skipBuffer, 0, (int)Math.min(skipBuffer.length,
>> remaining));
>
> It indeed is reallocated when the existing one is not large enough.
> I recommend moving `nr` declaration from the beginning of the method to where
> it's actually used (here)
@liach done.
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:45:31 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/java/io/InputStream.java line 557:
>>
>>> 555:
>>> 556: while (remaining > 0) {
>>> 557: nr = read(skipBuffer, 0, (int)Math.min(size, remaining));
>>
>> I recommend moving `nr` declaration from the beginning of the method to
>> where it's actually used (here)
>
> The check for `skipBuffer.length < size` makes it appear that the buffer can
> be re-allocated.
> If it is allocated once then only the `skipBuffer == null` is needed.
>
> The code may be simpler if the 'size' variable is removed.
>
> byte[] skipBuffer = this.skipBuffer;
> if (skipBuffer == null) {
> this.skipBuffer = skipBuffer =
> new byte[(remaining < MIN_SKIP_BUFFER_SIZE) ?
> MIN_SKIP_BUFFER_SIZE : MAX_SKIP_BUFFER_SIZE];
> }
> while (remaining > 0) {
> int nr = read(skipBuffer, 0, (int)Math.min(skipBuffer.length,
> remaining));
It indeed is reallocated when the existing one is not large enough.
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Tue, 12 Apr 2022 23:13:26 GMT, liach wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add MIN_SKIP_BUFFER_SIZE
>
> src/java.base/share/classes/java/io/InputStream.java line 557:
>
>> 555:
>> 556: while (remaining > 0) {
>> 557: nr = read(skipBuffer, 0, (int)Math.min(size, remaining));
>
> I recommend moving `nr` declaration from the beginning of the method to where
> it's actually used (here)
The check for `skipBuffer.length < size` makes it appear that the buffer can be
re-allocated.
If it is allocated once then only the `skipBuffer == null` is needed.
The code may be simpler if the 'size' variable is removed.
byte[] skipBuffer = this.skipBuffer;
if (skipBuffer == null) {
this.skipBuffer = skipBuffer =
new byte[(remaining < MIN_SKIP_BUFFER_SIZE) ?
MIN_SKIP_BUFFER_SIZE : MAX_SKIP_BUFFER_SIZE];
}
while (remaining > 0) {
int nr = read(skipBuffer, 0, (int)Math.min(skipBuffer.length,
remaining));
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:36:17 GMT, Alan Bateman wrote: > This change may be problematic for servers with a large number connections > and an input stream for each connection. It could add up to 2k to the > footprint of each connection when skip is used. @AlanBateman You are correct about this. But I wonder if this be a problem, why Reader class can afford store a skip buffer for each Reader. Is there anything different in the situations about skipBuffer in Reader and InputStream? - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Tue, 12 Apr 2022 22:19:18 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > add MIN_SKIP_BUFFER_SIZE This change may be problematic for servers with a large number connections and an input stream for each connection. It could add up to 2k to the footprint of each connection when skip is used. This change may be problematic for servers with a large number connections and an input stream for each connection. It could add up to 2k to the footprint of each connection when skip is used. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Tue, 12 Apr 2022 22:15:22 GMT, XenoAmess wrote: > What subclasses of InputStream in the JDK do not override skip(n)? >From a peek, the majority of subclasses do not override `skip(long)`. Most >overrides are delegations or optimizations for random access structures; but >there are some that create their custom local variable skip buffers, usually >of size 512 or minimum of 512 and the skip size. These custom skip buffer ones >IMO should have their overrides removed. > Most sequential streams are open for a relatively short period of time, the > lifetime of the > memory for the buffer won't change the memory usage enough to notice. True, and with good use of try-with-resources, these instance fields' array allocations shouldn't be too much of a problem compared to allocation in every skip(long) call. > If the concern is about tying up memory then allocate the buffer once and don't resize it. Each resize consumes extra memory and gc cycles to reclaim the last buffer. > Use the requested size but at least nnn and at most MAX_SKIP_BUFFER_SIZE. Shouldn't be too problematic, as most skip usages in JDK as I see are skipping small number of bytes like 2 or 4, or like skipping over attributes of Java class files. A minimum skip buffer size isn't that helpful, as I don't think we often see skip calls with slowly incremental sizes. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Tue, 12 Apr 2022 22:19:18 GMT, XenoAmess wrote:
>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add MIN_SKIP_BUFFER_SIZE
src/java.base/share/classes/java/io/InputStream.java line 557:
> 555:
> 556: while (remaining > 0) {
> 557: nr = read(skipBuffer, 0, (int)Math.min(size, remaining));
I recommend moving `nr` declaration from the beginning of the method to where
it's actually used (here)
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Tue, 12 Apr 2022 20:39:52 GMT, Roger Riggs wrote: > Without specific information about use cases, there isn't enough information > to craft a good algorithm/solution and simplicity is preferred. The > MAX_SKIP_BUFFER_SIZE is 2048 (not 8192). > > What subclasses of InputStream in the JDK do not override skip(n)? Most > sequential streams are open for a relatively short period of time, the > lifetime of the memory for the buffer won't change the memory usage enough to > notice. > > If the concern is about tying up memory then allocate the buffer once and > don't resize it. Each resize consumes extra memory and gc cycles to reclaim > the last buffer. Use the requested size but at least nnn and at most > MAX_SKIP_BUFFER_SIZE. @RogerRiggs Sounds reasonable and applied. Should we change the implementation in Reader class as well? - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
> @jmehrens what about this then? > I think it safe now(actually this mechanism is learned from Reader) XenoAmess has updated the pull request incrementally with one additional commit since the last revision: add MIN_SKIP_BUFFER_SIZE - Changes: - all: https://git.openjdk.java.net/jdk/pull/5872/files - new: https://git.openjdk.java.net/jdk/pull/5872/files/e29f7483..81e9ca49 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5872&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5872&range=00-01 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5872.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5872/head:pull/5872 PR: https://git.openjdk.java.net/jdk/pull/5872
