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/
>
