On Wed, 5 Apr 2023 07:08:58 GMT, Alan Bateman <al...@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). ------------- PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1498753613
