FishEye and Crucible

Hi Laszlo,

As requested in your recent email, I thought that I'd take a look at your patch. I have a couple of general comments in addition to Andi's comments:

1. Have you benchmarked and profiled the existing object pool implementation? Use of the "synchronized" keyword does not imply that a class will perform badly. An application's maximum theoretical throughput is dictated by the size of the largest common critical section. I'd be surprised if the existing object pool implementation is that bad. As they say: don't fix something unless it's known to be broken. I think Andi's comment above is hinting at this too - basically we're saying "beware of premature optimization". As a side note, the OpenDJ SDK's connection pool implementation uses very small synchronized blocks to manage the internal pool state, with care taken to perform anything that may potentially take a long time outside of the critical sections - it performs perfectly well for our needs (although it's been a while since I last stress tested it).
2. If profiling has shown that the object pool implementation is a significant source of application contention then you may want to take a look at the sample ObjectPool class that I've attached (a simplified example of what you're trying to do with monitoring and eviction capabilities removed). When I looked at yours I became a little a confused over its use of a LinkedBlockingQueue (LBQ) and a Semaphore. In particular, decisions as to whether to allocate new objects or not seem to be based on a non-atomic check of the LBQ size and semaphore acquisition. I think that this is unnecessary: if the Semaphore is used to measure the total number of available objects (including those already allocated and in use or idle, and those which have not yet been allocated), then a successful acquisition of the semaphore indicates that you can obtain an object from somewhere. In other words, if there is nothing in the queue then allocate one instead. The side effect of this is that you don't need to use an LBQ, since the blocking and bounded size capabilities are not needed: instead you can use a more efficient non-blocking and unbounded ConcurrentLinkedQueue (CLQ).
3. I think that you are right to require an explicit release of a pooled object. You may want to consider implementing Closeable in ObjectPoolEntry so that people can take advantage of try-with-resource in JDK7. I had thought about using SoftReferences and a ReferenceQueue to automatically capture leaked unreleased pooled objects but this will only be performed during a GC which won't prevent potential deadlocks. I think that it is better to fail fast rather tha try to hide memory leaks.