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