[ http://issues.apache.org/jira/browse/POOL-86?page=comments#action_12445490 ] Sandy McArthur commented on POOL-86: ------------------------------------
> Firstly, borrowObject() is returning the LRU object rather than the MRU > object. That minimizes rather than maximizes object reuse and tends to > refresh all the idle objects, preventing them from becoming evictable. I think you have minimize and maximize backwards and reusing idle objects is why you'd want to use a pool. There are a couple of reasons for GKOP preferring the LRU idle object: 1. The reason to use a pool is that you have reusable objects that are expensive to create or dispose of. Using a FIFO ordering (LRU) means you use all the pooled objects over time instead of just a handful. Idle object will be kept fresh and ready to reuse or they will fail validateObject and be discarded. LIFO ordering (MRU) will tend to reuse the same objects and others will rarely be used under peak load, unless you have an idle object evictor to check their reusability ... 2. Using an idle object evictor can cause deadlocks. If the PoolableObjectFactory uses synchronization and the pool is accessed in a synchronized context it can lead to locks being acquired in the opposite order which creates a deadlock possible race condition and thread-safty cannot be guaranteed. Basically, avoid idle object eviction if you can. 3. A FIFO pool is implicitly optimized to be prepared to handle the next load spike. And that is the point of the pool. Optimizing the pool for low load levels reduces the important advantage of using a pool, being prepared for lots of work. I can think of situations where the above doesn't apply but "Generic" pools are named "Generic" because they aren't optimized for any specific situation. > Secondly, evict() itself has a couple of problems, both of which only show up > when many keys are in play: > Here's a patch fixing both problems: If you could work the patch to only address the evict issue and attach it as a file instead of a copy/paste in it's own issue I'd appreciate it. > GenericKeyedObjectPool retaining too many idle objects > ------------------------------------------------------ > > Key: POOL-86 > URL: http://issues.apache.org/jira/browse/POOL-86 > Project: Commons Pool > Issue Type: Bug > Affects Versions: 1.3 > Reporter: Mike Martin > > There are two somewhat related problems in GenericKeyedObjectPool that cause > many more idle objects to be retained than should be, for much longer than > they > should be. > Firstly, borrowObject() is returning the LRU object rather than the MRU > object. > That minimizes rather than maximizes object reuse and tends to refresh all the > idle objects, preventing them from becoming evictable. > The idle LinkedList is being maintained with: > borrowObject: list.removeFirst() > returnObject: list.addLast() > These should either both be ...First() or both ...Last() so the list maintains > a newer-to-older, or vice-versa, ordering. The code in evict() works from the > end of the list which indicates newer-to-older might have been originally > intended. > Secondly, evict() itself has a couple of problems, both of which only show up > when many keys are in play: > 1. Once it processes a key it doesn't advance to the next key. > 2. _evictLastIndex is not working as documented ("Position in the _pool where > the _evictor last stopped"). Instead it's the position where the last > scan > started, and becomes the position at which it attempts to start scanning > *in the next pool*. That just causes objects eligible for eviction to > sometimes be skipped entirely. > Here's a patch fixing both problems: > GenericKeyedObjectPool.java > 990c990 > < pool.addLast(new ObjectTimestampPair(obj)); > --- > > pool.addFirst(new ObjectTimestampPair(obj)); > 1094,1102c1094,1095 > < } > < > < // if we don't have a keyed object pool iterator > < if (objIter == null) { > < final LinkedList list = (LinkedList)_poolMap.get(key); > < if (_evictLastIndex < 0 || _evictLastIndex > > list.size()) { > < _evictLastIndex = list.size(); > < } > < objIter = list.listIterator(_evictLastIndex); > --- > > LinkedList list = (LinkedList)_poolMap.get(key); > > objIter = list.listIterator(list.size()); > 1154,1155c1147 > < _evictLastIndex = -1; > < objIter = null; > --- > > key = null; > 1547,1551d1538 > < > < /** > < * Position in the _pool where the _evictor last stopped. > < */ > < private int _evictLastIndex = -1; > I have a local unit test for this but it depends on some other code I can't > donate. It works like this: > 1. Fill the pool with _maxTotal objects using many different keys. > 2. Select X as a small number, e.g. 2. > 3. Compute: > maxEvictionRunsNeeded = (maxTotal - X) / numTestsPerEvictionRun + 2; > maxEvictionTime = minEvictableIdleTimeMillis + maxEvictionRunsNeeded > * timeBetweenEvictionRunsMillis; > 4. Enter a loop: > a. Borrow X objects. > b. Exit if _totalIdle = 0 > c. Return the X objects. > Fail if loop doesn't exit within maxEvictionTime. > Mike -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
