[ http://issues.apache.org/jira/browse/POOL-86?page=comments#action_12445665 ] Mike Martin commented on POOL-86: ---------------------------------
Sandy wrote: > 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 ... I think you're omitting a key point: the reason to use a pool is not just to avoid object creation expense; it's also to minimize the number of objects needed to satisfy demand. At the least, that is *very* true of the primary use case I know of for pooling, DB connection pooling. DB connections are a very expensive resource to lie around unused (or underused) indefinitely. They are usually associated with an *entire process* on a DB server. My company began reworking its pooling code using commons-pool specifically to gain control over an exploding # of idle connections as the # of DB users and # of servers grew (I'm talking hundreds of users and thousands of connections). So we can't simply say "avoid idle object eviction"; for many folks it's absolutely necessary. And if the feature is there, it needs to work. Idle eviction can only work properly using a LIFO list. Consider again the unit test I described. It's not an exaggerated case. If I experience a momentary load spike that opens, say, 100 concurrent connections, and it's followed by a persistent "normal" load that needs only 2, I absolutely *cannot* continually consume 100 connections thereafter when 2 would suffice. The FIFO list only allows idleness to be detected if demand drops all the way to zero. Otherwise it becomes impossible for the pool to realize that only 2 connections are now sufficient; the 2 concurrent users keep cycling around all 100 connections, and each of the 100 sees very little work. In addition, consider that a LIFO list still works correctly if the user is *not* using idle eviction. But the reverse is not true; FIFO ruins things when idle eviction is needed. There is no such thing as keeping idle objects "fresh". By definition, every object in a pool is equivalent and interchangeable. You're right that LIFO *will* tend to reuse the same objects, but that's the whole point! That is how a pool tracks the demand being placed on it. I've done quite a bit of analysis of GKOP's behavior in both cases, before and after this fix, in a real-world production scenario. Idle eviction simply *does not work* with a FIFO list unless demand drops to zero. Otherwise the pool rises to its high-water mark and stays there. So I hope you'll reconsider fixing this, and I'll attach the patch as a file for you. Thanks, Mike > 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 > Assigned To: Sandy McArthur > > 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]
