RFR: 4926314: Optimize Reader.read(CharBuffer)

classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 4926314: Optimize Reader.read(CharBuffer)

Philippe Marschall-2
Implement three optimiztations for Reader.read(CharBuffer)

* Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
* Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
* Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
* Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.

-------------

Commit messages:
 - 4926314: Optimize Reader.read(CharBuffer)

Changes: https://git.openjdk.java.net/jdk/pull/1915/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-4926314
  Stats: 101 lines in 3 files changed: 68 ins; 8 del; 25 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1915.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1915/head:pull/1915

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer)

Philippe Marschall-2
On Thu, 31 Dec 2020 10:10:56 GMT, Philippe Marschall <[hidden email]> wrote:

> Implement three optimiztations for Reader.read(CharBuffer)
>
> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.

A couple of implementation notes:

Reader#read(CharBuffer)

on-heap case

I introduced a dedicated path for the on-heap case and directly read into the backing array. This completely avoids the intermediate allocation and copy. Compared to the initial proposal the buffer position is updated. This has to be done because we bypass the buffer and directly read into the array. This also handles the case that #read returns -1.

I am using a c-style array declaration because the rest of the class uses it.

off-heap case

I limit the intermadiate allocation to TRANSFER_BUFFER_SIZE. In order to reduce the total intermediate allocation we now call #read multiple times until the buffer is full or EOF is reached. This changes the behavior slightly as now possibly more data is read. However this should contribute to reduce the overall intermediate allocations.

A lock is acquired to keep the the read atomic. This is consistent with #skip which also holds a lock over multiple #read calls. This is inconsistent with #transferTo which does not acquire a lock over multiple #read calls.

The implementation took inspiration from java.io.InputStream#readNBytes(int).

InputStreamReader#read(CharBuffer)

Since StreamDecoder is a Reader as well we can simply delegate.

StreamDecoder#read(CharBuffer)

Interestingly this was not implemented even though StreamDecoder internally works on NIO buffers.

on-heap case

We see a performance improvement and the elimination of all intermediate allocation.

StreamDecoder#read(char[], int, int) and InputStreamReader#read(char[], int, int) may get a small improvement as we no longer #slice the buffer.

off-heap case

We see the elimination of all intermediate allocation but a performance penalty because we hit the slow path in #decodeLoop. There is a trade-off here, we could improve the performance to previous levels by introducing intermediate allocation again. I don't know how much the users of off-heap buffers care about introducing intermediate allocation vs maximum throughput.

I was struggling to come up with microbenchmarks because the built in InputStream and Reader implementations are finite but JMH calls the benchmark methods repeatably. I ended up implementing custom infinite InputStream and Reader implementations. The code can be found there:

https://github.com/marschall/reader-benchmarks/tree/master/src/main/java/com/github/marschall/readerbenchmarks

An Excel with charts can be found here:

https://github.com/marschall/reader-benchmarks/raw/master/src/main/resources/4926314.xlsx

I looked for java.io.Reader#read(CharBuffer) users in the JDK and only found java.util.Scanner#readInput(). I wrote a microbenchmark for this but only found minuscule improvements due to regex dominating the runtime.

There seem to be no direct Reader tests in the tier1 suite, the initial code was wrong because I forgot to update the buffer code position but I only found out because some Scanner tests failed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v2]

Philippe Marschall-2
In reply to this post by Philippe Marschall-2
> Implement three optimiztations for Reader.read(CharBuffer)
>
> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.

Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:

  Fix off-heap code path
 
  The off-heap code path was missing a buffer.put

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1915/files
  - new: https://git.openjdk.java.net/jdk/pull/1915/files/967b314a..a88cd931

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=00-01

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1915.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1915/head:pull/1915

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer)

Brian Burkhalter-3
In reply to this post by Philippe Marschall-2
On Thu, 31 Dec 2020 10:11:58 GMT, Philippe Marschall <[hidden email]> wrote:

>> Implement three optimiztations for Reader.read(CharBuffer)
>>
>> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
>> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
>> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
>> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.
>
> A couple of implementation notes:
>
> Reader#read(CharBuffer)
>
> on-heap case
>
> I introduced a dedicated path for the on-heap case and directly read into the backing array. This completely avoids the intermediate allocation and copy. Compared to the initial proposal the buffer position is updated. This has to be done because we bypass the buffer and directly read into the array. This also handles the case that #read returns -1.
>
> I am using a c-style array declaration because the rest of the class uses it.
>
> off-heap case
>
> I limit the intermadiate allocation to TRANSFER_BUFFER_SIZE. In order to reduce the total intermediate allocation we now call #read multiple times until the buffer is full or EOF is reached. This changes the behavior slightly as now possibly more data is read. However this should contribute to reduce the overall intermediate allocations.
>
> A lock is acquired to keep the the read atomic. This is consistent with #skip which also holds a lock over multiple #read calls. This is inconsistent with #transferTo which does not acquire a lock over multiple #read calls.
>
> The implementation took inspiration from java.io.InputStream#readNBytes(int).
>
> InputStreamReader#read(CharBuffer)
>
> Since StreamDecoder is a Reader as well we can simply delegate.
>
> StreamDecoder#read(CharBuffer)
>
> Interestingly this was not implemented even though StreamDecoder internally works on NIO buffers.
>
> on-heap case
>
> We see a performance improvement and the elimination of all intermediate allocation.
>
> StreamDecoder#read(char[], int, int) and InputStreamReader#read(char[], int, int) may get a small improvement as we no longer #slice the buffer.
>
> off-heap case
>
> We see the elimination of all intermediate allocation but a performance penalty because we hit the slow path in #decodeLoop. There is a trade-off here, we could improve the performance to previous levels by introducing intermediate allocation again. I don't know how much the users of off-heap buffers care about introducing intermediate allocation vs maximum throughput.
>
> I was struggling to come up with microbenchmarks because the built in InputStream and Reader implementations are finite but JMH calls the benchmark methods repeatably. I ended up implementing custom infinite InputStream and Reader implementations. The code can be found there:
>
> https://github.com/marschall/reader-benchmarks/tree/master/src/main/java/com/github/marschall/readerbenchmarks
>
> An Excel with charts can be found here:
>
> https://github.com/marschall/reader-benchmarks/raw/master/src/main/resources/4926314.xlsx
>
> I looked for java.io.Reader#read(CharBuffer) users in the JDK and only found java.util.Scanner#readInput(). I wrote a microbenchmark for this but only found minuscule improvements due to regex dominating the runtime.
>
> There seem to be no direct Reader tests in the tier1 suite, the initial code was wrong because I forgot to update the buffer code position but I only found out because some Scanner tests failed.

I changed the JBS issue summary to match the title of this PR so that integration blocker is removed.

How does the performance of `InputStreamReader.read(CharBuffer)` compare for the case where only the change to `Reader` is made versus if all your proposed changes are applied?

Some kind of specific test should likely be included.

Note that the more recent copyright year is now 2021.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]

Philippe Marschall-2
In reply to this post by Philippe Marschall-2
> Implement three optimiztations for Reader.read(CharBuffer)
>
> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.

Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:

  Update copyright years

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1915/files
  - new: https://git.openjdk.java.net/jdk/pull/1915/files/a88cd931..8d405587

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=01-02

  Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1915.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1915/head:pull/1915

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer)

Philippe Marschall
In reply to this post by Brian Burkhalter-3


On 05.01.21 01:53, Brian Burkhalter wrote:
> ...
>
> I changed the JBS issue summary to match the title of this PR so that integration blocker is removed.

Thank you.

> How does the performance of `InputStreamReader.read(CharBuffer)` compare for the case where only the change to `Reader` is made versus if all your proposed changes are applied?

I can look into this, this will take a moment. I guess it would also
make sense to have a comparison with a version that does intermediate
heap allocation for off-heap buffers in InputStreamReader#read(CharBuffer).

> Some kind of specific test should likely be included.

Sure. The existing tests in this area seem to be #main-based. Would you
prefer #main based tests or TestNG tests?

> Note that the more recent copyright year is now 2021.

Indeed it is, fixed.

Cheers
Philippe
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer)

Brian Burkhalter-2

> On Jan 5, 2021, at 9:53 AM, Philippe Marschall <[hidden email]> wrote:
>
>> How does the performance of `InputStreamReader.read(CharBuffer)` compare for the case where only the change to `Reader` is made versus if all your proposed changes are applied?
>
> I can look into this, this will take a moment. I guess it would also
> make sense to have a comparison with a version that does intermediate
> heap allocation for off-heap buffers in InputStreamReader#read(CharBuffer).

If you like. I was just wondering whether the change to Reader.read(CharBuffer) would be enough.

>> Some kind of specific test should likely be included.
>
> Sure. The existing tests in this area seem to be #main-based. Would you
> prefer #main based tests or TestNG tests?

For new tests we seem to be preferring Tests NG.

>> Note that the more recent copyright year is now 2021.
>
> Indeed it is, fixed.

Thanks.

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]

Philippe Marschall-2
In reply to this post by Philippe Marschall-2
> Implement three optimiztations for Reader.read(CharBuffer)
>
> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.

Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:

  Add unit tests
 
  - add unit test for Reader#read(CharBuffer)
  - add unit test for InputStreamReader#reader(CharBuffer)
  - test with both on-heap and off-heap buffers

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1915/files
  - new: https://git.openjdk.java.net/jdk/pull/1915/files/8d405587..d247b637

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=02-03

  Stats: 170 lines in 2 files changed: 170 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1915.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1915/head:pull/1915

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]

Brett Okken-2
On Sat, 9 Jan 2021 23:06:22 GMT, Philippe Marschall <[hidden email]> wrote:

>> Implement three optimiztations for Reader.read(CharBuffer)
>>
>> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
>> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
>> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
>> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.
>
> Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add unit tests
>  
>   - add unit test for Reader#read(CharBuffer)
>   - add unit test for InputStreamReader#reader(CharBuffer)
>   - test with both on-heap and off-heap buffers

src/java.base/share/classes/java/io/Reader.java line 198:

> 196:         } else {
> 197:             int remaining = target.remaining();
> 198:             char cbuf[] = new char[Math.min(remaining, TRANSFER_BUFFER_SIZE)];

Would there be value in making this a (lazily created) member variable? That would allow a single instance to be reused. It seems likely that, if one call is made with a direct CharBuffer, subsequent calls will also be made with direct instances (probably same instance?).

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer)

Philippe Marschall
In reply to this post by Brian Burkhalter-3


On 05.01.21 01:53, Brian Burkhalter wrote:
> On Thu, 31 Dec 2020 10:11:58 GMT, Philippe Marschall <[hidden email]> wrote:
> ...
>
> How does the performance of `InputStreamReader.read(CharBuffer)` compare for the case where only the change to `Reader` is made versus if all your proposed changes are applied?

I left the delegating one in InputStreamReader in but removed all
changes in StreamDecoder. Performance looks pretty good:

on-heap CharBuffer
- Throughput is a slightly lower than with the changes but close and
still better than before.
- Allocation rate is drastically reduced and comparable to the results
with the changes except for small buffer sizes (128 bytes).

off-heap CharBuffer
- Relative throughput depends on the buffer size. For small buffers (128
bytes) throughput is a bit lower than master but increases with buffer
size. For 1 KB buffers it is about even, for 1 MB buffers it is better.
Throughput is a lot better than with the StreamDecoder changes without
intermediate allocation, there we lose about one half to two thirds of
throughput.
- Allocation rate stays high (1.5 - 3 GB/s) and only drops with large
buffer sizes (1 MB) to 20 - 30 MB/s. The StreamDecoder changes cause the
allocation rate to drop to almost zero.

To be honest backing out of the StreamDecoder changes looks like a good
comprise to me to reduce the risk while still improving throughput and
reducing allocation rate, especially in the on-heap path.

Looking a bit further I wonder if CharArrayReader and StringReader
should implement #read(CharBuffer) as well and call CharBuffer#put
directly. And maybe even #transferTo(Writer).


Cheers
Philippe
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]

Philippe Marschall-2
In reply to this post by Brett Okken-2
On Sun, 10 Jan 2021 01:59:18 GMT, Brett Okken <[hidden email]> wrote:

>> Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Add unit tests
>>  
>>   - add unit test for Reader#read(CharBuffer)
>>   - add unit test for InputStreamReader#reader(CharBuffer)
>>   - test with both on-heap and off-heap buffers
>
> src/java.base/share/classes/java/io/Reader.java line 198:
>
>> 196:         } else {
>> 197:             int remaining = target.remaining();
>> 198:             char cbuf[] = new char[Math.min(remaining, TRANSFER_BUFFER_SIZE)];
>
> Would there be value in making this a (lazily created) member variable? That would allow a single instance to be reused. It seems likely that, if one call is made with a direct CharBuffer, subsequent calls will also be made with direct instances (probably same instance?).

I am not sure. It would help to reduce the allocation rate when reading a large amount of data into a small direct CharBuffer. I don't know how common that is. We would introduce an instance variable for one path in one method.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]

Peter Levart-3
In reply to this post by Philippe Marschall-2
On Sat, 9 Jan 2021 23:06:22 GMT, Philippe Marschall <[hidden email]> wrote:

>> Implement three optimiztations for Reader.read(CharBuffer)
>>
>> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
>> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
>> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
>> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.
>
> Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add unit tests
>  
>   - add unit test for Reader#read(CharBuffer)
>   - add unit test for InputStreamReader#reader(CharBuffer)
>   - test with both on-heap and off-heap buffers

src/java.base/share/classes/java/io/Reader.java line 207:

> 205:                         target.put(cbuf, 0, n);
> 206:                         nread += n;
> 207:                         remaining -= n;

Wouldn't there be a possibility for target.put(cbuf, 0, n) to throw BufferOverflowException ?
For example:
- there's room (remaining) for TRANSFER_BUFFER_SIZE + 1 characters in target
- cbuff is sized to TRANSFER_BUFFER_SIZE
- 1st iteration of do loop transfers TRANSFER_BUFFER_SIZE charasters (remaining == 1)
- 2nd iteration reads > 1 (up to TRANSFER_BUFFER_SIZE) characters
- target.put throws BufferOverflowException

You have to limit the amount read in each iteration to be Math.min(remaining, cbuf.length)

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]

Philippe Marschall-2
On Mon, 18 Jan 2021 07:47:30 GMT, Peter Levart <[hidden email]> wrote:

>> Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Add unit tests
>>  
>>   - add unit test for Reader#read(CharBuffer)
>>   - add unit test for InputStreamReader#reader(CharBuffer)
>>   - test with both on-heap and off-heap buffers
>
> src/java.base/share/classes/java/io/Reader.java line 207:
>
>> 205:                         target.put(cbuf, 0, n);
>> 206:                         nread += n;
>> 207:                         remaining -= n;
>
> Wouldn't there be a possibility for target.put(cbuf, 0, n) to throw BufferOverflowException ?
> For example:
> - there's room (remaining) for TRANSFER_BUFFER_SIZE + 1 characters in target
> - cbuff is sized to TRANSFER_BUFFER_SIZE
> - 1st iteration of do loop transfers TRANSFER_BUFFER_SIZE charasters (remaining == 1)
> - 2nd iteration reads > 1 (up to TRANSFER_BUFFER_SIZE) characters
> - target.put throws BufferOverflowException
>
> You have to limit the amount read in each iteration to be Math.min(remaining, cbuf.length)

You're correct. I need to expand the unit tests.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v5]

Philippe Marschall-2
In reply to this post by Philippe Marschall-2
> Implement three optimiztations for Reader.read(CharBuffer)
>
> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.

Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:

  Limit amount read to avoid BufferOverflowException
 
  - limit the amount read
  - add tests

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1915/files
  - new: https://git.openjdk.java.net/jdk/pull/1915/files/d247b637..a8531c1b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=03-04

  Stats: 79 lines in 2 files changed: 62 ins; 2 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1915.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1915/head:pull/1915

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]

Philippe Marschall-2
In reply to this post by Philippe Marschall-2
On Tue, 19 Jan 2021 07:22:49 GMT, Philippe Marschall <[hidden email]> wrote:

>> src/java.base/share/classes/java/io/Reader.java line 207:
>>
>>> 205:                         target.put(cbuf, 0, n);
>>> 206:                         nread += n;
>>> 207:                         remaining -= n;
>>
>> Wouldn't there be a possibility for target.put(cbuf, 0, n) to throw BufferOverflowException ?
>> For example:
>> - there's room (remaining) for TRANSFER_BUFFER_SIZE + 1 characters in target
>> - cbuff is sized to TRANSFER_BUFFER_SIZE
>> - 1st iteration of do loop transfers TRANSFER_BUFFER_SIZE charasters (remaining == 1)
>> - 2nd iteration reads > 1 (up to TRANSFER_BUFFER_SIZE) characters
>> - target.put throws BufferOverflowException
>>
>> You have to limit the amount read in each iteration to be Math.min(remaining, cbuf.length)
>
> You're correct. I need to expand the unit tests.

Fixed

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer)

Philippe Marschall
In reply to this post by Philippe Marschall


On 17.01.21 18:48, Philippe Marschall wrote:
> ...
>
> To be honest backing out of the StreamDecoder changes looks like a good
> comprise to me to reduce the risk while still improving throughput and
> reducing allocation rate, especially in the on-heap path.

I gave it some more thought and propose to back out of the StreamDecoder
changes. While that leaves some optimization potential unused it keeps
the patch smaller, the risk lower and avoids any throughput regressions.

> Looking a bit further I wonder if CharArrayReader and StringReader
> should implement #read(CharBuffer) as well and call CharBuffer#put
> directly. And maybe even #transferTo(Writer).

I did have a look at this [1] for coders LATIN1(0) and UTF16(1) as well
as 128 and 1024 char sized readers for both on- and off-heap buffers.

CharArrayReader#read(CharBuffer)

on-heap
Higher throughput than master but slightly lower throughput than the
current PR. I donĀ“ t know why that is, maybe the additional checks in
CharBuffer show up here.

off-heap
Higher throughput than master or the PR, all intermediate allocation is
gone.

StringReader#read(CharBuffer)

on-heap
Higher throughput than master but slightly lower throughput than the
current PR. I donĀ“ t know why that is, maybe the additional checks in
CharBuffer show up here. The situation is similar to CharArrayReader.


off-heap
Lower throughput but all intermediate allocation is gone. What shows up
here is the lack of an optimized #put(String) method for off-heap
CharBuffers like was done in JDK-8011135 for on-heap buffers.

Based on this is propose to add CharArrayReader#read(CharBuffer),
assuming it is still in the scope of the bug. I wouldn't add
StringReader#read(CharBuffer) due to the throughput regression. I think
#transferTo(Writer) would be out of the scope of the bug. Is that ok?

  [1]
https://github.com/marschall/reader-benchmarks/blob/master/src/main/java/com/github/marschall/readerbenchmarks/CharsequenceReaderBenchmarks.java

Cheers
Philippe
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]

Brian Burkhalter-3
In reply to this post by Philippe Marschall-2
On Tue, 5 Jan 2021 17:44:20 GMT, Philippe Marschall <[hidden email]> wrote:

>> Implement three optimiztations for Reader.read(CharBuffer)
>>
>> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
>> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
>> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
>> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.
>
> Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:
>
>   Update copyright years

src/java.base/share/classes/java/io/Reader.java line 198:

> 196:         } else {
> 197:             int remaining = target.remaining();
> 198:             char cbuf[] = new char[Math.min(remaining, TRANSFER_BUFFER_SIZE)];

As `cbuf` for the off-heap case is used in a synchronized block, is there the opportunity for some sort of cached array here and would it help?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v5]

Brian Burkhalter-3
In reply to this post by Philippe Marschall-2
On Tue, 26 Jan 2021 18:22:02 GMT, Philippe Marschall <[hidden email]> wrote:

>> Implement three optimiztations for Reader.read(CharBuffer)
>>
>> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
>> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
>> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
>> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.
>
> Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:
>
>   Limit amount read to avoid BufferOverflowException
>  
>   - limit the amount read
>   - add tests

src/java.base/share/classes/java/io/Reader.java line 194:

> 192:             nread = this.read(cbuf, off, len);
> 193:             if (nread > 0)
> 194:                 target.position(target.position() + nread);

As `target` is mutable, I think you would do better to change lines 189-194 to something like:
            char cbuf[] = target.array();
            int pos = target.position();
            int rem = target.limit() - pos;
            if (rem <= 0)
                return -1;
            int off = target.arrayOffset() + pos;
            nread = this.read(cbuf, off, rem);
            if (nread > 0)
                target.position(pos + nread);

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]

Philippe Marschall-2
In reply to this post by Brian Burkhalter-3
On Tue, 5 Jan 2021 18:10:52 GMT, Brian Burkhalter <[hidden email]> wrote:

>> Philippe Marschall has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Update copyright years
>
> src/java.base/share/classes/java/io/Reader.java line 198:
>
>> 196:         } else {
>> 197:             int remaining = target.remaining();
>> 198:             char cbuf[] = new char[Math.min(remaining, TRANSFER_BUFFER_SIZE)];
>
> As `cbuf` for the off-heap case is used in a synchronized block, is there the opportunity for some sort of cached array here and would it help?

That would be possible. It would help in cases where a large Reader is read into one or several relatively small off-heap CharBuffers, requiring multiple #read calls. This can only be done when the caller is able to work with only a partial input. I don't know how common this case is.

We could re-purpose #skipBuffer, it has the same maximum size (8192) but determined by a different constant (#maxSkipBufferSize instead of #TRANSFER_BUFFER_SIZE). That would likely require it to be renamed and maybe we should even remove #maxSkipBufferSize. We could also do the reallocation and growing similar as is currently done in #skip.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1915
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v6]

Philippe Marschall-2
In reply to this post by Philippe Marschall-2
> Implement three optimiztations for Reader.read(CharBuffer)
>
> * Add a code path for heap buffers in Reader#read to use the backing array instead of allocating a new one.
> * Change the code path for direct buffers in Reader#read to limit the intermediate allocation to `TRANSFER_BUFFER_SIZE`.
> * Implement `InputStreamReader#read(CharBuffer)` and delegate to `StreamDecoder`.
> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation.

Philippe Marschall has updated the pull request incrementally with four additional commits since the last revision:

 - Update copyright year
 - Implement review comment
 - Revert StreamDecoder changes
 - Implement CharArrayReader#read(CharBuffer)

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1915/files
  - new: https://git.openjdk.java.net/jdk/pull/1915/files/a8531c1b..08948f93

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=04-05

  Stats: 105 lines in 3 files changed: 38 ins; 50 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1915.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1915/head:pull/1915

PR: https://git.openjdk.java.net/jdk/pull/1915
12