commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1086194 - in /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl: GenericKeyedObjectPool.java GenericObjectPool.java
Date Mon, 28 Mar 2011 11:35:07 GMT
Author: markt
Date: Mon Mar 28 11:35:06 2011
New Revision: 1086194

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

Modified:
    commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
    commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java

Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java?rev=1086194&r1=1086193&r2=1086194&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
Mon Mar 28 11:35:06 2011
@@ -274,8 +274,10 @@ public class GenericKeyedObjectPool<K,V>
      * Use a negative value for no limit.
      * @see #getMaxTotal
      */
-    public synchronized void setMaxTotalPerKey(int maxTotalPerKey) {
-        this.maxTotalPerKey = maxTotalPerKey;
+    public void setMaxTotalPerKey(int maxTotalPerKey) {
+        synchronized(this) {
+            this.maxTotalPerKey = maxTotalPerKey;
+        }
         allocate();
     }
 
@@ -301,8 +303,10 @@ public class GenericKeyedObjectPool<K,V>
      * Use a negative value for no limit.
      * @see #getMaxTotal
      */
-    public synchronized void setMaxTotal(int maxTotal) {
-        this.maxTotal = maxTotal;
+    public void setMaxTotal(int maxTotal) {
+        synchronized(this) {
+            this.maxTotal = maxTotal;
+        }
         allocate();
     }
 
@@ -327,8 +331,10 @@ public class GenericKeyedObjectPool<K,V>
      * @param whenExhaustedAction the action code
      * @see #getWhenExhaustedAction
      */
-    public synchronized void setWhenExhaustedAction(WhenExhaustedAction whenExhaustedAction)
{
-        this.whenExhaustedAction = whenExhaustedAction;
+    public void setWhenExhaustedAction(WhenExhaustedAction whenExhaustedAction) {
+        synchronized(this) {
+            this.whenExhaustedAction = whenExhaustedAction;
+        }
         allocate();
     }
 
@@ -367,8 +373,10 @@ public class GenericKeyedObjectPool<K,V>
      * @see #setWhenExhaustedAction
      * @see WhenExhaustedAction#BLOCK
      */
-    public synchronized void setMaxWait(long maxWait) {
-        this.maxWait = maxWait;
+    public void setMaxWait(long maxWait) {
+        synchronized(this) {
+            this.maxWait = maxWait;
+        }
         allocate();
     }
 
@@ -400,8 +408,10 @@ public class GenericKeyedObjectPool<K,V>
      *
      * @since 2.0
      */
-    public synchronized void setMaxIdlePerKey(int maxIdlePerKey) {
-        this.maxIdlePerKey = maxIdlePerKey;
+    public void setMaxIdlePerKey(int maxIdlePerKey) {
+        synchronized(this) {
+            this.maxIdlePerKey = maxIdlePerKey;
+        }
         allocate();
     }
 
@@ -685,11 +695,10 @@ public class GenericKeyedObjectPool<K,V>
 
             // 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) {
@@ -747,6 +756,7 @@ public class GenericKeyedObjectPool<K,V>
                                     }
                                 }
                             } catch(InterruptedException e) {
+                                boolean doAllocate = false;
                                 synchronized (this) {
                                     // Need to handle the all three possibilities
                                     if (latch.getPair() == null && !latch.mayCreate())
{
@@ -757,7 +767,7 @@ public class GenericKeyedObjectPool<K,V>
                                         // 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();
@@ -765,6 +775,9 @@ public class GenericKeyedObjectPool<K,V>
                                         returnObject(latch.getkey(), latch.getPair().getValue());
                                     }
                                 }
+                                if (doAllocate) {
+                                    allocate();
+                                }
                                 Thread.currentThread().interrupt();
                                 throw e;
                             }
@@ -801,8 +814,8 @@ public class GenericKeyedObjectPool<K,V>
                         synchronized (this) {
                             latch.getPool().decrementInternalProcessingCount();
                             // No need to reset latch - about to throw exception
-                            allocate();
                         }
+                        allocate();
                     }
                 }
             }
@@ -833,8 +846,8 @@ public class GenericKeyedObjectPool<K,V>
                         latch.reset();
                         _allocationQueue.add(0, latch);
                     }
-                    allocate();
                 }
+                allocate();
                 if (newlyCreated) {
                     throw new NoSuchElementException(
                        "Could not create a validated object, cause: " +
@@ -850,7 +863,8 @@ public class GenericKeyedObjectPool<K,V>
     /**
      * 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;
@@ -1078,8 +1092,8 @@ public class GenericKeyedObjectPool<K,V>
                                 _poolList.remove(key);
                             }
                         }
-                        allocate();
                     }
+                    allocate();
                 }
             }
         }
@@ -1171,8 +1185,8 @@ public class GenericKeyedObjectPool<K,V>
                         _poolMap.remove(key);
                         _poolList.remove(key);
                     }
-                    allocate();
                 }
+                allocate();
             }
         }
     }
@@ -1207,6 +1221,7 @@ public class GenericKeyedObjectPool<K,V>
 
         // 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 = (_poolMap.get(key));
@@ -1235,11 +1250,14 @@ public class GenericKeyedObjectPool<K,V>
                     if (decrementNumActive) {
                         pool.decrementActiveCount();
                     }
-                    allocate();
+                    doAllocate = true;
                 }
             }
         }
-
+        if (doAllocate) {
+            allocate();
+        }
+        
         // Destroy the instance if necessary
         if (shouldDestroy) {
             try {
@@ -1257,8 +1275,8 @@ public class GenericKeyedObjectPool<K,V>
                         _poolMap.remove(key);
                         _poolList.remove(key);
                     }
-                    allocate();
                 }
+                allocate();
             }
         }
     }
@@ -1285,8 +1303,8 @@ public class GenericKeyedObjectPool<K,V>
                     _poolList.add(key);
                 }
                 pool.decrementActiveCount();
-                allocate(); // _totalActive has changed
             }
+            allocate(); // _totalActive has changed
         }
     }
 
@@ -1632,8 +1650,8 @@ public class GenericKeyedObjectPool<K,V>
             } finally {
                 synchronized (this) {
                     pool.decrementInternalProcessingCount();
-                    allocate();
                 }
+                allocate();
             }
         }
     }

Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1086194&r1=1086193&r2=1086194&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
Mon Mar 28 11:35:06 2011
@@ -256,8 +256,10 @@ public class GenericObjectPool<T> extend
      * by the pool.
      * @see #getMaxTotal
      */
-    public synchronized void setMaxTotal(int maxTotal) {
-        this.maxTotal = maxTotal;
+    public void setMaxTotal(int maxTotal) {
+        synchronized(this) {
+            this.maxTotal = maxTotal;
+        }
         allocate();
     }
 
@@ -283,8 +285,10 @@ public class GenericObjectPool<T> extend
      *        or {@link WhenExhaustedAction#GROW}
      * @see #getWhenExhaustedAction
      */
-    public synchronized void setWhenExhaustedAction(WhenExhaustedAction whenExhaustedAction)
{
-        this.whenExhaustedAction = whenExhaustedAction;
+    public void setWhenExhaustedAction(WhenExhaustedAction whenExhaustedAction) {
+        synchronized(this) {
+            this.whenExhaustedAction = whenExhaustedAction;
+        }
         allocate();
     }
 
@@ -323,8 +327,10 @@ public class GenericObjectPool<T> extend
      * @see #setWhenExhaustedAction
      * @see WhenExhaustedAction#BLOCK
      */
-    public synchronized void setMaxWait(long maxWait) {
-        this.maxWait = maxWait;
+    public void setMaxWait(long maxWait) {
+        synchronized(this) {
+            this.maxWait = maxWait;
+        }
         allocate();
     }
 
@@ -350,8 +356,10 @@ public class GenericObjectPool<T> extend
      * Use a negative value to indicate an unlimited number of idle instances.
      * @see #getMaxIdle
      */
-    public synchronized void setMaxIdle(int maxIdle) {
-        this.maxIdle = maxIdle;
+    public void setMaxIdle(int maxIdle) {
+        synchronized(this) {
+            this.maxIdle = maxIdle;
+        }
         allocate();
     }
 
@@ -367,8 +375,10 @@ public class GenericObjectPool<T> extend
      * @see #getMinIdle
      * @see #getTimeBetweenEvictionRunsMillis()
      */
-    public synchronized void setMinIdle(int minIdle) {
-        this.minIdle = minIdle;
+    public void setMinIdle(int minIdle) {
+        synchronized(this) {
+            this.minIdle = minIdle;
+        }
         allocate();
     }
 
@@ -656,11 +666,10 @@ public class GenericObjectPool<T> extend
 
             // 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) {
@@ -719,6 +728,7 @@ public class GenericObjectPool<T> extend
                                     }
                                 }
                             } catch(InterruptedException e) {
+                                boolean doAllocate = false;
                                 synchronized(this) {
                                     // Need to handle the all three possibilities
                                     if (latch.getPair() == null && !latch.mayCreate())
{
@@ -729,7 +739,7 @@ public class GenericObjectPool<T> extend
                                         // 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--;
@@ -737,6 +747,9 @@ public class GenericObjectPool<T> extend
                                         returnObject(latch.getPair().getValue());
                                     }
                                 }
+                                if (doAllocate) {
+                                    allocate();
+                                }
                                 Thread.currentThread().interrupt();
                                 throw e;
                             }
@@ -774,8 +787,8 @@ public class GenericObjectPool<T> extend
                         synchronized (this) {
                             _numInternalProcessing--;
                             // No need to reset latch - about to throw exception
-                            allocate();
                         }
+                        allocate();
                     }
                 }
             }
@@ -807,8 +820,8 @@ public class GenericObjectPool<T> extend
                         latch.reset();
                         _allocationQueue.add(0, latch);
                     }
-                    allocate();
                 }
+                allocate();
                 if(newlyCreated) {
                     throw new NoSuchElementException("Could not create a validated object,
cause: " + e.getMessage());
                 }
@@ -822,7 +835,8 @@ public class GenericObjectPool<T> extend
     /**
      * 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;
@@ -869,8 +883,8 @@ public class GenericObjectPool<T> extend
         } finally {
             synchronized (this) {
                 _numActive--;
-                allocate();
             }
+            allocate();
         }
     }
 
@@ -919,8 +933,8 @@ public class GenericObjectPool<T> extend
             } finally {
                 synchronized (this) {
                     _numInternalProcessing--;
-                    allocate();
                 }
+                allocate();
             }
         }
     }
@@ -979,8 +993,8 @@ public class GenericObjectPool<T> extend
             // "behavior flag", decrementNumActive, from addObjectToPool.
             synchronized(this) {
                 _numActive--;
-                allocate();
             }
+            allocate();
         }
     }
 
@@ -1009,6 +1023,7 @@ public class GenericObjectPool<T> extend
 
         // 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;
@@ -1026,10 +1041,13 @@ public class GenericObjectPool<T> extend
                     if (decrementNumActive) {
                         _numActive--;
                     }
-                    allocate();
+                    doAllocate = true;
                 }
             }
         }
+        if (doAllocate) {
+            allocate();
+        }
 
         // Destroy the instance if necessary
         if(shouldDestroy) {
@@ -1042,8 +1060,8 @@ public class GenericObjectPool<T> extend
             if (decrementNumActive) {
                 synchronized(this) {
                     _numActive--;
-                    allocate();
                 }
+                allocate();
             }
         }
 
@@ -1178,8 +1196,8 @@ public class GenericObjectPool<T> extend
             } finally {
                 synchronized (this) {
                     _numInternalProcessing--;
-                    allocate();
                 }
+                allocate();
             }
         }
     }



Mime
View raw message