Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v10]
On Sun, 25 Apr 2021 10:38:03 GMT, Philippe Marschall 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: > > Fix typos in comments Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
On Fri, 16 Apr 2021 17:00:16 GMT, Brian Burkhalter 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
>
> 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"
Done, and the other two as well.
-
PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v10]
> 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 typos in comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/1915/files - new: https://git.openjdk.java.net/jdk/pull/1915/files/c4c859e0..bbc4274f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1915&range=08-09 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
On Sat, 13 Mar 2021 14:28:25 GMT, Philippe Marschall 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 This PR is on hold pending the author's issuing the `/integrate` command. - PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
On Sat, 13 Mar 2021 14:28:25 GMT, Philippe Marschall 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 Approved modulo my "as they may" remarks on three comment lines in the tests. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
On Sat, 13 Mar 2021 14:28:25 GMT, Philippe Marschall 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 Thanks for rolling back to the direct buffer case, the updated implementation changes look okay. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
On Sat, 13 Mar 2021 14:28:25 GMT, Philippe Marschall 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/Reader/ReadCharBuffer.java line 83: > 81: buffer.limit(8 + 8192 + 1); > 82: buffer.position(8); > 83: assertEquals(reader.read(buffer), 8192 + 1); Lines 80 + 83 might be easier to read if replaced with int position = 8; limit = position + 8192 + 1; buffer.limit(limit); buffer.position(position); assertEquals(reader.read(buffer), limit - position); - PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
On Sat, 13 Mar 2021 14:28:25 GMT, Philippe Marschall
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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
On Sat, 13 Mar 2021 14:40:14 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
On Sun, 14 Mar 2021 12:57:01 GMT, Alan Bateman 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
On Sat, 13 Mar 2021 14:28:25 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
On Tue, 16 Feb 2021 23:49:36 GMT, Brian Burkhalter 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Fri, 19 Feb 2021 07:34:57 GMT, Alan Bateman 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Tue, 16 Feb 2021 23:52:09 GMT, Brian Burkhalter 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Tue, 16 Feb 2021 23:50:30 GMT, Brian Burkhalter 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v9]
> 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v8]
> 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Thu, 18 Feb 2021 20:35:10 GMT, Brian Burkhalter 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Wed, 17 Feb 2021 15:37:11 GMT, Alan Bateman 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Wed, 17 Feb 2021 15:37:11 GMT, Alan Bateman 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Fri, 12 Feb 2021 09:18:10 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Fri, 12 Feb 2021 09:18:10 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
On Tue, 5 Jan 2021 00:48:54 GMT, Brian Burkhalter 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]
On Wed, 10 Feb 2021 21:58:02 GMT, Brian Burkhalter 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
> 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]
On Tue, 9 Feb 2021 14:08:10 GMT, Philippe Marschall
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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v5]
On Fri, 5 Feb 2021 22:08:17 GMT, Brian Burkhalter 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v6]
> 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]
On Tue, 5 Jan 2021 18:10:52 GMT, Brian Burkhalter 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v5]
On Tue, 26 Jan 2021 18:22:02 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]
On Tue, 5 Jan 2021 17:44:20 GMT, Philippe Marschall
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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v5]
> 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]
On Tue, 19 Jan 2021 07:22:49 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]
On Mon, 18 Jan 2021 07:47:30 GMT, Peter Levart 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]
On Sat, 9 Jan 2021 23:06:22 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]
On Sun, 10 Jan 2021 01:59:18 GMT, Brett Okken
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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
On 05.01.21 01:53, Brian Burkhalter wrote: On Thu, 31 Dec 2020 10:11:58 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]
On Sat, 9 Jan 2021 23:06:22 GMT, Philippe Marschall
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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v4]
> 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
> On Jan 5, 2021, at 9:53 AM, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]
> 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
On Thu, 31 Dec 2020 10:11:58 GMT, Philippe Marschall 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v2]
> 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
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
On Thu, 31 Dec 2020 10:10:56 GMT, Philippe Marschall 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
