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

[Xueming Shen]

On 05/23/2016 11:00 AM, Peter Levart wrote:
> Hi Sherman,
>
> On 05/23/2016 05:44 PM, Xueming Shen wrote:
>> It appears the scope of the change is bigger than the problem we are trying to solve here.
>
> The ClassLoader(s)/Resource changes are there just to measure the impact if input streams opened from a jar file are closed explicitly as soon as possible when loading classes. This makes the cache/pool of Inflater's native resources more efficient. Those changes are not meant to be included in this patch - will be stripped off and proposed as a separate enhancement later.
>
> There are also cleanups that are not absolutely necessary such as InflaterInputStream's package-private constructor with trusted subclasses using it so that usesDefaultInflater flag can be private and final in InflaterInputStream. They can be reverted if you don't like them.
>
>>
>> The problem we are having here is that in certain use scenario the vm keeps a huge number
>> of zip/jar files open and in which makes even the minimal cache mechanism (keep even one
>> deflater for one zip file) not desirable.
>
> Right. Even if vm keeps huge number of jar files open, it doesn't read from all of them at the same time, but typically a single thread only reads from one entry of one jar file at a time before going to the next one. So keeping an isolated cache of native resource that could be re-used to read from multiple jar files, per jar file, is not the most efficient way to cache it. It doesn't matter until the number of jar files is large and VMs with gigantic classpaths are an important use case.
>
>>
>> It might be desirable to have a better cache mechanism, such as the "ObjectPool" approach you're
>> proposing here, to help in this situation. But I doubt if it is necessary to extend the "functionality"
>> to the rest of the package. For example, In most use scenario, we don't have such cache issue with
>> the deflater/inflater, a usual "open-end+finalize()" mechanism appears just fine for now, I don't
>> see a compelling reason to replace the existing mechanism with a new one. It might be better
>> to be conservative here to not make big change to something not proven broken.
>
> As  presented in my 1st message in the thread, what Martin is seeing is a combination of several issues:
>
> - class loading is not closing streams explicitly
> - caching is not effective for Inflaters for which the streams are closed with finalizers because when finalizer invokes close() on stream, the WeakHashMap<InputStream, Inflater> is not keeping the InputStream any more, so the associated Inflater, while maybe still reachable in object graph, can not be retrieved (map.remove(stream) returns null).
> - caching of Inflaters for streams that happen to be explicitly closed is not bounded so they may pile-up.
>
> I claim that we must solve all 3 issues to make Martin happy again. :-)
>
>>
>> It does not seem really necessary to have Deflater to be aware of the "ObjectPool" and the cleaner/
>> dealloctor, maybe all of these should be kept as an implementation inside the ObjectPool?  which
>> can have a cleaner to just invoke deflater.end() when needed?
>
> Deflater is not aware of ObjectPool. It just uses Cleaner instead of finalize() because as a twin brother of Inflater, it would be nice for it to have the same cleanup mechanism. Cleaner is more efficient than finalization when the Deflater is explicitly end()-ed which is also a nice property. Otherwise the mechanism of Cleaner is similar to finalization. Cleaner is using PhantomReference(s) which are cleared when discovered and enqueued, while finalization is using FinalReference(s) which are not cleared and so present additional burden to CPU even in cases where Deflater is not explicitly end()ed. But if you don't like that, the changes to Deflater could be reverted back to using finalize().
>

My apology. I meant to say "to have Inflater to be aware of the ObjectPool...".
The proposed change switches from the finalizer to the ObjectPool to clean up
the native resource for Inflater. It appears to be a bigger change. Which has
a default 32/maxCacheSize and 1 secod keepAliveTime setting. It might have
a big impact to existing applications on how to use the Inflater. It would be
fine to have the ZipFile to arrange how to use the Inflater based on its use
scenario, but I'm concerned if it's the correct approach to change the default
behavior of Inflater, which might have nothing to do with ZipFile.

sherman

> Even Inflater could use finalize() for what it is worth, combined with ObjectPool<ZStreamRef>, but why would we not take an opportunity and replace finalize() as we go? It's plain and simple. And Cleaner API was developed for just that purpose.