On Thu, 23 Mar 2023 21:15:44 GMT, Eirik Bjorsnos <d...@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+li...@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
