On Wed, 26 Aug 2026 07:43:04 GMT, Alan Bateman <[email protected]> wrote:
>> Alan, I am perfectly fine with your proposal and adopted it into the PR (all
>> our additional tests run fine with that). In fact, this is what I originally
>> started with, but then Roger's comment about the bytes in the decoder pushed
>> me a bit too far in the slow-path-direction... 😉
>>
>> An AI-driven global static search on Github has proven your claim: While
>> there is a massive use of `InputStreamReader::readAllAsString`, *almost
>> every* code location is calling it without any prior `read()` operation;
>> "Almost everybody" is bulk-slurping the whole stream *always*. Hence, it
>> makes not really much sense to further spend time with fixing the slow path.
>> 👍
>>
>> Having said that, I wonder why you're going with an *extra* `try...()`
>> method, instead of simply implementing `StreamingDecoder::readAllAsString()`
>> *directly*? 🤔
>>
>> public String readAllAsString() throws IOException {
>> synchronized (lock) {
>> ensureOpen();
>> if (in != null && decoderFromCharset && !readCalled) {
>> return new String(in.readAllBytes(), cs);
>> }
>> }
>> return super.readAllAsString();
>> }
>
>> An AI-driven global static search on Github has proven your claim: While
>> there is a massive use of `InputStreamReader::readAllAsString`, _almost
>> every_ code location is calling it without any prior `read()` operation;
>> "Almost everybody" is bulk-slurping the whole stream _always_. Hence, it
>> makes not really much sense to further spend time with fixing the slow path.
>> 👍
>
> Good, and this avoids duplicate stream coding logic.
>
> On whether stream decoder gets a readAllAsString or tryReadAllAsString may
> not matter. The above comment/suggestion was conservative, only because there
> can sometimes be surprises when sub-classing and delegation are in the same
> room. It needs the implementation in the delegate to be as "leafy" as
> possible.
Agreed. So where to go from here?
* Keep the current PR as-is, or simplify implementation to less conservative
but slightly shorter alternative?
```java
@Override
public String readAllAsString() throws IOException {
return sd.readAllAsString();
}
```
```java
@Override
public String readAllAsString() throws IOException {
synchronized (lock) {
ensureOpen();
if (in != null && decoderFromCharset && !readCalled) {
return new String(in.readAllBytes(), cs);
}
return super.readAllAsString();
}
}
```
* Keep the current PR as-is, or removing some of the new tests?
* Whom to ask as second reviewer, as nobody responded so far?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32264#discussion_r3863216872
