On Mon, 7 Oct 2024 19:44:12 GMT, Chen Liang <[email protected]> wrote:
>> I really do see you point, nevertheless I think we should be kind to "bad"
>> users, too. In the end, there is *no* specification which explicitly tells
>> "bad" users that readers are support to be short-living (despite it being
>> rather obvious), while `close` explicitly mentions freeing *all* resources,
>> so it is rather unexpected to keep something past `close`, even for short
>> term. NB: `StringReader` applies the very same behavior, too (using `s` and
>> `str` variables), that's where I copied the code from originally. It is not
>> a big problem for us to have two variables, neither that the name of the
>> variable is `source` (in fact, I do like `source` even more than `cs`, but
>> this is just my personal preference). To sum up, I really dislike the idea
>> to keep the reference for any longer than essentially needed. If it's just
>> about the name of the variable, I could simply switch them if you prefer
>> that.
>
> On a linux x64 build:
>
> ./javap -p java.io.Reader$2
> Compiled from "Reader.java"
> class java.io.Reader$2 extends java.io.Reader {
> private final int length;
> private java.lang.CharSequence cs;
> private int next;
> private int mark;
> final java.lang.CharSequence val$source;
> java.io.Reader$2(java.lang.CharSequence);
> private void ensureOpen() throws java.io.IOException;
> public int read() throws java.io.IOException;
> public int read(char[], int, int) throws java.io.IOException;
> public long skip(long) throws java.io.IOException;
> public boolean ready() throws java.io.IOException;
> public boolean markSupported();
> public void mark(int) throws java.io.IOException;
> public void reset() throws java.io.IOException;
> public void close();
> }
>
>
> The javap in the built binaries of this patch shows that this anonymous class
> already keeps the `CharSequence val$source` in a final field; therefore, your
> setting the `cs` field to `null` will not help GC or anything of that sort.
Sigh, too bad, as keeping `val$source` is actually useless in our case, but I
understand that this happens due to our class being an anonymous inner class,
not a standalone class (as it was the case with `StringReader`, where this
source code originates from).
Anyways, adopted your proposal now to proceed with this PR.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1791494712
