Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
xueming.shen at oracle.com
Mon Apr 18 16:49:00 UTC 2011
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)
If you are now explicitly synchronize on "streams" everywhere, I don't
think we even need a copy
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
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