Hi Sherman,
On 09/30/17 02:02, Xueming Shen wrote:
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported
exception Throwable; must be caught or declared to be thrown
super.finalize();
^
(the overridden finalize() in InstrumentedZipFile should now declare
throws Throwable, since it overrides Object.finalize() and not
ZipFile.finalize() which is gone).
The other thing I noticed is that Releaser 1st closes the streams
(that are still reachable via streams WeakHashMap) and also ends the
associated inflaters. But closing the stream will already release the
inflater (in case it is a ZipFileInflaterInputStream) into the
inflaters cache and the cache is scanned and inflaters ended later.
So we don't need a stream -> inflater association outside the stream
in the form of WeekHashMap. But we still need to keep the set of
input streams weakly reachable from ZipFile in case we want to close
the ZipFile explicitly (and there is no harm although useless if this
also happens as a result of automatic ZipFile cleaner processing).
This could be implemented in a form of:
final Set<InputStream> streams = Collections.newSetFromMap(new
WeakHashMap<>());
I also noticed that it is useless to test whether the inflater is
ended() when obtaining it from or releasing it into cache if the code
keeps the invariant that it never ends inflaters while they are still
in cache or associated with the open stream (the only place where
inflaters should be ended explicitly is in the Releaser). To make
this even more obvious, it might be good to move the
obtaining/releasing logic directly into the
ZipFileInflaterInputStream constructor which would be passed a
reference to the inflatersCache instead of the Inflater instance.
Here's what I have in mind (I cahnged just the ZipFile and
FinalizeZipFile):
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.01/
What do you think?
Peter,
I read those old emails back to 2011 on why we put the "Inflater" into
the streams as
the value of the map entry. It appears the main (only) purpose is to
keep the finalization
of <stream, inflater> in order. As I explained in previous email,
stream and its inflater
can be phantom reachable at same round, if inflater gets
finalized/ended first, then it
can no longer be reused/cached and has to be thrown away, which means
the caching
mechanism is actually broken/not functioning. We do have use scenario
depends on it
to avoid too many "new Inflater()' that might pressure the memory
usage. Putting the
pair in a weakhashmap appears to solve this problem back then (the
ended() checks are
still there just in case there is rare case, the inflater still gets
cleaned up first).
The situation might be changed now in Cleaner case, as the cleaner
object itself now
holds a strong reference, which in theory will/should prevent the
inflater from being
phantom reachable before stream is cleaned, so we might no longer need
this
<stream, inflater> pair in a map to have a "ordered" cleanup. Just
wanted to make sure
my assumption is correct here.
sherman
Right, the Inflater is captured by the cleaning function for the
ZipFileInflaterInputStream and is kept strongly reachable until the
function fires, which may happen automatically or manually. At that
time, it is returned to the inflaters cache, which is rooted at the
ZipFile instance and also captured by the ZipFile cleaning function -
the Releaser, which is strongly reachable until fired. So I claim that
there is no possibility for Inflaters used in ZipFile to ever get
cleaned-up (ended) automatically by their cleaner functions as a result
of getting phantom reachable. The situation has changed. It could even
be beneficial to create a package-private Inflater constructor for this
case, which creates Inflaters without cleaner registration.
Regards, Peter
