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();
}
}
}
