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

[Sergey Tsypanov]

On Thu, 23 Mar 2023 21:15:44 GMT, Eirik Bjorsnos <duke at openjdk.org> wrote:

>> Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update src/java.base/share/classes/java/io/BufferedInputStream.java
>>   
>>   Co-authored-by: liach <7806504+liach at users.noreply.github.com>
>
> My last comment was so heavily edited, I think it makes sense to delete it and include the last edit here:
> 
>> @eirbjo you mean benchmark for BIS instantiation?
> 
> My general thought is that a benchmark demonstrating the proposed performance benefits is always good and should perhaps be a requirement for such performance related changes. An effort should be made to detect regressions as well. This is just my personal preference, won't claim this is policy of any kind.
> 
> Until something is benchmarked, nobody knows if the benefit is just speculation.
> 
> I have experienced and suggested several PRs myself which benchmark work later showed dubious improvements which cased the change to be withdrawn.
> 
> In https://github.com/openjdk/jdk/pull/13121, I ran some (existing) benchmark on the PR only to discover it seems to have a weird, subtle regression.
> 
> Besides this: A benchmark also help you save the performance improvements for the future. Without a benchmark, it is easier for someone to come along and nullify your improvements by some independent change.
> 
> Personal bottom line: When a PR is motivated by performance, suggested benefits should be demonstrated through benchmarks.
> 
> Very bottom line: I do not claim that this PR has no performance benefits.
> 
> Final bottom line: Performance claims need proofs and the proof needs to fit in the margin.

@eirbjo added benchmark

aster

Benchmark                                                             Mode  Cnt      Score     Error   Units
BufferedInputStreamBenchmark.readAllBytes                             avgt   20      4.717 ±   0.620   us/op
BufferedInputStreamBenchmark.readAllBytes:·gc.alloc.rate              avgt   20   5313.239 ± 678.404  MB/sec
BufferedInputStreamBenchmark.readAllBytes:·gc.alloc.rate.norm         avgt   20  25752.007 ±   0.001    B/op
BufferedInputStreamBenchmark.readAllBytes:·gc.count                   avgt   20    201.000            counts
BufferedInputStreamBenchmark.readAllBytes:·gc.time                    avgt   20    840.000                ms
BufferedInputStreamBenchmark.readAllBytesCascade                      avgt   20      8.815 ±   5.016   us/op
BufferedInputStreamBenchmark.readAllBytesCascade:·gc.alloc.rate       avgt   20   4156.740 ± 685.852  MB/sec
BufferedInputStreamBenchmark.readAllBytesCascade:·gc.alloc.rate.norm  avgt   20  34064.013 ±   0.007    B/op
BufferedInputStreamBenchmark.readAllBytesCascade:·gc.count            avgt   20    138.000            counts
BufferedInputStreamBenchmark.readAllBytesCascade:·gc.time             avgt   20    933.000                ms


8304745

Benchmark                                                             Mode  Cnt      Score     Error   Units
BufferedInputStreamBenchmark.readAllBytes                             avgt   20      2.931 ±   0.208   us/op
BufferedInputStreamBenchmark.readAllBytes:·gc.alloc.rate              avgt   20   5738.023 ± 369.653  MB/sec
BufferedInputStreamBenchmark.readAllBytes:·gc.alloc.rate.norm         avgt   20  17552.004 ±   0.001    B/op
BufferedInputStreamBenchmark.readAllBytes:·gc.count                   avgt   20    168.000            counts
BufferedInputStreamBenchmark.readAllBytes:·gc.time                    avgt   20   1254.000                ms
BufferedInputStreamBenchmark.readAllBytesCascade                      avgt   20      3.219 ±   0.177   us/op
BufferedInputStreamBenchmark.readAllBytesCascade:·gc.alloc.rate       avgt   20   5247.600 ± 288.255  MB/sec
BufferedInputStreamBenchmark.readAllBytesCascade:·gc.alloc.rate.norm  avgt   20  17664.005 ±   0.001    B/op
BufferedInputStreamBenchmark.readAllBytesCascade:·gc.count            avgt   20    157.000            counts
BufferedInputStreamBenchmark.readAllBytesCascade:·gc.time             avgt   20   1168.000                ms

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

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