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

[Martin Buchholz]

Peter and Sherman,

I'm still quixotic fighting java memory bloat.  Caching Inflaters at
all is only barely profitable; a one-element Inflater cache is
probably fine for those apps that occasionally iterate through the
classpath.  I don't want ThreadLocal bloat; I want _all_ the Inflaters
to go away by themselves after a while e.g. in a long-running server.
I'm open to having the cache contain more than one element, as long as
they go away eventually.  If so, I would choose a design based on
linked nodes, still with a WeakReference to make them go away.

On Thu, May 19, 2016 at 9:49 AM, Peter Levart <peter.levart at gmail.com> wrote:
>
>
> 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);
>
>
>
>