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.lev...@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.lev...@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);
>
>
>
>
