On Tue, 25 Aug 2026 17:51:58 GMT, Markus KARG <[email protected]> wrote:
>> From a quick look, the updated version looks right but it means tricky
>> decoder logic is duplicated. I think it would be good to see if a simpler
>> approach to just implement the fastpath in SD would be sufficient, e.g.
>>
>> public String tryReadAllAsString() throws IOException {
>> synchronized (lock) {
>> ensureOpen();
>> if (in != null && decoderFromCharset && !readCalled) {
>> return new String(in.readAllBytes(), cs);
>> }
>> }
>> return null;
>> }
>>
>>
>> Overall control would remaining in the enclosing InputStreamReader where the
>> readAllAsString override would be something like this:
>>
>> @Override
>> public String readAllAsString() throws IOException {
>> String s = sd.tryReadAllAsString();
>> return (s != null) ? s : super.readAllAsString();
>> }
>>
>> Would you mind trying that direction? My guess (and I don't have evidence)
>> is that the users of readAllAsString are looking to slurp the entire content
>> rather than then remaining content, maybe a static analysis would suggestion
>> otherwise.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32264#discussion_r3860634517
