Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
spoole at linux.vnet.ibm.com
Wed Apr 13 15:25:06 UTC 2011
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
>> "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
>> 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?
>> 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
>>>> 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
>>> and having started to be used by that other stream at the point at
>>> 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
>>> 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,
More information about the core-libs-dev