Steve, Neil,
As we discussed in previous emails that Neil's last patch (to put the
inflater into the "streams" as
the value of the map) does solve the FinalizeInflater failure issue
(provides a "orderly" final cleanup
between the "stream" and the "inflater". However to leave
releaseInfalter() to be solely driven
by GC might delay the inflater to be cached/re-used in a timely manner,
even in scenario that
those streams get closed explicitly by the invoker. Ideally it is
desirable that the inflater can be
re-used/put back into the cache when ZFIIS.close() gets invoked explicitly.
It appears it might be too hard, if not impossible, to arrange the
releaseInflater() to be invoked from
both clearStaleStreams()/staleStreamQueue and ZFIIS.close(), given the
non-guarantee, by the VM/GC,
of the timing/order among the clear up of weak reference objects,
enqueue them and the invocation of
finalize(). So I would suggest a relatively simple solution as showed in
webrev at
http://cr.openjdk.java.net/~sherman/7031076/webrev/src/share/classes/java/util/zip/ZipFile.java.sdiff.html
(1) simply use the WeakHashMap to keep the <InputStream, Inflater> pair,
we don't try
to release/put the used inflater back to cache, if the ZFIIS
stream does not get closed explicitly
(its underlying ZFIS stream and the inflater will then get
finalized by themself).
(2) release the inflater back to cache in ZFIIS.close(), only if the
inflater is sill "alive" in the
"streams", which guarantees that the inflater is not being
finalized and safe to put back
to cache (so long as we have the weak reference -> inflater pair
in "streams", it does not
matter if the reference to ZFIIS object is cleared or not). In
most cases, I would expect
when the ZFIIS.close() is invoked, both ZFIIS object and its
inflater are still reachable, so
the inflater gets re-used as expected. In "weird" scenario like
what we have in
FinalizeInflater, the ZFIIS object and its outer wrapper object
are being finalized when
ZFIIS.close() gets invoked (from wrapper.finalize()), there is
possibility that the inflater might
not be put back to cache, if the weak reverence->inflater pair has
been expunged out of the
"streams" already (because the weak reference to ZFIIS might have
been cleared, so its
entry in "streams" can be expunged any time, by any thread. At
this moment, the inflater
object might be declared as "un-reachable" and no longer safe to
put back into cache), but
I think we can live with this "missing".
(3) I realized that we actually are not removing the closed ZFIIS
objects (only the ZFIS objects
are removed when closed) from "streams" even though those input
streams are closed
gracefully by the invoker (as preferred, suggested, desired),
which means all those ZFIIS
streams will stay in "streams" (strong referenced in current
implementation), no matter
whether or not they get closed explicitly, until the ZipFile
object itself gets closed/finalized.
This might contribute to the problem Neil observed at first place
(the ZipFile InputStreams
accumulat in heap), actually I do have another incident report
#7033523 which I have
closed as the dup of #7031076.
Opinion? anything I overlooked/missed?
-Sherman
On 04/13/2011 08:25 AM, Steve Poole wrote:
On 13/04/11 16:19, Steve Poole wrote:
On 12/04/11 20:33, Xueming Shen wrote:
Hi Neil,
Hi Sherman , Neil is out on vacation so I will do my best to stand
in for him.
(1) I believe it would be better to keep the synchronization lock
for get/releaseInfalter()
"local" instead of using the "global" ZipFile.this, which I
agree is "simple". But it also
means each/every time when you release the used inflater back
to cache, ZipFile.this
has to be held and any possible/potential read operation on
ZipFile from other thead/
inputstream has to wait ("get" seems fine, the current impl
holds the ZipFile.this
anyway before reach the getInflater()).
Ok - I agree - seems sensible. (Though I reserve the right to
contradict myself later)
(2) The "resource" Infalter mainly holds is the native memory block
it alloc-ed at native
for zlib, which is not in the Java heap, so I doubt making it
"softly" really helps for GC.
Sure, cleanup of those "objects" themself is a plus, but I'm
not sure if it is really worth
using SoftReference in this case (it appears you are now
invoking clearStaleStreams()
from the finalize()).
I'd like to keep this in - not all implementations of zlib are equal
in where they allocate memory.
(3) The releaseInfalter() is now totally driven from
clearStaleStreams()/staleStreamQueue,
which is under full control of GC. If GC does not kick in for
hours/days, infalters can never
be put back into its cache after use, even the applications
correctly/promptly close those
streams after use. And when the GC kicks in, we might see
"bunch" (hundreds, thousands)
of inflaters get pushed into cache. The approach does solve the
"timing" issue that got
us here, but it also now has this "native memory cache"
mechanism totally under the
control of/driven by the GC, the java heap management
mechanism, which might not be
a preferred scenario. Just wonder if we can have a better
choice, say with this GC-driven
as the backup cleanup and meanwhile still have the
ZFIIS.close() to do something to safely
put the used inflater back to cache promptly. To put the
infalter as the value of the "streams"
map appears to be a good idea/start, now the "infalter" will
not be targeted for finalized
until the entry gets cleaned from the map, in which might in
fact provide us a sort of
"orderly" (between the "stream" and its "inflater") clearup
that the GC/finalizer can't
guarantee. We still have couple days before we hit the
"deadline" (to get this one in), so it
might worth taking some time on this direction?
Dang! - pressed send when I meant save. I understand your comments -
on the face of it I agree with what you're suggesting - let me think
it through some more though.
What is this "deadline" you are talking about?
-Sherman
On 04/11/2011 05:15 AM, Neil Richards wrote:
On Sun, 2011-04-10 at 18:28 +0100, Neil Richards wrote:
With releaseInflater() being driven from the cleanup of stale
'streams'
entries, it no longer needs to be called from ZFIIS.close(), so,
instead, only Inflater.reset() is called from there (providing the
inflater object has not explicitly been ended) so that it releases
the
buffer it has been holding.
Actually, I have a slight change of heart on this aspect.
Because close() may be driven from a finalize() method in user code
(as
is done in the InflaterFinalizer test case), I believe it is possible
(albeit unlikely) for an inflater object to have been reclaimed from
'streams' (by a call to clearStaleStreams()), put into the inflater
cache, retrieved from there (by another thread creating an input
stream)
and having started to be used by that other stream at the point at
which
the code in close() is run.
(This is because weak references will be cleared, and *may* be
queued on
the ReferenceQueue, prior to finalization).
Because of this, it's not actually entirely safe now to call
inf.reset()
from ZFIIS.close().
Instead, I have added additional calls to clearStaleStreams() from the
finalize() methods of both InputStream implementations in the latest
(hopefully in the meaning of "last") changeset included below.
As always, please get back to me with any comments, questions or
suggestions on this,
Thanks,
Neil
