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