Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
xueming.shen at oracle.com
Mon Apr 18 17:32:56 UTC 2011
All tests passed.
I'm starting to push your last patch. I generated the webrev at
It should be exactly the same as your last patch.
On 4/18/2011 9:49 AM, Xueming Shen wrote:
> On 4/18/2011 5:33 AM, Neil Richards wrote:
>> On Thu, 2011-04-14 at 14:48 -0700, Xueming Shen wrote:
>>> Opinion? anything I overlooked/missed?
>> Hi Sherman,
>> Thanks once more for all your help and advice on this - I'm in favour of
>> almost all of what you suggest. :-)
>> I think it's worthwhile trying to clear 'streams' entries from a
>> finalize method in ZFIIS, to help ensure they are cleared in a timely
>> I don't think the Inflater objects need to be held softly in
>> 'inflaterCache', as they will have been reset() at the point they are
>> put into that cache (in releaseInflater()).
>> (I was holding them softly due to a worry over any delay in clearing
>> their 'buf' reference, which is done when reset() is called. Now that
>> the 'streams' entries are being cleared from close() and finalize() -
>> ie. up front - I think this worry is adequately addressed.)
>> I'm worried about changing the object 'streams' refers to in
>> ZipFile.close(), as threads are using that object to synchronize
>> I don't believe it is currently giving the guarantee against
>> ConcurrentModificationException being seen from the iterator that I
>> believe you intend it to be, as other threads, calling (for example)
>> ZFIIS.close() at the same time will not be guaranteed to see the update
>> to the value of 'streams'.
>> Instead, I've modified this code so that a real copy of 'streams' is
>> produced, by calling 'new HashMap<>(streams)'. It is this copy that is
>> iterated through to close() the InputStreams and end() the Inflaters.
>> Once the copy is obtained, 'streams' can be safely clear()'d.
>> Because it is not clear that a Collections.synchronizedMap will give
>> consistent results when fed into the constructor of HashMap, I've used
>> explicit synchronization on 'streams' instead, to ensure 'streams' isn't
>> modified during the construction of 'copy'.
>> As each of the calls to InputStream.close() will synchronize on
>> 'streams' to call its remove() method, I've held that monitor whilst
>> those calls are made.
> (resent, got some mail issue)
> Hi Neil,
> If you are now explicitly synchronize on "streams" everywhere, I don't
> think we even need a copy
> at close().
> To add "close() in ZFIIS.finalize() appears to be safe now (?, maybe I
> should think it again), but
> given we are now using WeakHashMap, those weak reference will get
> expunged every time that
> map gets accessed, I doubt that really help lots (have to wait for the
> GC kicks in anyway, if not
> closed explicitly)
> I'm running tests now, with the "copy" mentioned above.
>> Other minor tweaks are changing 'isClosed' fields to
>> 'closeRequested' (to better describe their use now), changing
>> 'ZipFile.closeRequested' to be volatile (so it can be checked before
>> synchronization, and so that it conforms to the pattern now established
>> elsewhere in the file), and reducing the synchronization block in
>> releaseInflater() (only the call to 'inflateCache.add()' need be
>> protected by synchronization on 'inflaterCache').
>> Please find below the resultant changeset,
>> Let me know what you think,
More information about the core-libs-dev