RFR: 8304745: Lazily initialize byte[] in java.io.BufferedInputStream [v2]
Sergey Tsypanov
stsypanov at openjdk.org
Fri Mar 24 15:40:46 UTC 2023
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
More information about the core-libs-dev
mailing list