RFR: 8304745: Lazily initialize byte[] in java.io.BufferedInputStream [v9]

[Jaikiran Pai]

On Thu, 6 Apr 2023 09:55:57 GMT, Sergey Tsypanov <stsypanov at openjdk.org> wrote:

>>> So I think the main thing now is to decide whether the benchmark should be included or not.
>> 
>> In my review I hadn't paid attention to the benchmark class. I now applied this patch locally and reviewed the benchmark code and even ran it locally, with and without changes.
>> 
>> Like Alan notes, the change that has been done in `BufferedInputStream` code, in this PR, now delays the allocation of the internal byte[]. Unlike previously where it used to be allocated when the `BufferedInputStream` was instantiated. For this change, I think a benchmark isn't necessary - what would it test, how quickly a `new BufferedInputStream` returns as compared to previous code? Or how slow a `read()` operation (which could potentially end up allocating the internal byte[]) would now run as compared to the previous code? To me, looking at the change it's clear that delaying the byte[] creation to the point where it's really needed does add value and I don't think a benchmark which checks the duration of a read() operation, would be able to capture this accurately.
>> 
>> So, I think it's OK if we drop the benchmark from this PR, if @stsypanov agrees. If however we do go ahead with including this benchmark, then I think the benchmark class file will need a change in its package declaration. It should be:
>> 
>> 
>> package org.openjdk.bench.java.io;
>> 
>> to match where it resides (just like other benchmark classes in this directory).
>
>>  For this change, I think a benchmark isn't necessary - what would it test, how quickly a new BufferedInputStream returns as compared to previous code?
> 
> @jaikiran first of all it demonstrates reducing allocation for plain/cascaded readAllBytes() operation. If the question was only about buffer allocation delay the change would be pointless to me.
> 
>> So, I think it's OK if we drop the benchmark from this PR, if @stsypanov agrees
> 
> I agree. There'd be hardly more changes into buffer allocation, so we needn't meaure it. And if this patch is for some reason reverted then we needn't it either.

Thank you for the update, @stsypanov. This looks good to me. I had run this against our internal CI yesterday and the test runs completed fine. I'll go ahead and sponsor this shortly.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1498807545