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

[Peter Levart]

Hi Martin,

To make it simpler to compare your and mine changes, here's the diff 
between your changed ZipFile.java and mine (mostly just removal of code):


diff -r 45dcd8ef14a7 src/java.base/share/classes/java/util/zip/ZipFile.java
--- a/src/java.base/share/classes/java/util/zip/ZipFile.java    Thu May 
19 17:10:12 2016 +0200
+++ b/src/java.base/share/classes/java/util/zip/ZipFile.java    Thu May 
19 17:12:54 2016 +0200
@@ -31,11 +31,9 @@
  import java.io.EOFException;
  import java.io.File;
  import java.io.RandomAccessFile;
-import java.lang.ref.WeakReference;
  import java.nio.charset.Charset;
  import java.nio.charset.StandardCharsets;
  import java.nio.file.attribute.BasicFileAttributes;
-import java.nio.file.Path;
  import java.nio.file.Files;

  import java.util.ArrayList;
@@ -43,21 +41,18 @@
  import java.util.Enumeration;
  import java.util.HashMap;
  import java.util.Iterator;
-import java.util.Map;
  import java.util.Objects;
  import java.util.NoSuchElementException;
  import java.util.Spliterator;
  import java.util.Spliterators;
  import java.util.WeakHashMap;
  import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicReference;
  import java.util.stream.Stream;
  import java.util.stream.StreamSupport;
  import jdk.internal.misc.JavaUtilZipFileAccess;
  import jdk.internal.misc.SharedSecrets;
  import jdk.internal.perf.PerfCounter;

-import static java.util.zip.ZipConstants.*;
  import static java.util.zip.ZipConstants64.*;
  import static java.util.zip.ZipUtils.*;

@@ -370,7 +365,7 @@
          private boolean eof = false;

          ZipFileInflaterInputStream(ZipFileInputStream zfin, int size) {
-            super(zfin, getInflater(), size);
+            super(zfin, new Inflater(true), size, true);
          }

          public void close() throws IOException {
@@ -379,7 +374,6 @@
                  synchronized (streams) {
                      streams.remove(this);
                  }
-                releaseInflater(inf);
              }
          }

@@ -408,72 +402,6 @@
                  ? Integer.MAX_VALUE
                  : (int) avail;
          }
-
-        protected void finalize() throws Throwable {
-            close();
-        }
-    }
-
-    /**
-     * A "pooled" inflater.  The weak reference does not prevent 
finalization
-     * of the Inflater, but if get() returns non-null, then the referent is
-     * surely not yet eligible for finalization (which would otherwise be a
-     * rich source of bugs).
-     */
-    static class CachedInflater extends WeakReference<Inflater> {
-        final AtomicBoolean inUse;
-        CachedInflater(Inflater inf, boolean inUse) {
-            super(inf);
-            this.inUse = new AtomicBoolean(inUse);
-        }
-    }
-
-    /**
-     * Cache of Inflaters.  We use a simple one-element "pool".
-     * Performance measurements show that it's barely profitable to
-     * cache an Inflater (which consumes around 32kb of native memory)
-     * while iterating through a zip file and decompressing many small
-     * entries.
-     */
-    private static final AtomicReference<CachedInflater> inflaterCache
-        = new AtomicReference<>(new CachedInflater(new Inflater(true), 
false));
-
-    /**
-     * Returns an Inflater, either a new one, or cached and reset.
-     */
-    private static Inflater getInflater() {
-        CachedInflater cachedInflater = inflaterCache.get();
-        Inflater inf = cachedInflater.get();
-        if (inf == null) {
-            inf = new Inflater(true);
-            // attempt to install as the new cache, but failure is OK.
-            inflaterCache.compareAndSet(cachedInflater,
-                                        new CachedInflater(inf, true));
-            return inf;
-        } else if (!cachedInflater.inUse.get()
-                   && cachedInflater.inUse.compareAndSet(false, true)) {
-            assert !inf.ended();
-            // we now "own" this Inflater, but must keep it weakly 
referenced.
-            return inf;
-        } else {
-            return new Inflater(true);
-        }
-    }
-
-    /**
-     * 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".
-            inf.reset();
-            assert cachedInflater.inUse.get();
-            cachedInflater.inUse.set(false);
-        }
      }

      /**
@@ -804,10 +732,6 @@
                  }
              }
          }
-
-        protected void finalize() {
-            close();
-        }
      }

      static {




Regards, Peter

On 05/19/2016 05:00 PM, 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);