RFR: 8156484: ZipFile retains too much native memory from caching Inflaters

[Xueming Shen]

It appears the scope of the change is bigger than the problem we are 
trying to solve here.

The problem we are having here is that in certain use scenario the vm 
keeps a huge number
of zip/jar files open and in which makes even the minimal cache 
mechanism (keep even one
deflater for one zip file) not desirable.

It might be desirable to have a better cache mechanism, such as the 
"ObjectPool" approach you're
proposing here, to help in this situation. But I doubt if it is 
necessary to extend the "functionality"
to the rest of the package. For example, In most use scenario, we don't 
have such cache issue with
the deflater/inflater, a usual "open-end+finalize()" mechanism appears 
just fine for now, I don't
see a compelling reason to replace the existing mechanism with a new 
one. It might be better
to be conservative here to not make big change to something not proven 
broken.

It does not seem really necessary to have Deflater to be aware of the 
"ObjectPool" and the cleaner/
dealloctor, maybe all of these should be kept as an implementation 
inside the ObjectPool?  which
can have a cleaner to just invoke deflater.end() when needed?

-Sherman


On 5/23/16 7:56 AM, Peter Levart wrote:
> Hi Martin,
>
>
> On 05/20/2016 08:53 PM, Martin Buchholz wrote:
>> Peter,
>>
>> You keep impressing me with your development speed!
>> Looking at ObjectPool ... looking pretty good
>
> Thank
>
>>
>> keepalivetime should be > 0.
>
> Right. ScheduledThreadPoolExecutor.scheduleWithFixedDelay throws 
> IllegalArgumentException when the 'delay' argument is not positive. 
> ObjectPool should have the same check.
>
>>   (also very small keepalive times are a
>> bad idea, but what's "very small"?)
>> I'm thinking one second for ZipFile keep alive time.  Thread pools
>> have 60 seconds.
>
> Exactly. I set 1 second of keep-alive for the two pools in Inflater.
>
>>
>> Maybe we need a Consumer<T> resetter in addition to a releaser.
>
> I've been thinking of that too. It makes the usages nicer as they 
> don't have to deal with resetting the object before returning it to 
> the pool. In addition, the pool could reset the object lazily just 
> before handing it out again. The "resetter" could also serve as an 
> object "validator" - either being a Predicate or a Consumer throwing 
> an exception. Thinking of objects like Connection(s). In case 
> validation fails, the pool would release the object and try another one.
>
>>
>> I'm opposed to creating a new STPE.  jdk 9 CompletableFuture comes
>> with a hidden thread scheduler, so the idea is to use delayedExecutor
>> to share that one thread for all common scheduling.
>
> Nice. It even simplifies the ObjectPool that way. I peeked at code and 
> I see it uses an internal shared STPE under the hood.
>
>>
>> I think you want to call pop poll instead, since it can return null?
>
> That's better yes.
>
>>
>> We want a purger task to be created at most once per keep alive
>> period, and never after the pool is quiescent.  Not sure if we're
>> there yet.
>
> The task itself can be reused, yes. I wanted to respect the 
> keepAliveTime so that pooled objects are released as soon as 
> keepAliveTime is reached after last object was returned to the pool. I 
> also wanted, as optimization, to schedule a task for periodic 
> invocation, which only guarantees that objects are released sometime 
> between keepAliveTime ... 2 * keepAliveTime after the last object is 
> returned to the pool. But then I realized that periodic scheduling 
> with constant delay is just a fancy API facade for scheduling next 
> invocation at the end of the previous one. In either case the internal 
> "heap" queue has to be updated at every firing of the task.
>
> So this is much simpler now with CompletableFuture:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/ZipFile_Inflater.cleanup/webrev.03/ 
>
>
> The only change from webrev.02 is in ObjectPool implementation.
>
> java/util/zip tests pass with this patch. The TestZipFile.java was 
> failing for me with OOME and I had to increase the heap to 4G to make 
> it pass. Heap dump shows that the test really needs that much. It 
> keeps ZipEntries arround in a LinkedHashMap with associated byte[] 
> arrays holding the contents of the zip-ed files.
>
> So, Martin, what do you think of this one?
>
> Regards, Peter
>
>>
>> On Fri, May 20, 2016 at 7:31 AM, Peter Levart 
>> <peter.levart at gmail.com> wrote:
>>> Hi Martin,
>>>
>>>
>>>
>>> On 05/20/2016 02:51 AM, Martin Buchholz wrote:
>>>> On Thu, May 19, 2016 at 7:29 AM, Peter Levart <peter.levart at gmail.com>
>>>> wrote:
>>>>> So Martin, what do you think?
>>>> I studied your code and we are fundamentally in agreement!
>>>> ... Except for need for periodic cleanup of the cache...
>>>> Maybe we should do
>>>> CompletableFuture.runAsync(purgeCache,
>>>> CompletableFuture.delayedExecutor(1, SECONDS))
>>>> as a periodic task while cache is non-empty instead of my strategy of
>>>> relying on a weak reference.
>>>>
>>>> Purging a cache on quiescence in the optimal way seems to be a hard
>>>> problem.
>>>> Time to write ObjectPool<T> (as many others have done before us)?
>>>>
>>>> ObjectPool(Supplier<T> factory,  Consumer<T> releaser, maxCacheSize,
>>>> keepAliveTime)
>>>>
>>>> Except this time we'll do it right!  With jdk9 machinery!
>>>
>>> Something like the following?
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/ZipFile_Inflater.cleanup/webrev.02/ 
>>>
>>>
>>> I still must debug the cause of OutOfMemoryError in
>>> java/util/zip/ZipFile/TestZipFile.java.
>>>
>>> Regards, Peter
>>>
>