On Thu, 6 Apr 2023 09:55:57 GMT, Sergey Tsypanov <stsypa...@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). > >> 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? > > @jaikiran first of all it demonstrates reducing allocation for plain/cascaded > readAllBytes() operation. If the question was only about buffer allocation > delay the change would be pointless to me. > >> So, I think it's OK if we drop the benchmark from this PR, if @stsypanov >> agrees > > I agree. There'd be hardly more changes into buffer allocation, so we needn't > meaure it. And if this patch is for some reason reverted then we needn't it > either. Thank you for the update, @stsypanov. This looks good to me. I had run this against our internal CI yesterday and the test runs completed fine. I'll go ahead and sponsor this shortly. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13150#issuecomment-1498807545
