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

Hi Sherman, Martin, On 05/23/2016 08:28 PM, Xueming Shen wrote: My apology. I meant to say "to have Inflater to be aware of the ObjectPool...". The proposed change switches from the finalizer to the ObjectPool to clean up the native resource for Inflater. It appears to be a bigger change.

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

On Mon, May 23, 2016 at 12:55 PM, Xueming Shen wrote: > looks fine. i just pushed in a change. so you may need a sync :-) Thanks - pushed.

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

looks fine. i just pushed in a change. so you may need a sync :-) On 5/23/16, 12:49 PM, Martin Buchholz wrote: The utterly boring parts of my changes are in http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ZipFile-tidy/ Can we get these in now?

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

The utterly boring parts of my changes are in http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ZipFile-tidy/ Can we get these in now?

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

On Mon, May 23, 2016 at 11:28 AM, Xueming Shen wrote: > The proposed change switches from the finalizer to the ObjectPool to clean > up > the native resource for Inflater. It appears to be a bigger change. Which > has > a default 32/maxCacheSize and 1 secod keepAliveTime

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

On 05/23/2016 11:00 AM, Peter Levart wrote: Hi Sherman, On 05/23/2016 05:44 PM, Xueming Shen wrote: It appears the scope of the change is bigger than the problem we are trying to solve here. The ClassLoader(s)/Resource changes are there just to measure the impact if input streams opened

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

Hi Sherman, On 05/23/2016 05:44 PM, Xueming Shen wrote: It appears the scope of the change is bigger than the problem we are trying to solve here. The ClassLoader(s)/Resource changes are there just to measure the impact if input streams opened from a jar file are closed explicitly as soon as

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

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

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

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

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

Peter, You keep impressing me with your development speed! Looking at ObjectPool ... looking pretty good keepalivetime should be > 0. (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.

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

Hi Martin, On 05/20/2016 02:51 AM, Martin Buchholz wrote: On Thu, May 19, 2016 at 7:29 AM, Peter Levart 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

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

On 20/05/2016 07:25, Peter Levart wrote: Hi Martin, On 05/20/2016 12:04 AM, Martin Buchholz wrote: Hi Peter, I would make e.g. the change to Resource an independent change. I agree. It's just that if we measure the impact of ZipFile changes to class loading, for example, we should

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

Hi Martin, On 05/20/2016 12:04 AM, Martin Buchholz wrote: Hi Peter, I would make e.g. the change to Resource an independent change. I agree. It's just that if we measure the impact of ZipFile changes to class loading, for example, we should measure them with this change included so that

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

On 5/19/16 5:51 PM, Martin Buchholz wrote: On Thu, May 19, 2016 at 7:29 AM, Peter Levart 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

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

On Thu, May 19, 2016 at 7:29 AM, Peter Levart 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,

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

Hi Peter, I would make e.g. the change to Resource an independent change. Below you declare that you throw IOException, but you actually swallow it, which is not good. /** + * Closes cached input stream used for getBytes / getByteBuffer + * @throws IOException + */ +

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

Peter and Sherman, I'm still quixotic fighting java memory bloat. Caching Inflaters at all is only barely profitable; a one-element Inflater cache is probably fine for those apps that occasionally iterate through the classpath. I don't want ThreadLocal bloat; I want _all_ the Inflaters to go

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

On 05/19/2016 06:31 PM, Xueming Shen wrote: Martin, Given we now only cache one deflater per Zip/JarFile object, is WeakReference here really necessary? Basically wr is based on the vm heap memory and deflater is a native memory, arguably we are using the wrong measurement to decide

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

Martin, Given we now only cache one deflater per Zip/JarFile object, is WeakReference here really necessary? Basically wr is based on the vm heap memory and deflater is a native memory, arguably we are using the wrong measurement to decide whether or not to give up the deflater's native

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

Hi Martin, To make it simpler to compare your and mine changes, here's the diff between your changed ZipFile.java and mine (mostly just removal of code): diff -r 45dcd8ef14a7 src/java.base/share/classes/java/util/zip/ZipFile.java --- a/src/java.base/share/classes/java/util/zip/ZipFile.java

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

On Thu, May 19, 2016 at 7:29 AM, Peter Levart wrote: > But I have reservation for the implementation of one-element global cache of > Inflater. This cache can't be efficient. In order for cache to be efficient, > majority of calls to ZipFile.getInputStream(zipEntry) would

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

Hi Martin, On 05/19/2016 04:27 AM, Martin Buchholz wrote: Another batch of ZipFile hackery. I think I finally understand how weak references and finalizers interact. We could eliminate the Inflater cache entirely, but this proposal keeps a one-element Inflater cache.