RFR: 8304745: Lazily initialize byte[] in java.io.BufferedInputStream
Sergey Tsypanov
stsypanov at openjdk.org
Thu Mar 23 19:17:33 UTC 2023
On Wed, 22 Mar 2023 23:01:32 GMT, Chen Liang <liach at openjdk.org> wrote:
> This change exposes the CLOSED array
Otherwise we have to either call `getInIfOpen()`/`getBufIfOpen` making them at least protected or dodge this somehow via `SharedSecrets`
> I mean that additional API exposure is acceptable and backward compatible, but the previous contract to subclasses that the protected `buf` is initialized after superclass constructor execution breaks. Before this change proceeds, we have to check how many subclasses of `BufferedInputStream` access this `buf` in a wide range of libraries and evaluate the impact, which I anticipate to be huge.
>
> A slightly less impactful approach would be sharing a `byte[0]` for a newly initialized state, like in ArrayList, so users anticipating a closed `BufferedInputStream` to have a null `buf` will still work. But users anticipating a usable buf after constructor will still break: for instance, since `getBufIfOpen` is private but the `buf` field it uses is protected, nothing prevents subclasses from copying the same implementation over; and the same implementation will stop working with this new optimization.
@liach with your approach tests are green
> src/java.base/share/classes/java/io/BufferedInputStream.java line 78:
>
>> 76: private final InternalLock lock;
>> 77:
>> 78: /**
>
> These should be reverted, now that the subclasses will see the same behavior as before.
But it's just comment clean up
> src/java.base/share/classes/java/io/BufferedInputStream.java line 211:
>
>> 209: throw new IllegalArgumentException("Buffer size <= 0");
>> 210: }
>> 211: buf = new byte[0];
>
> Two recommendations:
> 1. This `new byte[0]` can be safely shared, so I recommend you to put it in a static final field to avoid reallocation
> 2. The subclass compatibility can be retained by:
> Suggestion:
>
> buf = (getClass() == BufferedInputStream.class) ? new byte[0] : new byte[size];
Good point, done!
> src/java.base/share/classes/java/io/BufferedInputStream.java line 216:
>
>> 214: throw new IllegalArgumentException("Buffer size <= 0");
>> 215: }
>> 216: buf = getClass() == BufferedInputStream.class ? EMPTY : new byte[size];
>
> Actually, you can merge this `getClass()` check with the one below for locks.
Done
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1480701591
PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1481236336
PR Review Comment: https://git.openjdk.org/jdk/pull/13150#discussion_r1146316757
PR Review Comment: https://git.openjdk.org/jdk/pull/13150#discussion_r1146321335
PR Review Comment: https://git.openjdk.org/jdk/pull/13150#discussion_r1146414668
More information about the core-libs-dev
mailing list