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