On Tue, 25 Aug 2026 15:25:13 GMT, Alan Bateman <[email protected]> wrote:
>> Fast path is now enabled only for charset-created InputStreamReaders.
>>
>> Slow path is now handling "read() after readAllAsString()" (returns `-1`,
>> does not throw). There is a test for it.
>>
>> I am not sure I understood your proposal correct, so kindly asking for
>> advice:
>> - Do you want me to *remove* the slow path? That would be ok for me, as my
>> original proposal only had the *fast* path in mind, actually. OTOH I think
>> it should be working now, and is covered with tests.
>> - Do you want me to cover the slow path *with more tests*? In fact I do not
>> see *which ones*.
>> - Do you want me to provide a full suite of tests covering *all* corner
>> cases, including externally provided CharsetDecoder? This would be a real
>> lot of tests. So I added *just some* for now.
>>
>> You're the lead. What direction do you want me to go?
>
> 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();
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32264#discussion_r3855717812
