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

[Xueming Shen]

Neil,

Regression test /test/java/util/zip/ZipFile/FinalizeInflater.java failed 
with this fix.

The possible solution I can think of now is to do nothing but simply 
return the inflater back
to the cache, but do an additional sanity check when "check out"

private Inflater getInflater() {

     synchronized (inflaters) {
         int size = inflaters.size();
         while ((size = inflaters.size())>  0) {
             Inflater inf = inflaters.remove(size - 1);
             if (!inf.ended())
                 return inf;
             }
         return new Inflater(true);
     }
}


http://cr.openjdk.java.net/~sherman/7031076/webrev/

What do you think? have a better idea?

Sherman


On 04-08-2011 5:36 AM, Neil Richards wrote:
> On Thu, 2011-04-07 at 16:02 -0700, Xueming Shen wrote:
>> It appears it might not be necessary to do the finalize() in
>> ZipFileInflaterInputStream. The ZipFileInflaterInputStream itself does
>> not directly hold any native resource by itself that needs to be
>> released at the end of its life circle, if not closed explicitly. The
>> native resource/memory that need to be taken care of are held by its
>> fields "inf" and "zfin", which should be finalized by the
>> corresponding finalize() of their own classes (again, if not closed
>> explicitly), when their outer ZFIIS object is unreachable. The Inflater
>> class has its own finalize() implemented already to invoke its cleanup
>> method end(), so the only thing need to be addressed is to add the
>> finalize() into ZipFileInputStream class to call its close(), strictly
>> speaking this issue is not the regression caused by #6735255, we have
>> this "leak" before #6735255.
>>
>> Also, would you like to consider to use WeakHeapMap<InputStream, Void>
>> instead of handling all the weak reference impl by yourself, the bonus
>> would be that the stalled entries might be cleaned up more frequently.
>>
> Hi Sherman,
> Thanks for your continuing analysis of this change.
>
> I concur with your assessment above, and agree that making the suggested
> modifications to the changeset results in the code being simpler and
> clearer.
>
> Please find below an updated changeset incorporating these suggestions
> (and rebased off jdk7-b136),
>
> Let me know if you need anything else to progress this fix forward,
> Thanks,
> Neil
>