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

[Xueming Shen]

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.

Hi Steve,

It might be better/appropriate to use the SoftReference in the 
"inflaterCache" instead of the
"streams", if we want to use the SoftReference for inflater caching purpose.

     /*
      * Gets an inflater from the list of available inflaters or allocates
      * a new one.
      */
     private synchronized Inflater getInflater() {
         SoftReference<Inflater> sref;
         Inflater inf;
         while (null != (sref = inflaterCache.poll())) {
             if ((inf = sref.get()) != null && !inf.ended()) {
                 return inf;
             }
         }
         return new Inflater(true);
     }

     /*
      * Releases the specified inflater to the list of available inflaters.
      */
     private synchronized void releaseInflater(Inflater inf) {
         if ((null != inf) && (false == inf.ended())) {
             inf.reset();
             inflaterCache.add(new SoftReference(inf));
         }
     }

     // List of available Inflater objects for decompression
     private Deque<SoftReference<Inflater>> inflaterCache = new 
ArrayDeque<>();

-Sherman

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