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

[Xueming Shen]

Martin,

Given we now only cache one deflater per Zip/JarFile object, is 
WeakReference here really
necessary? Basically wr is based on the vm heap memory and deflater is a 
native memory,
arguably we are using the wrong measurement to decide whether or not to 
give up the
deflater's native memory. Given someone (classloader) is keeping 
hundreds and thousands
of jar/zip files alive, is it reasonable to assume it'd better to keep 
the 32k cache for each
of them?

-Sherman

On 5/19/16 8:00 AM, Martin Buchholz wrote:
> On Thu, May 19, 2016 at 7:29 AM, Peter Levart <peter.levart at gmail.com> wrote:
>> 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.
> That's my assumption.  In most cases, failure to close something that
> can be closed is a bug.
> If there's code in the JDK that fails to do that, it should be fixed
> independently.
>
>> 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.
> Yeah, that's a bug. We can only assert after we verify that the
> Inflater is still weakly reachable.
> Updating my webrev with:
>
>        * Releases the Inflater for possible later reuse.
>        */
>       private static void releaseInflater(Inflater inf) {
> -        assert !inf.ended();
>           CachedInflater cachedInflater = inflaterCache.get();
>           if (inf != cachedInflater.get()) {
>               inf.end();
>           } else {
>               // "return" the Inflater to the "pool".
> +            assert !inf.ended();
>               inf.reset();
>               assert cachedInflater.inUse.get();
>               cachedInflater.inUse.set(false);