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

[Xueming Shen]

Hi Neil,

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

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

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

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