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

[Peter Levart]

On 05/19/2016 06:31 PM, Xueming Shen wrote:
>
> 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?

It would perhaps make more sense to keep a 32k native object for each 
thread - not each jar file, don't you think?

Regards, Peter

>
> -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);
>
>