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
