RFR: 4926314: Optimize Reader.read(CharBuffer)

classic Classic list List threaded Threaded
43 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Philippe Marschall-2
On Fri, 5 Feb 2021 22:08:17 GMT, Brian Burkhalter <[hidden email]> wrote:

>> 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);

Done

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

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]

Brian Burkhalter-3
In reply to this post by Philippe Marschall-2
On Tue, 9 Feb 2021 14:08:10 GMT, Philippe Marschall <[hidden email]> wrote:

>> 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.

Perhaps a static final `WORK_BUFFER_SIZE` could be added with value 8192 and `maxSkipBufferSize` and `TRANSFER_BUFFER_SIZE` replaced with that? Then `skipBuffer` could be renamed to `workBuffer` and used in both `read(CharBuffer)` and `skip(long)`. That shouldn't be a problem as both uses are in synchronized blocks. Also I suggest putting the declaration of `workBuffer` just below that of `lock` instead of lower down the file where `skipBuffer` is.

Lastly you mentioned C-style array declarations like `char buf[]`. As there are only four of these in the file it might be good to just go ahead and change them, I don't think that adds much noise or risk.

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

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

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

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 two additional commits since the last revision:

 - Replace c-style array declarations
 - Share work buffer between #skip and #read

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

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

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

  Stats: 28 lines in 1 file changed: 8 ins; 6 del; 14 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) [v3]

Philippe Marschall-2
In reply to this post by Brian Burkhalter-3
On Wed, 10 Feb 2021 21:58:02 GMT, Brian Burkhalter <[hidden email]> wrote:

>> 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.
>
> Perhaps a static final `WORK_BUFFER_SIZE` could be added with value 8192 and `maxSkipBufferSize` and `TRANSFER_BUFFER_SIZE` replaced with that? Then `skipBuffer` could be renamed to `workBuffer` and used in both `read(CharBuffer)` and `skip(long)`. That shouldn't be a problem as both uses are in synchronized blocks. Also I suggest putting the declaration of `workBuffer` just below that of `lock` instead of lower down the file where `skipBuffer` is.
>
> Lastly you mentioned C-style array declarations like `char buf[]`. As there are only four of these in the file it might be good to just go ahead and change them, I don't think that adds much noise or risk.

Done. I left #transferTo(Writer) untouched for now. Firstly it is not already behind a synchronized. Secondly it writes so there is no need for repeated calls.

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

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 Brian Burkhalter-3
On Tue, 5 Jan 2021 00:48:54 GMT, Brian Burkhalter <[hidden email]> wrote:

>> 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 off-heap 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.

I think the implementation changes here look good. I don't know however whether there is enough coverage in the tests. These should verify that the `Reader`, `CharArrayReader`, and `InputStreamReader` implementations of `read(CharBuffer)` are accurate. If there is already sufficient coverage in the tests in `test/jdk/java/io` then that is good enough and nothing need be added.

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

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

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

Brian Burkhalter-3
In reply to this post by Philippe Marschall-2
On Fri, 12 Feb 2021 09:18:10 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 two additional commits since the last revision:
>
>  - Replace c-style array declarations
>  - Share work buffer between #skip and #read

test/jdk/java/io/InputStreamReader/ReadCharBuffer.java line 34:

> 32: import org.testng.annotations.Test;
> 33:
> 34: import java.io.*;

It's generally better not to use a wild card.

test/jdk/java/io/InputStreamReader/ReadCharBuffer.java line 73:

> 71:         }
> 72:
> 73:         buffer.clear();

I think `buffer.rewind()` would be more in keeping with the specification verbiage although there will be no practical effect here. Same comment applies below and in the other test `ReadCharBuffer`.

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

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

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

Alan Bateman-2
In reply to this post by Philippe Marschall-2
On Fri, 12 Feb 2021 09:18:10 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 two additional commits since the last revision:
>
>  - Replace c-style array declarations
>  - Share work buffer between #skip and #read

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

> 219:                     // if the last call to read returned -1 or the number of bytes
> 220:                     // requested have been read then break
> 221:                 } while (n >= 0 && remaining > 0);

The code for case that the char buffer has a backing array looks okay but I'm not sure about the direct buffer/other cases. One concern is that this is a read method, not a transferXXX method so we shouldn't be calling the underlying read several times. You'll see what I mean if you consider the scenario where you read < rem, then read again and the second read blocks or throws. I'l also concerned about "workBuffer" adding more per-stream footprint for cases where skip or read(CB) is used. Objects such as InputStreamReader are already a problem due to the underlying stream decoder.

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

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

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

Philippe Marschall-2
On Wed, 17 Feb 2021 15:37:11 GMT, Alan Bateman <[hidden email]> wrote:

>> Philippe Marschall has updated the pull request incrementally with two additional commits since the last revision:
>>
>>  - Replace c-style array declarations
>>  - Share work buffer between #skip and #read
>
> src/java.base/share/classes/java/io/Reader.java line 221:
>
>> 219:                     // if the last call to read returned -1 or the number of bytes
>> 220:                     // requested have been read then break
>> 221:                 } while (n >= 0 && remaining > 0);
>
> The code for case that the char buffer has a backing array looks okay but I'm not sure about the direct buffer/other cases. One concern is that this is a read method, not a transferXXX method so we shouldn't be calling the underlying read several times. You'll see what I mean if you consider the scenario where you read < rem, then read again and the second read blocks or throws. I'l also concerned about "workBuffer" adding more per-stream footprint for cases where skip or read(CB) is used. Objects such as InputStreamReader are already a problem due to the underlying stream decoder.

Right. So you propose to revert the off-heap path to the current master? That would be fine with me. The original bug and my motivation was only about the backing array case, the rest crept in. That would certainly keep the risk and impact lower.

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

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

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

Brian Burkhalter-3
In reply to this post by Alan Bateman-2
On Wed, 17 Feb 2021 15:37:11 GMT, Alan Bateman <[hidden email]> wrote:

>> Philippe Marschall has updated the pull request incrementally with two additional commits since the last revision:
>>
>>  - Replace c-style array declarations
>>  - Share work buffer between #skip and #read
>
> src/java.base/share/classes/java/io/Reader.java line 221:
>
>> 219:                     // if the last call to read returned -1 or the number of bytes
>> 220:                     // requested have been read then break
>> 221:                 } while (n >= 0 && remaining > 0);
>
> The code for case that the char buffer has a backing array looks okay but I'm not sure about the direct buffer/other cases. One concern is that this is a read method, not a transferXXX method so we shouldn't be calling the underlying read several times. You'll see what I mean if you consider the scenario where you read < rem, then read again and the second read blocks or throws. I'l also concerned about "workBuffer" adding more per-stream footprint for cases where skip or read(CB) is used. Objects such as InputStreamReader are already a problem due to the underlying stream decoder.

I think that's what @AlanBateman intended. The `skip()` changes would revert also (I think) but the C-style array changes can stay. Thanks.

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

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

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

Alan Bateman-2
On Thu, 18 Feb 2021 20:35:10 GMT, Brian Burkhalter <[hidden email]> wrote:

>> src/java.base/share/classes/java/io/Reader.java line 221:
>>
>>> 219:                     // if the last call to read returned -1 or the number of bytes
>>> 220:                     // requested have been read then break
>>> 221:                 } while (n >= 0 && remaining > 0);
>>
>> The code for case that the char buffer has a backing array looks okay but I'm not sure about the direct buffer/other cases. One concern is that this is a read method, not a transferXXX method so we shouldn't be calling the underlying read several times. You'll see what I mean if you consider the scenario where you read < rem, then read again and the second read blocks or throws. I'l also concerned about "workBuffer" adding more per-stream footprint for cases where skip or read(CB) is used. Objects such as InputStreamReader are already a problem due to the underlying stream decoder.
>
> I think that's what @AlanBateman intended. The `skip()` changes would revert also (I think) but the C-style array changes can stay. Thanks.

Yes, let's keep bring it back to just eliminating the intermediate array when the buffer has a backing array. The other case that been examined separated if needed but we can't use the approach proposed in the current PR because it changes the semantics of read when there is a short-read followed by a blocking or exception throwing read.

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

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

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

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 three additional commits since the last revision:

 - Fix bug in CharArrayReader and add unit test
 - Clean up unit tests
 - Revert off-heap code path

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1915/files
  - new: https://git.openjdk.java.net/jdk/pull/1915/files/fc29f3e6..5fa832b1

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

  Stats: 134 lines in 5 files changed: 100 ins; 19 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) [v9]

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 with a new target base due to a merge or a rebase. The pull request now contains 15 commits:

 - Merge master
 - Fix bug in CharArrayReader and add unit test
 - Clean up unit tests
 - Revert off-heap code path
 - Replace c-style array declarations
 - Share work buffer between #skip and #read
 - Update copyright year
 - Implement review comment
 - Revert StreamDecoder changes
 - Implement CharArrayReader#read(CharBuffer)
 - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d339320e...c4c859e0

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

Changes: https://git.openjdk.java.net/jdk/pull/1915/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=08
  Stats: 371 lines in 6 files changed: 361 ins; 0 del; 10 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) [v7]

Philippe Marschall-2
In reply to this post by Brian Burkhalter-3
On Tue, 16 Feb 2021 23:50:30 GMT, Brian Burkhalter <[hidden email]> wrote:

>> Philippe Marschall has updated the pull request incrementally with two additional commits since the last revision:
>>
>>  - Replace c-style array declarations
>>  - Share work buffer between #skip and #read
>
> test/jdk/java/io/InputStreamReader/ReadCharBuffer.java line 34:
>
>> 32: import org.testng.annotations.Test;
>> 33:
>> 34: import java.io.*;
>
> It's generally better not to use a wild card.

Done

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

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

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

Philippe Marschall-2
In reply to this post by Brian Burkhalter-3
On Tue, 16 Feb 2021 23:52:09 GMT, Brian Burkhalter <[hidden email]> wrote:

>> Philippe Marschall has updated the pull request incrementally with two additional commits since the last revision:
>>
>>  - Replace c-style array declarations
>>  - Share work buffer between #skip and #read
>
> test/jdk/java/io/InputStreamReader/ReadCharBuffer.java line 73:
>
>> 71:         }
>> 72:
>> 73:         buffer.clear();
>
> I think `buffer.rewind()` would be more in keeping with the specification verbiage although there will be no practical effect here. Same comment applies below and in the other test `ReadCharBuffer`.

`buffer.rewind()` would not work in this specific case as it does not reset the limit. I want to assert the entire buffers content to make sure the method didn't accidentally write where it shouldn't have. Therefore limit needs to be set to capacity.

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

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

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

Philippe Marschall-2
In reply to this post by Alan Bateman-2
On Fri, 19 Feb 2021 07:34:57 GMT, Alan Bateman <[hidden email]> wrote:

>> I think that's what @AlanBateman intended. The `skip()` changes would revert also (I think) but the C-style array changes can stay. Thanks.
>
> Yes, let's bring it back to just eliminating the intermediate array when the buffer has a backing array. The other case that been examined separated if needed but we can't use the approach proposed in the current PR because it changes the semantics of read when there is a short-read followed by a blocking or exception throwing read.

Done

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

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
In reply to this post by Brian Burkhalter-3
On Tue, 16 Feb 2021 23:49:36 GMT, Brian Burkhalter <[hidden email]> wrote:

> I think the implementation changes here look good. I don't know however whether there is enough coverage in the tests. These should verify that the `Reader`, `CharArrayReader`, and `InputStreamReader` implementations of `read(CharBuffer)` are accurate. If there is already sufficient coverage in the tests in `test/jdk/java/io` then that is good enough and nothing need be added.

`CharArrayReader` was lacking a test. I added a test which found a bug and fixed the bug. The PR also contains new tests for `Reader` and `InputStreamReader`. They cover on-heap and off-heap cases.

Is there a way to get test coverage with JTReg tests? I only found [1] which seems out of date and points to an Oracle internal wiki.

 [1] https://wiki.openjdk.java.net/display/CodeTools/JCov+FAQ#JCovFAQ-HowdoIuseJCovwithjtreg?

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

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

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

Alan Bateman-2
In reply to this post by Philippe Marschall-2
On Sat, 13 Mar 2021 14:28:25 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 with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
>
>  - Merge master
>  - Fix bug in CharArrayReader and add unit test
>  - Clean up unit tests
>  - Revert off-heap code path
>  - Replace c-style array declarations
>  - Share work buffer between #skip and #read
>  - Update copyright year
>  - Implement review comment
>  - Revert StreamDecoder changes
>  - Implement CharArrayReader#read(CharBuffer)
>  - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d339320e...c4c859e0

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

> 203:                 target.put(cbuf, 0, nread);
> 204:         }
> 205:         return nread;

Thanks for bringing this back to just the heap buffer case. This part looks good now.

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

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

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

Brian Burkhalter-3
On Sun, 14 Mar 2021 12:57:01 GMT, Alan Bateman <[hidden email]> wrote:

>> Philippe Marschall has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
>>
>>  - Merge master
>>  - Fix bug in CharArrayReader and add unit test
>>  - Clean up unit tests
>>  - Revert off-heap code path
>>  - Replace c-style array declarations
>>  - Share work buffer between #skip and #read
>>  - Update copyright year
>>  - Implement review comment
>>  - Revert StreamDecoder changes
>>  - Implement CharArrayReader#read(CharBuffer)
>>  - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d339320e...c4c859e0
>
> src/java.base/share/classes/java/io/Reader.java line 205:
>
>> 203:                 target.put(cbuf, 0, nread);
>> 204:         }
>> 205:         return nread;
>
> Thanks for bringing this back to just the heap buffer case. This part looks good now.

Agreed.

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

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
In reply to this post by Philippe Marschall-2
On Sat, 13 Mar 2021 14:40:14 GMT, Philippe Marschall <[hidden email]> wrote:

>> I think the implementation changes here look good. I don't know however whether there is enough coverage in the tests. These should verify that the `Reader`, `CharArrayReader`, and `InputStreamReader` implementations of `read(CharBuffer)` are accurate. If there is already sufficient coverage in the tests in `test/jdk/java/io` then that is good enough and nothing need be added.
>
>> I think the implementation changes here look good. I don't know however whether there is enough coverage in the tests. These should verify that the `Reader`, `CharArrayReader`, and `InputStreamReader` implementations of `read(CharBuffer)` are accurate. If there is already sufficient coverage in the tests in `test/jdk/java/io` then that is good enough and nothing need be added.
>
> `CharArrayReader` was lacking a test. I added a test which found a bug and fixed the bug. The PR also contains new tests for `Reader` and `InputStreamReader`. They cover on-heap and off-heap cases.
>
> Is there a way to get test coverage with JTReg tests? I only found [1] which seems out of date and points to an Oracle internal wiki.
>
>  [1] https://wiki.openjdk.java.net/display/CodeTools/JCov+FAQ#JCovFAQ-HowdoIuseJCovwithjtreg?

How do we proceed here? Are there additional changes that you would like me to perform or undo?

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

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

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

Brian Burkhalter-3
In reply to this post by Philippe Marschall-2
On Sat, 13 Mar 2021 14:28:25 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 with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
>
>  - Merge master
>  - Fix bug in CharArrayReader and add unit test
>  - Clean up unit tests
>  - Revert off-heap code path
>  - Replace c-style array declarations
>  - Share work buffer between #skip and #read
>  - Update copyright year
>  - Implement review comment
>  - Revert StreamDecoder changes
>  - Implement CharArrayReader#read(CharBuffer)
>  - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d339320e...c4c859e0

test/jdk/java/io/CharArrayReader/ReadCharBuffer.java line 50:

> 48:     @DataProvider(name = "buffers")
> 49:     public Object[][] createBuffers() {
> 50:         // test both on-heap and off-heap buffers has they may use different code paths

"as they may"

test/jdk/java/io/InputStreamReader/ReadCharBuffer.java line 52:

> 50:     @DataProvider(name = "buffers")
> 51:     public Object[][] createBuffers() {
> 52:         // test both on-heap and off-heap buffers has they make use different code paths

"as they may"

test/jdk/java/io/Reader/ReadCharBuffer.java line 51:

> 49:     @DataProvider(name = "buffers")
> 50:     public Object[][] createBuffers() {
> 51:         // test both on-heap and off-heap buffers has they make use different code paths

"as they may"

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

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