On Thu, 23 Mar 2023 19:27:04 GMT, Sergey Tsypanov <stsypa...@openjdk.org> wrote:
>> By default `BufferedInputStream` is constructed with internal buffer with >> capacity 8192. In some cases this buffer is never used, e.g. when we call >> `IS.readNBytes()` or `IS.readAllBytes()` (relying on `BIS.read1()`) or when >> `BufferedInputStream` is cascaded. > > 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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1481922199
