On Thu, 3 Jul 2025 13:41:02 GMT, Roger Riggs wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8358533: Decrease value of minGrowth parameter
>
> Another review would be good too.
Thanks @RogerRiggs, @xuemingshe
On Wed, 2 Jul 2025 21:37:00 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
On Wed, 2 Jul 2025 21:37:00 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Fri, 27 Jun 2025 23:00:04 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/Reader.java line 504:
>>
>>> 502: }
>>> 503: if (fragLen >= cb.length/2) {
>>> 504: // allocate larger buffer and copy chars to
>>> be
On Tue, 1 Jul 2025 15:10:10 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/Reader.java line 482:
>>
>>> 480: if (pos == limit) {
>>> 481: int len = limit - start;
>>> 482: if (len >= cb.length) {
>>
>> Observation: thi
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Tue, 1 Jul 2025 19:17:17 GMT, Roger Riggs wrote:
>> With this change
>>
>> --- a/test/jdk/java/io/Reader/ReadAll.java
>> +++ b/test/jdk/java/io/Reader/ReadAll.java
>> @@ -115,6 +115,8 @@ public static void setup() throws IOException {
>> sb.setLength(0);
>> }
>>
>> +
On Tue, 1 Jul 2025 16:34:51 GMT, Brian Burkhalter wrote:
>> I don't know, but I doubt it. I will look into adding something.
>
> With this change
>
> --- a/test/jdk/java/io/Reader/ReadAll.java
> +++ b/test/jdk/java/io/Reader/ReadAll.java
> @@ -115,6 +115,8 @@ public static void setup() throws IO
On Tue, 1 Jul 2025 15:10:33 GMT, Brian Burkhalter wrote:
>> test/jdk/java/io/Reader/ReadAll.java line 117:
>>
>>> 115: sb.setLength(0);
>>> 116: }
>>> 117:
>>
>> Does one of these cases result in a *very very long line without a
>> terminator"; something that would trigger
On Tue, 1 Jul 2025 01:37:27 GMT, Roger Riggs wrote:
> I think resizing the buffer when the line length exceeds cb.length/2 will be
> more efficient (cpu wise).
I concur. I missed that this had been changed in the most recent commit.
> test/jdk/java/io/Reader/ReadAll.java line 117:
>
>> 115:
On Mon, 30 Jun 2025 19:38:59 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
On Mon, 30 Jun 2025 19:38:59 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Mon, 30 Jun 2025 18:28:26 GMT, Xueming Shen wrote:
> the throughput appears to back to normal with 'find term while loop' fast path
This version passes the regression test and is a little faster than the latest
PR version. I'll run it through the CI and probably use it in the next commit.
T
On Fri, 27 Jun 2025 19:41:01 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
On Sat, 28 Jun 2025 08:41:27 GMT, Xueming Shen wrote:
> The modified code below is a PoC for reference. It seems to work as expected,
> though not fully tested.
This looks like a nice reworking, and appears to pass the regression test, but
its throughput is less than half that of the current v
On Fri, 27 Jun 2025 19:41:01 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
On Fri, 27 Jun 2025 22:46:40 GMT, Xueming Shen wrote:
> [...] more in line with how resizing is handled in string builder
Looks like `StringBuilder` eventually calls
`ArraysSupport.newLength(int,int.int)`. This is probably worth checking out.
-
PR Review Comment: https://git.openj
On Fri, 27 Jun 2025 17:32:25 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
On Fri, 27 Jun 2025 19:21:19 GMT, Roger Riggs wrote:
> More directly, use limit instead of term in the computation of length.
Good point. So changed in 3e5e498.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25863#discussion_r2172749427
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Fri, 27 Jun 2025 18:54:20 GMT, Brian Burkhalter wrote:
>> Yes, `term == limit`. The "no line terminator" means none was encountered
>> before the buffer's end was reached.
>
> Maybe it should be something like:
>
> // no line terminator before end of buffer
More directly, use limit instead
On Fri, 27 Jun 2025 18:52:14 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/Reader.java line 494:
>>
>>> 492: skipLF = isCR;
>>> 493: } else { // no line terminator
>>> 494: int len = term - pos;
>>
>> I think term ==
On Fri, 27 Jun 2025 18:44:55 GMT, Roger Riggs wrote:
>> Brian Burkhalter has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - 8358533: Use a boolean instead of fragPos != -1
>> - 8358533: Immediately skip LF right after CR
>
> src/java.bas
On Fri, 27 Jun 2025 17:32:25 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
On Fri, 27 Jun 2025 15:36:29 GMT, Roger Riggs wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8358533: Test change accidentally omitted from previous commit
>
> src/java.base/share/classes/java/io/Reader.java l
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Fri, 27 Jun 2025 15:03:05 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Tue, 24 Jun 2025 19:15:38 GMT, Brian Burkhalter wrote:
> > One possibility is to copy the remaining fragment of a line to a new
> > transfer buffer (maybe twice the size).
>
> It's probably worth testing this.
In 0ee3f59 I implemented something like this. It is a bit faster than the
currec
On Wed, 25 Jun 2025 19:57:56 GMT, Xueming Shen wrote:
>> Fixed in
>> [d5abfa4](https://github.com/openjdk/jdk/pull/25863/commits/d5abfa450cb3fcd604560833038735e41952bce9
>> ).
>
> My apologies if this is a naive question :-) I'm sure this was discussed when
> the method was added, so this is ju
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Tue, 24 Jun 2025 18:51:04 GMT, Brian Burkhalter wrote:
>> Right, the specification here requires an unmodifiable List, so an
>> unmodifiable wrapper or a list from `List.copyOf()` is appropriate.
>
> Fixed in
> [d5abfa4](https://github.com/openjdk/jdk/pull/25863/commits/d5abfa450cb3fcd604560
On Fri, 20 Jun 2025 17:16:58 GMT, Xueming Shen wrote:
>> The change is motivated by performance, but there will be many inputs that
>> are less than the transfer buffer size and those will not use the
>> StringBuilder, so creating it before it is needed could be avoided.
>> When a partial line
On Tue, 24 Jun 2025 18:49:08 GMT, Brian Burkhalter wrote:
>> resizing/newCapacity is always expensive and tricky, string builder
>> included. so maybe we should decide if 'long lines' (> transfer buffer size)
>> is the goal of this pr. if not, it might be reasonable/make sense (???) to
>> sim
On Tue, 24 Jun 2025 08:08:58 GMT, Andrey Turbanov wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8358533: Init StringBuilder to size zero; use StringBuilder.isEmpty()
>
> test/micro/org/openjdk/bench/java/io/R
On Mon, 23 Jun 2025 21:23:05 GMT, Stuart Marks wrote:
>> src/java.base/share/classes/java/io/Reader.java line 508:
>>
>>> 506: }
>>> 507:
>>> 508: return lines;
>>
>> Do we really want to return a mutable `ArrayList` here? In earlier
>> discussions about this very API I was to
On Tue, 24 Jun 2025 19:12:25 GMT, Roger Riggs wrote:
> One possibility is to copy the remaining fragment of a line to a new transfer
> buffer (maybe twice the size).
It's probably worth testing this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25863#discussion_r216472586
On Tue, 24 Jun 2025 18:46:49 GMT, Brian Burkhalter wrote:
>> The `readAllLines` method has a specification of line terminators that
>> agrees with that of `BufferedReader::readLine` and `String::lines` and so we
>> don't want to change it to be different.
>>
>> Unfortunately `Scanner` doesn't
On Tue, 24 Jun 2025 19:00:42 GMT, Brian Burkhalter wrote:
>>> My suggestion is to call `new StringBuilder(0)`
>>
>> So changed in 8ccfd54.
>
>> When a partial line is left in the transfer buffer, copy it to the beginning
>> of the buffer and read more characters for the remaining size of the bu
On Wed, 18 Jun 2025 00:05:17 GMT, Brian Burkhalter wrote:
> The throughput of the implementation [...].
The performance comments made
[here](https://github.com/openjdk/jdk/pull/25863#issuecomment-2982169343) still
apply to the most recent commit.
-
PR Comment: https://git.openjdk
On Thu, 19 Jun 2025 10:59:14 GMT, Markus KARG wrote:
> Is there a reason for `sb.length() == 0` instead of `sb.isEmpty()`?
The former is identical. Modified to use `isEmpty` in 8ccfd54.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25863#discussion_r2164686620
On Wed, 18 Jun 2025 02:12:50 GMT, Shaojin Wen wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8358533: Init StringBuilder to size zero; use StringBuilder.isEmpty()
>
> src/java.base/share/classes/java/io/Reader
On Wed, 18 Jun 2025 02:25:03 GMT, Chen Liang wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8358533: Init StringBuilder to size zero; use StringBuilder.isEmpty()
>
> src/java.base/share/classes/java/io/Reader.
On Mon, 23 Jun 2025 21:16:30 GMT, Stuart Marks wrote:
>> `Scanner` seems to scan for even more characters:
>> https://github.com/openjdk/jdk/blob/c4fb00a7be51c7a05a29d3d57d787feb5c698ddf/src/java.base/share/classes/java/util/Scanner.java#L490
>>
>> Would it make sense to resemble this? Would it
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Tue, 24 Jun 2025 14:42:57 GMT, Brian Burkhalter wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
> to the list.
Brian Burkhalter has updated the pull request
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
On Thu, 19 Jun 2025 11:08:32 GMT, Markus KARG wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
>>
On Thu, 19 Jun 2025 10:57:52 GMT, Markus KARG wrote:
>>> I think we should treat "\r\n" as a single line terminator?
>>
>> You are correct: that needs to be fixed:
>>
>> jshell> Reader r = new StringReader("hello\r\nworld")
>> r ==> java.io.StringReader@480bdb19
>>
>> jshell> r.readAllLines()
On Fri, 20 Jun 2025 15:52:08 GMT, Roger Riggs wrote:
>> My suggestion is to call `new StringBuilder(0)` as it is possible this is
>> completely unused because we always hit the `eol && sb.length() == 0` path
>> below.
>
> The change is motivated by performance, but there will be many inputs tha
On Thu, 19 Jun 2025 10:53:14 GMT, Markus KARG wrote:
>>> Is there a reason for this pre-allocation?
>>
>> What would you suggest? Start with a smaller allocation and increase it if
>> needed? There is no possibility of knowing the length of the stream.
>
> As this PR explicitly targets performa
On Fri, 20 Jun 2025 15:50:53 GMT, Chen Liang wrote:
>> As this PR explicitly targets performance and as the aim of this method is
>> to keep **all** content in-memory anyways, I wonder if it would be
>> acceptable and even faster to pre-allocate `new
>> StringBuilder(TRANSFER_BUFFER_SIZE)`? In
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
On Wed, 18 Jun 2025 16:57:22 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/Reader.java line 455:
>>
>>> 453: List lines = new ArrayList();
>>> 454:
>>> 455: StringBuilder sb = new StringBuilder(82);
>>
>> Is there a reason for this pre-allocation? If the w
On Wed, 18 Jun 2025 16:45:33 GMT, Brian Burkhalter wrote:
>> I think we should treat "\r\n" as a single line terminator? for example
>>
>> "hello\r\nworld".lines().forEach(line -> out.format("[%s]\n", line));
>> =>
>> [hello]
>> [world]
>>
>> instead of (the current impl)
>>
>> [hello]
>> []
On Wed, 18 Jun 2025 02:26:48 GMT, Chen Liang wrote:
> Is there a reason for this pre-allocation?
What would you suggest? Start with a smaller allocation and increase it if
needed? There is no possibility of knowing the length of the stream.
-
PR Review Comment: https://git.openjdk
On Wed, 18 Jun 2025 02:38:04 GMT, Chen Liang wrote:
> Edit: No, we need to check empty lines.
Right. I caught this during testing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25863#discussion_r2155088265
On Wed, 18 Jun 2025 02:33:40 GMT, Xueming Shen wrote:
> I think we should treat "\r\n" as a single line terminator?
You are correct: that needs to be fixed:
jshell> Reader r = new StringReader("hello\r\nworld")
r ==> java.io.StringReader@480bdb19
jshell> r.readAllLines()
$3 ==> [hello, , world
On Wed, 18 Jun 2025 09:20:12 GMT, Shaojin Wen wrote:
> If we want better performance, we should go a step further and overload the
> readAllLines method in the Reader implementation class.
Perhaps, but not in this request. A separate issue should be filed and
addressed subsequently.
-
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
On Wed, 18 Jun 2025 01:34:14 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/Reader.java line 469:
>>
>>> 467: if (c == '\r' || c == '\n')
>>> 468: break;
>>> 469: term++;
>>
>> It might be worth adding a test o
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
On Wed, 18 Jun 2025 00:53:51 GMT, Roger Riggs wrote:
>> Replaces the implementation `readAllCharsAsString().lines().toList()` with
>> reading into a temporary `char` array which is then processed to detect line
>> terminators and copy non-terminating characters into strings which are added
>>
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
Replaces the implementation `readAllCharsAsString().lines().toList()` with
reading into a temporary `char` array which is then processed to detect line
terminators and copy non-terminating characters into strings which are added to
the list.
-
Commit messages:
- 8358533: Improve p
On Wed, 18 Jun 2025 00:04:37 GMT, Brian Burkhalter wrote:
> Replaces the implementation `readAllCharsAsString().lines().toList()` with
> reading into a temporary `char` array which is then processed to detect line
> terminators and copy non-terminating characters into strings which are added
>
73 matches
Mail list logo
