Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand

[Steve Poole]

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
>>>
>>
>