commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1086190 - in /commons/proper/pool/branches/POOL_1_X/src: changes/changes.xml java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java java/org/apache/commons/pool/impl/GenericObjectPool.java
Date Mon, 28 Mar 2011 11:22:09 GMT
Author: markt
Date: Mon Mar 28 11:22:08 2011
New Revision: 1086190

URL: http://svn.apache.org/viewvc?rev=1086190&view=rev
Log:
Move allocate() calls outside sync blocks to address performance issues identified in Phil's
testing

Modified:
    commons/proper/pool/branches/POOL_1_X/src/changes/changes.xml
    commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
    commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericObjectPool.java

Modified: commons/proper/pool/branches/POOL_1_X/src/changes/changes.xml
URL: http://svn.apache.org/viewvc/commons/proper/pool/branches/POOL_1_X/src/changes/changes.xml?rev=1086190&r1=1086189&r2=1086190&view=diff
==============================================================================
--- commons/proper/pool/branches/POOL_1_X/src/changes/changes.xml (original)
+++ commons/proper/pool/branches/POOL_1_X/src/changes/changes.xml Mon Mar 28 11:22:08 2011
@@ -36,6 +36,9 @@
       Correct bug that could lead to inappropriate pool starvation when evict()
       and borrowObject() are called concurrently.
     </action>
+    <action dev="markt" type="fix" due-to="psteitz">
+      Fix performance issues when object destruction has latency.
+    </action>
   </release>
   <release version="1.5.5" date="2010-09-10" description=
      "This is a patch release, including bugfixes, documentation improvements and some deprecations

Modified: commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java?rev=1086190&r1=1086189&r2=1086190&view=diff
==============================================================================
--- commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
(original)
+++ commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
Mon Mar 28 11:22:08 2011
@@ -657,8 +657,10 @@ public class GenericKeyedObjectPool exte
      *
      * @see #getMaxActive
      */
-    public synchronized void setMaxActive(int maxActive) {
-        _maxActive = maxActive;
+    public void setMaxActive(int maxActive) {
+        synchronized(this) {
+            _maxActive = maxActive;
+        }
         allocate();
     }
 
@@ -684,8 +686,10 @@ public class GenericKeyedObjectPool exte
      * Use a negative value for no limit.
      * @see #getMaxTotal
      */
-    public synchronized void setMaxTotal(int maxTotal) {
-        _maxTotal = maxTotal;
+    public void setMaxTotal(int maxTotal) {
+        synchronized(this) {
+            _maxTotal = maxTotal;
+        }
         allocate();
     }
 
@@ -712,17 +716,19 @@ public class GenericKeyedObjectPool exte
      *        or {@link #WHEN_EXHAUSTED_GROW}
      * @see #getWhenExhaustedAction
      */
-    public synchronized void setWhenExhaustedAction(byte whenExhaustedAction) {
-        switch(whenExhaustedAction) {
-            case WHEN_EXHAUSTED_BLOCK:
-            case WHEN_EXHAUSTED_FAIL:
-            case WHEN_EXHAUSTED_GROW:
-                _whenExhaustedAction = whenExhaustedAction;
-                allocate();
-                break;
-            default:
-                throw new IllegalArgumentException("whenExhaustedAction " + whenExhaustedAction
+ " not recognized.");
+    public void setWhenExhaustedAction(byte whenExhaustedAction) {
+        synchronized(this) {
+            switch(whenExhaustedAction) {
+                case WHEN_EXHAUSTED_BLOCK:
+                case WHEN_EXHAUSTED_FAIL:
+                case WHEN_EXHAUSTED_GROW:
+                    _whenExhaustedAction = whenExhaustedAction;
+                    break;
+                default:
+                    throw new IllegalArgumentException("whenExhaustedAction " + whenExhaustedAction
+ " not recognized.");
+            }
         }
+        allocate();
     }
 
 
@@ -760,8 +766,10 @@ public class GenericKeyedObjectPool exte
      * @see #setWhenExhaustedAction
      * @see #WHEN_EXHAUSTED_BLOCK
      */
-    public synchronized void setMaxWait(long maxWait) {
-        _maxWait = maxWait;
+    public void setMaxWait(long maxWait) {
+        synchronized(this) {
+            _maxWait = maxWait;
+        }
         allocate();
     }
 
@@ -789,8 +797,10 @@ public class GenericKeyedObjectPool exte
      * @see #getMaxIdle
      * @see #DEFAULT_MAX_IDLE
      */
-    public synchronized void setMaxIdle(int maxIdle) {
-        _maxIdle = maxIdle;
+    public void setMaxIdle(int maxIdle) {
+        synchronized(this) {
+            _maxIdle = maxIdle;
+        }
         allocate();
     }
 
@@ -1089,11 +1099,10 @@ public class GenericKeyedObjectPool exte
 
             // Add this request to the queue
             _allocationQueue.add(latch);
-
-            // Work the allocation queue, allocating idle instances and
-            // instance creation permits in request arrival order
-            allocate();
         }
+        // Work the allocation queue, allocating idle instances and
+        // instance creation permits in request arrival order
+        allocate();
 
         for(;;) {
             synchronized (this) {
@@ -1151,6 +1160,7 @@ public class GenericKeyedObjectPool exte
                                     }
                                 }
                             } catch(InterruptedException e) {
+                                boolean doAllocate = false;
                                 synchronized (this) {
                                     // Need to handle the all three possibilities
                                     if (latch.getPair() == null && !latch.mayCreate())
{
@@ -1161,7 +1171,7 @@ public class GenericKeyedObjectPool exte
                                         // Case 2: latch has been given permission to create
                                         //         a new object
                                         latch.getPool().decrementInternalProcessingCount();
-                                        allocate();
+                                        doAllocate = true;
                                     } else {
                                         // Case 3: An object has been allocated
                                         latch.getPool().decrementInternalProcessingCount();
@@ -1169,6 +1179,9 @@ public class GenericKeyedObjectPool exte
                                         returnObject(latch.getkey(), latch.getPair().getValue());
                                     }
                                 }
+                                if (doAllocate) {
+                                    allocate();
+                                }
                                 Thread.currentThread().interrupt();
                                 throw e;
                             }
@@ -1205,8 +1218,8 @@ public class GenericKeyedObjectPool exte
                         synchronized (this) {
                             latch.getPool().decrementInternalProcessingCount();
                             // No need to reset latch - about to throw exception
-                            allocate();
                         }
+                        allocate();
                     }
                 }
             }
@@ -1237,8 +1250,8 @@ public class GenericKeyedObjectPool exte
                         latch.reset();
                         _allocationQueue.add(0, latch);
                     }
-                    allocate();
                 }
+                allocate();
                 if (newlyCreated) {
                     throw new NoSuchElementException(
                        "Could not create a validated object, cause: " +
@@ -1254,7 +1267,8 @@ public class GenericKeyedObjectPool exte
     /**
      * Allocate available instances to latches in the allocation queue.  Then
      * set _mayCreate to true for as many additional latches remaining in queue
-     * as _maxActive allows for each key.
+     * as _maxActive allows for each key. This method <b>MUST NOT</b> be called
+     * from inside a sync block.
      */
     private void allocate() {
         boolean clearOldest = false;
@@ -1484,8 +1498,8 @@ public class GenericKeyedObjectPool exte
                                 _poolList.remove(key);
                             }
                         }
-                        allocate();
                     }
+                    allocate();
                 }
             }
 
@@ -1574,8 +1588,8 @@ public class GenericKeyedObjectPool exte
                             _poolMap.remove(key);
                             _poolList.remove(key);
                         }
-                        allocate();
                     }
+                    allocate();
                 }
             }
         }
@@ -1611,6 +1625,7 @@ public class GenericKeyedObjectPool exte
 
         // Add instance to pool if there is room and it has passed validation
         // (if testOnreturn is set)
+        boolean doAllocate = false;
         synchronized (this) {
             // grab the pool (list) of objects associated with the given key
             pool = (ObjectQueue) (_poolMap.get(key));
@@ -1639,10 +1654,13 @@ public class GenericKeyedObjectPool exte
                     if (decrementNumActive) {
                         pool.decrementActiveCount();
                     }
-                    allocate();
+                    doAllocate = true;
                 }
             }
         }
+        if (doAllocate) {
+            allocate();
+        }
 
         // Destroy the instance if necessary
         if (shouldDestroy) {
@@ -1661,8 +1679,8 @@ public class GenericKeyedObjectPool exte
                         _poolMap.remove(key);
                         _poolList.remove(key);
                     }
-                    allocate();
                 }
+                allocate();
             }
         }
     }
@@ -1688,8 +1706,8 @@ public class GenericKeyedObjectPool exte
                     _poolList.add(key);
                 }
                 pool.decrementActiveCount();
-                allocate(); // _totalActive has changed
             }
+            allocate(); // _totalActive has changed
         }
     }
 
@@ -2078,8 +2096,8 @@ public class GenericKeyedObjectPool exte
             } finally {
                 synchronized (this) {
                     pool.decrementInternalProcessingCount();
-                    allocate();
                 }
+                allocate();
             }
         }
     }

Modified: commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericObjectPool.java?rev=1086190&r1=1086189&r2=1086190&view=diff
==============================================================================
--- commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericObjectPool.java
(original)
+++ commons/proper/pool/branches/POOL_1_X/src/java/org/apache/commons/pool/impl/GenericObjectPool.java
Mon Mar 28 11:22:08 2011
@@ -624,8 +624,10 @@ public class GenericObjectPool extends B
      * by the pool.
      * @see #getMaxActive
      */
-    public synchronized void setMaxActive(int maxActive) {
-        _maxActive = maxActive;
+    public void setMaxActive(int maxActive) {
+        synchronized(this) {
+            _maxActive = maxActive;
+        }
         allocate();
     }
 
@@ -651,17 +653,19 @@ public class GenericObjectPool extends B
      *        or {@link #WHEN_EXHAUSTED_GROW}
      * @see #getWhenExhaustedAction
      */
-    public synchronized void setWhenExhaustedAction(byte whenExhaustedAction) {
-        switch(whenExhaustedAction) {
-            case WHEN_EXHAUSTED_BLOCK:
-            case WHEN_EXHAUSTED_FAIL:
-            case WHEN_EXHAUSTED_GROW:
-                _whenExhaustedAction = whenExhaustedAction;
-                allocate();
-                break;
-            default:
-                throw new IllegalArgumentException("whenExhaustedAction " + whenExhaustedAction
+ " not recognized.");
+    public void setWhenExhaustedAction(byte whenExhaustedAction) {
+        synchronized(this) {
+            switch(whenExhaustedAction) {
+                case WHEN_EXHAUSTED_BLOCK:
+                case WHEN_EXHAUSTED_FAIL:
+                case WHEN_EXHAUSTED_GROW:
+                    _whenExhaustedAction = whenExhaustedAction;
+                    break;
+                default:
+                    throw new IllegalArgumentException("whenExhaustedAction " + whenExhaustedAction
+ " not recognized.");
+            }
         }
+        allocate();
     }
 
 
@@ -699,8 +703,10 @@ public class GenericObjectPool extends B
      * @see #setWhenExhaustedAction
      * @see #WHEN_EXHAUSTED_BLOCK
      */
-    public synchronized void setMaxWait(long maxWait) {
-        _maxWait = maxWait;
+    public void setMaxWait(long maxWait) {
+        synchronized(this) {
+            _maxWait = maxWait;
+        }
         allocate();
     }
 
@@ -726,8 +732,10 @@ public class GenericObjectPool extends B
      * Use a negative value to indicate an unlimited number of idle instances.
      * @see #getMaxIdle
      */
-    public synchronized void setMaxIdle(int maxIdle) {
-        _maxIdle = maxIdle;
+    public void setMaxIdle(int maxIdle) {
+        synchronized(this) {
+            _maxIdle = maxIdle;
+        }
         allocate();
     }
 
@@ -743,8 +751,10 @@ public class GenericObjectPool extends B
      * @see #getMinIdle
      * @see #getTimeBetweenEvictionRunsMillis()
      */
-    public synchronized void setMinIdle(int minIdle) {
-        _minIdle = minIdle;
+    public void setMinIdle(int minIdle) {
+        synchronized(this) {
+            _minIdle = minIdle;
+        }
         allocate();
     }
 
@@ -994,20 +1004,22 @@ public class GenericObjectPool extends B
      * @param conf configuration to use.
      * @see GenericObjectPool.Config
      */
-    public synchronized void setConfig(GenericObjectPool.Config conf) {
-        setMaxIdle(conf.maxIdle);
-        setMinIdle(conf.minIdle);
-        setMaxActive(conf.maxActive);
-        setMaxWait(conf.maxWait);
-        setWhenExhaustedAction(conf.whenExhaustedAction);
-        setTestOnBorrow(conf.testOnBorrow);
-        setTestOnReturn(conf.testOnReturn);
-        setTestWhileIdle(conf.testWhileIdle);
-        setNumTestsPerEvictionRun(conf.numTestsPerEvictionRun);
-        setMinEvictableIdleTimeMillis(conf.minEvictableIdleTimeMillis);
-        setTimeBetweenEvictionRunsMillis(conf.timeBetweenEvictionRunsMillis);
-        setSoftMinEvictableIdleTimeMillis(conf.softMinEvictableIdleTimeMillis);
-        setLifo(conf.lifo);
+    public void setConfig(GenericObjectPool.Config conf) {
+        synchronized (this) {
+            setMaxIdle(conf.maxIdle);
+            setMinIdle(conf.minIdle);
+            setMaxActive(conf.maxActive);
+            setMaxWait(conf.maxWait);
+            setWhenExhaustedAction(conf.whenExhaustedAction);
+            setTestOnBorrow(conf.testOnBorrow);
+            setTestOnReturn(conf.testOnReturn);
+            setTestWhileIdle(conf.testWhileIdle);
+            setNumTestsPerEvictionRun(conf.numTestsPerEvictionRun);
+            setMinEvictableIdleTimeMillis(conf.minEvictableIdleTimeMillis);
+            setTimeBetweenEvictionRunsMillis(conf.timeBetweenEvictionRunsMillis);
+            setSoftMinEvictableIdleTimeMillis(conf.softMinEvictableIdleTimeMillis);
+            setLifo(conf.lifo);
+        }
         allocate();
     }
 
@@ -1054,11 +1066,10 @@ public class GenericObjectPool extends B
 
             // Add this request to the queue
             _allocationQueue.add(latch);
-
-            // Work the allocation queue, allocating idle instances and
-            // instance creation permits in request arrival order
-            allocate();
         }
+        // Work the allocation queue, allocating idle instances and
+        // instance creation permits in request arrival order
+        allocate();
 
         for(;;) {
             synchronized (this) {
@@ -1117,6 +1128,7 @@ public class GenericObjectPool extends B
                                     }
                                 }
                             } catch(InterruptedException e) {
+                                boolean doAllocate = false;
                                 synchronized(this) {
                                     // Need to handle the all three possibilities
                                     if (latch.getPair() == null && !latch.mayCreate())
{
@@ -1127,7 +1139,7 @@ public class GenericObjectPool extends B
                                         // Case 2: latch has been given permission to create
                                         //         a new object
                                         _numInternalProcessing--;
-                                        allocate();
+                                        doAllocate = true;
                                     } else {
                                         // Case 3: An object has been allocated
                                         _numInternalProcessing--;
@@ -1135,6 +1147,9 @@ public class GenericObjectPool extends B
                                         returnObject(latch.getPair().getValue());
                                     }
                                 }
+                                if (doAllocate) {
+                                    allocate();
+                                }
                                 Thread.currentThread().interrupt();
                                 throw e;
                             }
@@ -1172,8 +1187,8 @@ public class GenericObjectPool extends B
                         synchronized (this) {
                             _numInternalProcessing--;
                             // No need to reset latch - about to throw exception
-                            allocate();
                         }
+                        allocate();
                     }
                 }
             }
@@ -1205,8 +1220,8 @@ public class GenericObjectPool extends B
                         latch.reset();
                         _allocationQueue.add(0, latch);
                     }
-                    allocate();
                 }
+                allocate();
                 if(newlyCreated) {
                     throw new NoSuchElementException("Could not create a validated object,
cause: " + e.getMessage());
                 }
@@ -1220,7 +1235,8 @@ public class GenericObjectPool extends B
     /**
      * Allocate available instances to latches in the allocation queue.  Then
      * set _mayCreate to true for as many additional latches remaining in queue
-     * as _maxActive allows.
+     * as _maxActive allows. While it is safe for GOP, for consistency with GKOP
+     * this method should not be called from inside a sync block. 
      */
     private synchronized void allocate() {
         if (isClosed()) return;
@@ -1268,8 +1284,8 @@ public class GenericObjectPool extends B
         } finally {
             synchronized (this) {
                 _numActive--;
-                allocate();
             }
+            allocate();
         }
     }
 
@@ -1317,8 +1333,8 @@ public class GenericObjectPool extends B
             } finally {
                 synchronized(this) {
                     _numInternalProcessing--;
-                    allocate();
                 }
+                allocate();
             }
         }
     }
@@ -1375,8 +1391,8 @@ public class GenericObjectPool extends B
                 // "behavior flag", decrementNumActive, from addObjectToPool.
                 synchronized(this) {
                     _numActive--;
-                    allocate();
                 }
+                allocate();
             }
         }
     }
@@ -1406,6 +1422,7 @@ public class GenericObjectPool extends B
 
         // Add instance to pool if there is room and it has passed validation
         // (if testOnreturn is set)
+        boolean doAllocate = false;
         synchronized (this) {
             if (isClosed()) {
                 shouldDestroy = true;
@@ -1423,10 +1440,13 @@ public class GenericObjectPool extends B
                     if (decrementNumActive) {
                         _numActive--;
                     }
-                    allocate();
+                    doAllocate = true;
                 }
             }
         }
+        if (doAllocate) {
+            allocate();
+        }
 
         // Destroy the instance if necessary
         if(shouldDestroy) {
@@ -1439,8 +1459,8 @@ public class GenericObjectPool extends B
             if (decrementNumActive) {
                 synchronized(this) {
                     _numActive--;
-                    allocate();
                 }
+                allocate();
             }
         }
 
@@ -1605,8 +1625,8 @@ public class GenericObjectPool extends B
             } finally {
                 synchronized (this) {
                     _numInternalProcessing--;
-                    allocate();
                 }
+                allocate();
             }
         }
     }



Mime
View raw message