RFR: 8156484: ZipFile retains too much native memory from caching Inflaters

[Peter Levart]

Hi Martin,


On 05/19/2016 04:27 AM, Martin Buchholz wrote:
> Another batch of ZipFile hackery.  I think I finally understand how
> weak references and finalizers interact.
> We could eliminate the Inflater cache entirely, but this proposal
> keeps a one-element Inflater cache.
>
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ZipFile-Inflater-cache/
> https://bugs.openjdk.java.net/browse/JDK-8156484
>
> - fixes the memory bloat from cached Inflaters
> - removes small races in close
> - various other minor improvements

The fixes to races are (almost) good. Also other minor improvements and 
cleanups.

But I have reservation for the implementation of one-element global 
cache of Inflater. This cache can't be efficient. In order for cache to 
be efficient, majority of calls to ZipFile.getInputStream(zipEntry) 
would have to be accompanied by a corresponding explicit close() for the 
input stream before the WeakReference to the cached Inflater is cleared. 
We know that all WeakReference(s) to a referent are 1st cleared and then 
the finalize() method is called on it. GC clears all WeakReference(s) to 
interconnected graph of weakly-reachable referents atomically, so if the 
ZipFileInflaterInputStream is closed implicitly by the finalize() 
invocation which then tries to releaseInflater(), the inflater will not 
be released as the 'cachedInflater' WeakReference will already be 
cleared. It is too late to release inflater into cache anyway because if 
the input stream is already being finalized, then the associated 
inflater has either already been finalized or will be shortly as GC 
discovers and enqueues the FinalReference(s) pointing to inflater and 
corresponding input stream in the same GC cycle - Inflater and 
corresponding input stream become eligible for finalization at the same 
time. The "assert !inf.ended()" in releaseInflater() can therefore fail 
as final() methods on individual objects that are eligible for 
finalization may be invoked in arbitrary order.

To correct that and improve the efficiency of cache, we have to do two 
things:

- we can't fix all the usages of ZipFile.getInputStream(zipEntry) out 
there, but we can at least fix the usages in URLClassLoader and 
BuiltinClassLoader which amount to most getInputStream() invocations 
without corresponding close()s when classes are being loaded (one stream 
for each class).
- we can make the Inflater cache a bit smarter.

http://cr.openjdk.java.net/~plevart/jdk9-dev/ZipFile_Inflater.cleanup/webrev.01/

We don't actually need to cache the Inflater objects themselves, but 
rather Inflater's internal ZStreamRef(s) which are just objects holding 
address to native zlib's z_stream structure. I also suggest using new 
Cleaner API for tracking reachability of Inflater(s) so that 
Inflater.end() that results from explicit stream close() clears the 
PhantomCleanable and allows GC to refrain from pushing already end()-ed 
Inflater(s)/FinalReference(s) through the finalization pipeline. I also 
took the liberty to create a trusted package-private InflaterInputStream 
constructor so that trusted subclasses can use it and so that 
usesDefaultInflater flag can be private and final. Deflater is using 
ZStreamRef(s) too, so I also had to modify Deflater, but there's no 
caching of ZStreamRef(s) in deflater as there are too many different 
kinds of them and this is not critical.

By making ZStreamRef(s) take care of themselves, corresponding logic in 
ZipFile's streams is much simpler - no need for finalize() method any 
more on ZipFileInflaterInputStream. finalize() is also not needed on 
ZipFileInputStream any more as 'streams' WeakHashMap is not holding 
Inflater instances as values any more and so finalize() would serve only 
to remove finalized ZipFile[Inflater]InputStream(s) from the WeakHashMap 
which the map is doing by itself already (expunging).

java.util.zip tests pass with this patch except 2 tests that are ignored 
and ZipFile/TestZipFile.java that prints:

java.lang.OutOfMemoryError: Java heap space: failed reallocation of 
scalar replaced objects

...which seems unrelated and the test passes when executed alone.

This touches quite a number of other classes, but it seems necessary.

So Martin, what do you think?

Regards, Peter