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.lev...@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.lev...@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
