As for increasing the buffer size, it makes sense but IMHO needs a CSR and
a release note.
Cheers, Thomas
On Thu, Apr 16, 2020 at 8:21 AM Thomas Stüfe <thomas.stu...@gmail.com>
wrote:
> Hi Volker,
>
> 252 // Check the decompressor
> 253 if (inf.finished() || (len == 0)/* no more input */) {
> 254 break;
> 255 }
>
> Not sure but I think this is wrong because now you bypass the followup
> handling of inf.needsDirectory.
>
> Inflater.inflate() returns 0 if either needsInput or needsDirectory. We
> have to ignore needsInput since we have no input anymore, but
> needsDirectory has to be handled, no?
>
> Cheers, Thomas
>
>
> On Wed, Apr 15, 2020 at 7:49 PM Volker Simonis <volker.simo...@gmail.com>
> wrote:
>
>> Hi,
>>
>> can I please have a review for the following simple performance
>> enhancement:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2020/8242848/
>> https://bugs.openjdk.java.net/browse/JDK-8242864
>>
>> InflaterOutputStream has an internal buffer which is used for the
>> inflated data. This buffer can be configured to a custom size (the
>> default is 512 bytes) by using the specialized
>> "InflaterOutputStream(OutputStream out, Inflater infl, int bufLen)"
>> constructor. A bigger buffer, results in fewer native calls to
>> "write()" on the associated OutputStream and better overall
>> performance.
>>
>> Unfortunately "InflaterOutputStream.write(byte[] b, int off, int len)"
>> unnecessarily chunks its own byte input buffer "b" into pieces of
>> maximum 512 bytes before feeding them to the underlying Inflater. This
>> unnecessarily increases the number of native calls to
>> Inflater.inflate() and limits the benefit of the configurable internal
>> buffer to a size of ~(512 * compression-ratio) bytes.
>>
>> Instead, we could easily pass the complete input buffer "b" as input
>> to the underlying Inflater. This simplifies the code and results in up
>> to ~25% performance improvements for bigger internal buffers (see the
>> JMH benchmark attached to the bug).
>>
>> Regarding the implementation, I initially wanted to completely remove
>> the "for (;;)" loop after removing the chunking of the input buffer.
>> But I think it is still necessary, because an InflaterOutputStream can
>> be created with a custom Inflater which already has some input data.
>> In that case, the Inflater will first consume its initial input data
>> in the first "for" loop iteration while "write()"s input buffer "b"
>> will only be consumed in the second "for" loop iteration.
>>
>> While doing this change, I've also realized that all the streams in
>> java.util.zip (i.e. DeflaterInputStream, GZIPInputStream,
>> GZIPOutputStream, InflaterInputStream, DeflaterOutputStream) use an
>> internal byte buffer of 512 bytes by default. Looking at the benchmark
>> attached to JDK-8242864, I think that increasing this default to a
>> bigger size (e.g. 4096 bytes) will considerably speed up (up to 50%)
>> read and write operations on these streams when they are created with
>> the default buffer size. I think the value "512" is a legacy of old
>> times when memory was more precious :) so I've opened "JDK-8242864:
>> Increase the default, internal buffer size of the Streams in
>> java.util.zip" to track that as as separate issue. Do you think it
>> makes sense to increase that default value?
>>
>> Thank you and best regards,
>> Volker
>>
>> PS: do you think it makes sense to contribute the benchmark attached
>> to JDK-8242864 to the code-tools/mh-jdk-microbenchmarks [1] project?
>>
>> [1] http://openjdk.java.net/projects/code-tools/jmh-jdk-microbenchmarks/
>>
>
