Re: RFR(s): 8244659: Improve ZipFile.getInputStream
On 2020-05-12 16:07, Martin Buchholz wrote: On Tue, May 12, 2020 at 6:42 AM Claes Redestad mailto:claes.redes...@oracle.com>> wrote: Hi Volker, I think this look like a nice improvement! One high-level concern I have with the design is that this will add and retain (at least) one 64k buffer to each Jar-/ZipFile we've read a stream from. We routinely see apps reading classes from 100s of jar files on their class path, so this could add noticeable overhead to the baseline retained memory usage of such applications. At Google, it's common to run java applications with 10,000 small jar files on the classpath. Ouch! That could mean a 600Mb+ footprint increase. Not very nice. Making the "quiescent cost" of each ZipFile object as small as possible is important. OTOH we currently retain the byte[] of the zip file central directory indefinitely. Striking the right trade-off is hard - and footprint has often gotten the shortest stick. Have you considered other strategies such as making the cache global? Since a (the?) common usage pattern is likely a single thread repeatedly reading resources from a series of jar files, contention on such a global cache is likely going to be very low, while likely reducing the total number of buffers we have to allocate and retain to single-digit numbers. I don't insist on a re-design, but it shouldn't be hard to prototype and run some numbers on it. Java resource management is still hard! ... but fun?! /Claes
Re: RFR(s): 8244659: Improve ZipFile.getInputStream
On Tue, May 12, 2020 at 6:42 AM Claes Redestad wrote: > Hi Volker, > > I think this look like a nice improvement! > > One high-level concern I have with the design is that this will add and > retain (at least) one 64k buffer to each Jar-/ZipFile we've read a > stream from. We routinely see apps reading classes from 100s of jar > files on their class path, so this could add noticeable overhead to the > baseline retained memory usage of such applications. > At Google, it's common to run java applications with 10,000 small jar files on the classpath. Making the "quiescent cost" of each ZipFile object as small as possible is important. OTOH we currently retain the byte[] of the zip file central directory indefinitely. > > Have you considered other strategies such as making the cache global? > Since a (the?) common usage pattern is likely a single thread repeatedly > reading resources from a series of jar files, contention on such a > global cache is likely going to be very low, while likely reducing the > total number of buffers we have to allocate and retain to single-digit > numbers. I don't insist on a re-design, but it shouldn't be hard to > prototype and run some numbers on it. > Java resource management is still hard!
Re: RFR(s): 8244659: Improve ZipFile.getInputStream
Hi Volker, I think this look like a nice improvement! One high-level concern I have with the design is that this will add and retain (at least) one 64k buffer to each Jar-/ZipFile we've read a stream from. We routinely see apps reading classes from 100s of jar files on their class path, so this could add noticeable overhead to the baseline retained memory usage of such applications. Have you considered other strategies such as making the cache global? Since a (the?) common usage pattern is likely a single thread repeatedly reading resources from a series of jar files, contention on such a global cache is likely going to be very low, while likely reducing the total number of buffers we have to allocate and retain to single-digit numbers. I don't insist on a re-design, but it shouldn't be hard to prototype and run some numbers on it. Minor random comments: Since you're not assigning null to bufferCache anywhere, this field could be final and the null-check in releaseBuffer removed. Pre-existing, but I wonder if there's a good reason to assign null to the inflaterCache in the run() method. Seems like trying to do the GCs job.. It could probably be removed, the field made final and the null check removed in the same way. On 2020-05-08 17:36, Volker Simonis wrote: Hi, can I please have a review for the following small enhancement which improves the speed of reading from ZipFile.getInputStream() by ~5%: http://cr.openjdk.java.net/~simonis/webrevs/2020/8244659/ https://bugs.openjdk.java.net/browse/JDK-8244659 ZipFile.getInputStream() tries to find a good size for sizing the internal buffer of the underlying InflaterInputStream. This buffer is used to read the compressed data from the associated InputStream. Unfortunately, ZipFile.getInputStream() uses CENLEN (i.e. the uncompressed size of a ZipEntry) instead of CENSIZ (i.e. the compressed size of a ZipEntry) to configure the input buffer and thus unnecessarily wastes memory, because the corresponding, compressed input data is at most CENSIZ bytes long. After fixing this and doing some benchmarks, I realized that a much bigger problem is the continuous allocation of new, temporary input buffers for each new input stream. Assuming that a zip files usually has hundreds if not thousands of ZipEntries, I think it makes sense to cache these input buffers. Fortunately, ZipFile already has a built-in mechanism for such caching which it uses for caching the Inflaters needed for each new input stream. In order to cache the buffers as well, I had to add a new , package-private constructor to InflaterInputStream. I'm not sure if it makes sense to make this new constructor public, to enable other users of InflaterInputStream to pre-allocate the buffer. If you think so, I'd be happy to do that change and open a CSR for this issue. This could be interesting for some non-ZipFile use cases such as reading gzipped content from network streams - but I think considering making it public should be done separately - along with some use case to motivate it - and not hold back this RFE. Thanks! /Claes Adding a cache for input stream buffers increases the speed of reading ZipEntries from an InputStream by roughly 5% (see benchmark results below). More importantly, it also decreases the memory consumption for each call to ZipFile.getInputStream() which can be quite significant if many ZipEntries are read from a ZipFile. One visible effect of caching the input buffers is that the manual JTreg test java/util/zip/ZipFile/TestZipFile.java, which regularly failed on my desktop with an OutOfMemoryError before, now reliably passes (this tests calls ZipFile.getInputStream() excessively). I've experimented with different buffer sizes (even with buffer sizes depending on the size of the compressed ZipEntries), but couldn't see any difference so I decided to go with a default buffer size of 65536 which already was the maximal buffer size in use before my change. I've also added a shortcut to Inflater which prevents us doing a native call down to libz's inflate() method every time we call Inflater.inflate() with "input == ZipUtils.defaultBuf" which is the default for every newly created Inflater and for Inflaters after "Inflater.reset()" has been called on them. Following some JMH benchmark results which show the time and memory used to read all bytes from a ZipEntry before and after this change. The 'size' parameter denotes the uncompressed size of the corresponding ZipEntries. In the "BEFORE" numbers, when looking at the "gc.alloc.rate.norm" values, you can see the anomaly caused by using CENLEN instead of CENSIZ in ZipFile.getInputStream(). I.e. getInputStream() chooses to big buffers because it looks at the uncompressed ZipEntry sizes which are ~ 6 times bigger than the compressed sizes. Also, the old implementation capped buffers bigger than 65536 to 8192 bytes. The memory savings for a call to getInputStream() are obviously the effect of repetadly calling
Re: RFR(s): 8244659: Improve ZipFile.getInputStream
Sure, here it is: http://cr.openjdk.java.net/~simonis/webrevs/2020/8244659.01/ Fortunately, it still applies cleanly :) It also passed the zip-related JTreg tests and submit repo. On Mon, May 11, 2020 at 10:27 PM Lance Andersen wrote: > Hi Volker, > > Could you update your patch now that Claes’s changes are back as I think > that would make it easier to review > > Thank you! > > On May 8, 2020, at 11:36 AM, Volker Simonis > wrote: > > Hi, > > can I please have a review for the following small enhancement which > improves the speed of reading from ZipFile.getInputStream() by ~5%: > > http://cr.openjdk.java.net/~simonis/webrevs/2020/8244659/ > https://bugs.openjdk.java.net/browse/JDK-8244659 > > ZipFile.getInputStream() tries to find a good size for sizing the internal > buffer of the underlying InflaterInputStream. This buffer is used to read > the compressed data from the associated InputStream. Unfortunately, > ZipFile.getInputStream() uses CENLEN (i.e. the uncompressed size of a > ZipEntry) instead of CENSIZ (i.e. the compressed size of a ZipEntry) to > configure the input buffer and thus unnecessarily wastes memory, because > the corresponding, compressed input data is at most CENSIZ bytes long. > > After fixing this and doing some benchmarks, I realized that a much bigger > problem is the continuous allocation of new, temporary input buffers for > each new input stream. Assuming that a zip files usually has hundreds if > not thousands of ZipEntries, I think it makes sense to cache these input > buffers. Fortunately, ZipFile already has a built-in mechanism for such > caching which it uses for caching the Inflaters needed for each new input > stream. In order to cache the buffers as well, I had to add a new , > package-private constructor to InflaterInputStream. I'm not sure if it > makes sense to make this new constructor public, to enable other users of > InflaterInputStream to pre-allocate the buffer. If you think so, I'd be > happy to do that change and open a CSR for this issue. > > Adding a cache for input stream buffers increases the speed of reading > ZipEntries from an InputStream by roughly 5% (see benchmark results below). > More importantly, it also decreases the memory consumption for each call to > ZipFile.getInputStream() which can be quite significant if many ZipEntries > are read from a ZipFile. One visible effect of caching the input buffers is > that the manual JTreg test java/util/zip/ZipFile/TestZipFile.java, which > regularly failed on my desktop with an OutOfMemoryError before, now > reliably passes (this tests calls ZipFile.getInputStream() excessively). > > I've experimented with different buffer sizes (even with buffer sizes > depending on the size of the compressed ZipEntries), but couldn't see any > difference so I decided to go with a default buffer size of 65536 which > already was the maximal buffer size in use before my change. > > I've also added a shortcut to Inflater which prevents us doing a native > call down to libz's inflate() method every time we call Inflater.inflate() > with "input == ZipUtils.defaultBuf" which is the default for every newly > created Inflater and for Inflaters after "Inflater.reset()" has been called > on them. > > Following some JMH benchmark results which show the time and memory used to > read all bytes from a ZipEntry before and after this change. The 'size' > parameter denotes the uncompressed size of the corresponding ZipEntries. > > In the "BEFORE" numbers, when looking at the "gc.alloc.rate.norm" values, > you can see the anomaly caused by using CENLEN instead of CENSIZ in > ZipFile.getInputStream(). I.e. getInputStream() chooses to big buffers > because it looks at the uncompressed ZipEntry sizes which are ~ 6 times > bigger than the compressed sizes. Also, the old implementation capped > buffers bigger than 65536 to 8192 bytes. > > The memory savings for a call to getInputStream() are obviously the effect > of repetadly calling getInputStream() on the same zip file (becuase only in > that case, the caching of the input buffers pays of). But as I wrote > before, I think it is common to have mor then a few entries in a zip file > and even if not, the overhead of caching is minimal compared to the > situation we had before the change. > > Thank you and best regards, > Volker > > = BEFORE 8244659 = > Benchmark (size) > Mode Cnt Score Error Units > ZipFileGetInputStream.readAllBytes 1024 > avgt3 13.577 ± 0.540 us/op > ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm 1024 > avgt3 1872.673 ± 0.317B/op > ZipFileGetInputStream.readAllBytes:·gc.count 1024 > avgt3 57.000counts > ZipFileGetInputStream.readAllBytes:·gc.time 1024 > avgt3 15.000ms > ZipFileGetInputStream.readAllBytes
Re: RFR(s): 8244659: Improve ZipFile.getInputStream
Hi Volker, Could you update your patch now that Claes’s changes are back as I think that would make it easier to review Thank you! > On May 8, 2020, at 11:36 AM, Volker Simonis wrote: > > Hi, > > can I please have a review for the following small enhancement which > improves the speed of reading from ZipFile.getInputStream() by ~5%: > > http://cr.openjdk.java.net/~simonis/webrevs/2020/8244659/ > https://bugs.openjdk.java.net/browse/JDK-8244659 > > ZipFile.getInputStream() tries to find a good size for sizing the internal > buffer of the underlying InflaterInputStream. This buffer is used to read > the compressed data from the associated InputStream. Unfortunately, > ZipFile.getInputStream() uses CENLEN (i.e. the uncompressed size of a > ZipEntry) instead of CENSIZ (i.e. the compressed size of a ZipEntry) to > configure the input buffer and thus unnecessarily wastes memory, because > the corresponding, compressed input data is at most CENSIZ bytes long. > > After fixing this and doing some benchmarks, I realized that a much bigger > problem is the continuous allocation of new, temporary input buffers for > each new input stream. Assuming that a zip files usually has hundreds if > not thousands of ZipEntries, I think it makes sense to cache these input > buffers. Fortunately, ZipFile already has a built-in mechanism for such > caching which it uses for caching the Inflaters needed for each new input > stream. In order to cache the buffers as well, I had to add a new , > package-private constructor to InflaterInputStream. I'm not sure if it > makes sense to make this new constructor public, to enable other users of > InflaterInputStream to pre-allocate the buffer. If you think so, I'd be > happy to do that change and open a CSR for this issue. > > Adding a cache for input stream buffers increases the speed of reading > ZipEntries from an InputStream by roughly 5% (see benchmark results below). > More importantly, it also decreases the memory consumption for each call to > ZipFile.getInputStream() which can be quite significant if many ZipEntries > are read from a ZipFile. One visible effect of caching the input buffers is > that the manual JTreg test java/util/zip/ZipFile/TestZipFile.java, which > regularly failed on my desktop with an OutOfMemoryError before, now > reliably passes (this tests calls ZipFile.getInputStream() excessively). > > I've experimented with different buffer sizes (even with buffer sizes > depending on the size of the compressed ZipEntries), but couldn't see any > difference so I decided to go with a default buffer size of 65536 which > already was the maximal buffer size in use before my change. > > I've also added a shortcut to Inflater which prevents us doing a native > call down to libz's inflate() method every time we call Inflater.inflate() > with "input == ZipUtils.defaultBuf" which is the default for every newly > created Inflater and for Inflaters after "Inflater.reset()" has been called > on them. > > Following some JMH benchmark results which show the time and memory used to > read all bytes from a ZipEntry before and after this change. The 'size' > parameter denotes the uncompressed size of the corresponding ZipEntries. > > In the "BEFORE" numbers, when looking at the "gc.alloc.rate.norm" values, > you can see the anomaly caused by using CENLEN instead of CENSIZ in > ZipFile.getInputStream(). I.e. getInputStream() chooses to big buffers > because it looks at the uncompressed ZipEntry sizes which are ~ 6 times > bigger than the compressed sizes. Also, the old implementation capped > buffers bigger than 65536 to 8192 bytes. > > The memory savings for a call to getInputStream() are obviously the effect > of repetadly calling getInputStream() on the same zip file (becuase only in > that case, the caching of the input buffers pays of). But as I wrote > before, I think it is common to have mor then a few entries in a zip file > and even if not, the overhead of caching is minimal compared to the > situation we had before the change. > > Thank you and best regards, > Volker > > = BEFORE 8244659 = > Benchmark (size) > Mode Cnt Score Error Units > ZipFileGetInputStream.readAllBytes 1024 > avgt3 13.577 ± 0.540 us/op > ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm 1024 > avgt3 1872.673 ± 0.317B/op > ZipFileGetInputStream.readAllBytes:·gc.count 1024 > avgt3 57.000counts > ZipFileGetInputStream.readAllBytes:·gc.time 1024 > avgt3 15.000ms > ZipFileGetInputStream.readAllBytes 4096 > avgt3 20.938 ± 0.577 us/op > ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm 4096 > avgt3 4945.793 ± 0.493B/op > ZipFileGetInputStream.readAllBytes:·gc.count
RFR(s): 8244659: Improve ZipFile.getInputStream
Hi, can I please have a review for the following small enhancement which improves the speed of reading from ZipFile.getInputStream() by ~5%: http://cr.openjdk.java.net/~simonis/webrevs/2020/8244659/ https://bugs.openjdk.java.net/browse/JDK-8244659 ZipFile.getInputStream() tries to find a good size for sizing the internal buffer of the underlying InflaterInputStream. This buffer is used to read the compressed data from the associated InputStream. Unfortunately, ZipFile.getInputStream() uses CENLEN (i.e. the uncompressed size of a ZipEntry) instead of CENSIZ (i.e. the compressed size of a ZipEntry) to configure the input buffer and thus unnecessarily wastes memory, because the corresponding, compressed input data is at most CENSIZ bytes long. After fixing this and doing some benchmarks, I realized that a much bigger problem is the continuous allocation of new, temporary input buffers for each new input stream. Assuming that a zip files usually has hundreds if not thousands of ZipEntries, I think it makes sense to cache these input buffers. Fortunately, ZipFile already has a built-in mechanism for such caching which it uses for caching the Inflaters needed for each new input stream. In order to cache the buffers as well, I had to add a new , package-private constructor to InflaterInputStream. I'm not sure if it makes sense to make this new constructor public, to enable other users of InflaterInputStream to pre-allocate the buffer. If you think so, I'd be happy to do that change and open a CSR for this issue. Adding a cache for input stream buffers increases the speed of reading ZipEntries from an InputStream by roughly 5% (see benchmark results below). More importantly, it also decreases the memory consumption for each call to ZipFile.getInputStream() which can be quite significant if many ZipEntries are read from a ZipFile. One visible effect of caching the input buffers is that the manual JTreg test java/util/zip/ZipFile/TestZipFile.java, which regularly failed on my desktop with an OutOfMemoryError before, now reliably passes (this tests calls ZipFile.getInputStream() excessively). I've experimented with different buffer sizes (even with buffer sizes depending on the size of the compressed ZipEntries), but couldn't see any difference so I decided to go with a default buffer size of 65536 which already was the maximal buffer size in use before my change. I've also added a shortcut to Inflater which prevents us doing a native call down to libz's inflate() method every time we call Inflater.inflate() with "input == ZipUtils.defaultBuf" which is the default for every newly created Inflater and for Inflaters after "Inflater.reset()" has been called on them. Following some JMH benchmark results which show the time and memory used to read all bytes from a ZipEntry before and after this change. The 'size' parameter denotes the uncompressed size of the corresponding ZipEntries. In the "BEFORE" numbers, when looking at the "gc.alloc.rate.norm" values, you can see the anomaly caused by using CENLEN instead of CENSIZ in ZipFile.getInputStream(). I.e. getInputStream() chooses to big buffers because it looks at the uncompressed ZipEntry sizes which are ~ 6 times bigger than the compressed sizes. Also, the old implementation capped buffers bigger than 65536 to 8192 bytes. The memory savings for a call to getInputStream() are obviously the effect of repetadly calling getInputStream() on the same zip file (becuase only in that case, the caching of the input buffers pays of). But as I wrote before, I think it is common to have mor then a few entries in a zip file and even if not, the overhead of caching is minimal compared to the situation we had before the change. Thank you and best regards, Volker = BEFORE 8244659 = Benchmark (size) Mode Cnt Score Error Units ZipFileGetInputStream.readAllBytes 1024 avgt3 13.577 ± 0.540 us/op ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm 1024 avgt3 1872.673 ± 0.317B/op ZipFileGetInputStream.readAllBytes:·gc.count 1024 avgt3 57.000counts ZipFileGetInputStream.readAllBytes:·gc.time 1024 avgt3 15.000ms ZipFileGetInputStream.readAllBytes 4096 avgt3 20.938 ± 0.577 us/op ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm 4096 avgt3 4945.793 ± 0.493B/op ZipFileGetInputStream.readAllBytes:·gc.count 4096 avgt3102.000counts ZipFileGetInputStream.readAllBytes:·gc.time 4096 avgt3 25.000ms ZipFileGetInputStream.readAllBytes 16384 avgt3 51.348 ± 2.600 us/op ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm 16384 avgt3
