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

[Peter Levart]

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

Thanks.

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