Uploaded image for project: 'Commons Pool'
  1. Commons Pool
  2. POOL-86

GenericKeyedObjectPool retaining too many idle objects

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.4
    • Labels:
      None

      Description

      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

        Attachments

        1. pool-86.withtest.patch
          4 kB
          Mike Martin
        2. pool-86.patch
          2 kB
          Mike Martin

          Issue Links

            Activity

              People

              • Assignee:
                Unassigned
                Reporter:
                jackknifebarber Mike Martin
              • Votes:
                1 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: