You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ps...@apache.org on 2007/12/10 03:53:14 UTC
svn commit: r602773 - in /commons/proper/pool/trunk:
src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
src/java/org/apache/commons/pool/impl/GenericObjectPool.java
xdocs/changes.xml
Author: psteitz
Date: Sun Dec 9 18:53:10 2007
New Revision: 602773
URL: http://svn.apache.org/viewvc?rev=602773&view=rev
Log:
Reduced synchronization in GenericObjectPool, GenericKeyedObjectPool.
Factory method activations within synchronized blocks were causing
performance problems in DBCP and other applications where factory methods
could block.
JIRA: POOL-93
JIRA: POOL-108
Reported and patched by Holger Hoffstätte, Matthew Moore
Patched by Marcos Sanz, Mark Thomas
Modified:
commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericObjectPool.java
commons/proper/pool/trunk/xdocs/changes.xml
Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java?rev=602773&r1=602772&r2=602773&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java (original)
+++ commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericKeyedObjectPool.java Sun Dec 9 18:53:10 2007
@@ -816,85 +816,91 @@
//-- ObjectPool methods ------------------------------------------
- public synchronized Object borrowObject(Object key) throws Exception {
- assertOpen();
+ public Object borrowObject(Object key) throws Exception {
long starttime = System.currentTimeMillis();
boolean newlyCreated = false;
for(;;) {
- ObjectQueue pool = (ObjectQueue)(_poolMap.get(key));
- if(null == pool) {
- pool = new ObjectQueue();
- _poolMap.put(key,pool);
- _poolList.add(key);
- }
ObjectTimestampPair pair = null;
- // if there are any sleeping, just grab one of those
- try {
- pair = (ObjectTimestampPair)(pool.queue.removeFirst());
- if(null != pair) {
- _totalIdle--;
- }
- } catch(NoSuchElementException e) { /* ignored */
- }
- // otherwise
- if(null == pair) {
- // if there is a totalMaxActive and we are at the limit then
- // we have to make room
- if ((_maxTotal > 0) && (_totalActive + _totalIdle >= _maxTotal)) {
- clearOldest();
- }
-
- // check if we can create one
- // (note we know that the num sleeping is 0, else we wouldn't be here)
- if ((_maxActive < 0 || pool.activeCount < _maxActive) &&
- (_maxTotal < 0 || _totalActive + _totalIdle < _maxTotal)) {
- Object obj = _factory.makeObject(key);
- pair = new ObjectTimestampPair(obj);
- newlyCreated = true;
- } else {
- // the pool is exhausted
- switch(_whenExhaustedAction) {
- case WHEN_EXHAUSTED_GROW:
- Object obj = _factory.makeObject(key);
- pair = new ObjectTimestampPair(obj);
- break;
- case WHEN_EXHAUSTED_FAIL:
- throw new NoSuchElementException();
- case WHEN_EXHAUSTED_BLOCK:
- try {
- if(_maxWait <= 0) {
- wait();
- } else {
- // this code may be executed again after a notify then continue cycle
- // so, need to calculate the amount of time to wait
- final long elapsed = (System.currentTimeMillis() - starttime);
- final long waitTime = _maxWait - elapsed;
- if (waitTime > 0)
- {
- wait(waitTime);
+ ObjectQueue pool = null;
+ synchronized (this) {
+ assertOpen();
+ pool = (ObjectQueue)(_poolMap.get(key));
+ if(null == pool) {
+ pool = new ObjectQueue();
+ _poolMap.put(key,pool);
+ _poolList.add(key);
+ }
+ // if there are any sleeping, just grab one of those
+ try {
+ pair = (ObjectTimestampPair)(pool.queue.removeFirst());
+ if(null != pair) {
+ _totalIdle--;
+ }
+ } catch(NoSuchElementException e) { /* ignored */
+ }
+ // otherwise
+ if(null == pair) {
+ // if there is a totalMaxActive and we are at the limit then
+ // we have to make room
+ if ((_maxTotal > 0) && (_totalActive + _totalIdle >= _maxTotal)) {
+ clearOldest();
+ }
+
+ // check if we can create one
+ // (note we know that the num sleeping is 0, else we wouldn't be here)
+ if ((_maxActive < 0 || pool.activeCount < _maxActive) &&
+ (_maxTotal < 0 || _totalActive + _totalIdle < _maxTotal)) {
+ Object obj = _factory.makeObject(key);
+ pair = new ObjectTimestampPair(obj);
+ newlyCreated = true;
+ } else {
+ // the pool is exhausted
+ switch(_whenExhaustedAction) {
+ case WHEN_EXHAUSTED_GROW:
+ Object obj = _factory.makeObject(key);
+ pair = new ObjectTimestampPair(obj);
+ break;
+ case WHEN_EXHAUSTED_FAIL:
+ throw new NoSuchElementException();
+ case WHEN_EXHAUSTED_BLOCK:
+ try {
+ if(_maxWait <= 0) {
+ wait();
+ } else {
+ // this code may be executed again after a notify then continue cycle
+ // so, need to calculate the amount of time to wait
+ final long elapsed = (System.currentTimeMillis() - starttime);
+ final long waitTime = _maxWait - elapsed;
+ if (waitTime > 0)
+ {
+ wait(waitTime);
+ }
}
+ } catch(InterruptedException e) {
+ // ignored
+ }
+ if(_maxWait > 0 && ((System.currentTimeMillis() - starttime) >= _maxWait)) {
+ throw new NoSuchElementException("Timeout waiting for idle object");
+ } else {
+ continue; // keep looping
}
- } catch(InterruptedException e) {
- // ignored
- }
- if(_maxWait > 0 && ((System.currentTimeMillis() - starttime) >= _maxWait)) {
- throw new NoSuchElementException("Timeout waiting for idle object");
- } else {
- continue; // keep looping
- }
- default:
- throw new IllegalArgumentException("whenExhaustedAction " + _whenExhaustedAction + " not recognized.");
+ default:
+ throw new IllegalArgumentException("whenExhaustedAction " + _whenExhaustedAction + " not recognized.");
+ }
}
}
+ pool.incrementActiveCount();
}
if (newlyCreated) {
- pool.incrementActiveCount();
return pair.value;
} else {
try {
_factory.activateObject(key, pair.value);
} catch (Exception e) {
try {
+ synchronized (this) {
+ pool.decrementActiveCount();
+ }
_factory.destroyObject(key,pair.value);
} catch (Exception e2) {
// swallowed
@@ -910,6 +916,9 @@
}
if (invalid) {
try {
+ synchronized (this) {
+ pool.decrementActiveCount();
+ }
_factory.destroyObject(key,pair.value);
} catch (Exception e) {
// swallowed
@@ -918,7 +927,6 @@
throw new NoSuchElementException("Could not create a validated object");
} // else keep looping
} else {
- pool.incrementActiveCount();
return pair.value;
}
@@ -1066,59 +1074,65 @@
return pool != null ? pool.queue.size() : 0;
}
- public synchronized void returnObject(Object key, Object obj) throws Exception {
+ public void returnObject(Object key, Object obj) throws Exception {
+ try {
+ addObjectToPool(key, obj, true);
+ } catch (Exception e) {
+ if (_factory != null) {
+ try {
+ _factory.destroyObject(key, obj);
+ } catch (Exception e2) {
+ // swallowed
+ }
+ }
+ }
+ }
+
+ private void addObjectToPool(Object key, Object obj,
+ boolean decrementNumActive) throws Exception {
// if we need to validate this object, do so
boolean success = true; // whether or not this object passed validation
if(_testOnReturn && !_factory.validateObject(key, obj)) {
success = false;
- try {
- _factory.destroyObject(key, obj);
- } catch(Exception e) {
- // ignored
- }
} else {
- try {
- _factory.passivateObject(key, obj);
- } catch(Exception e) {
- success = false;
- }
- }
-
- if (isClosed()) {
- try {
- _factory.destroyObject(key, obj);
- } catch(Exception e) {
- // ignored
- }
- return;
+ _factory.passivateObject(key, obj);
}
boolean shouldDestroy = false;
- // grab the pool (list) of objects associated with the given key
- ObjectQueue pool = (ObjectQueue) (_poolMap.get(key));
- // if it doesn't exist, create it
- if(null == pool) {
- pool = new ObjectQueue();
- _poolMap.put(key, pool);
- _poolList.add(key);
- }
- pool.decrementActiveCount();
- // if there's no space in the pool, flag the object for destruction
- // else if we passivated successfully, return it to the pool
- if(_maxIdle >= 0 && (pool.queue.size() >= _maxIdle)) {
- shouldDestroy = true;
- } else if(success) {
- // borrowObject always takes the first element from the queue,
- // so for LIFO, push on top, FIFO add to end
- if (_lifo) {
- pool.queue.addFirst(new ObjectTimestampPair(obj));
+
+ synchronized (this) {
+ // grab the pool (list) of objects associated with the given key
+ ObjectQueue pool = (ObjectQueue) (_poolMap.get(key));
+ // if it doesn't exist, create it
+ if(null == pool) {
+ pool = new ObjectQueue();
+ _poolMap.put(key, pool);
+ _poolList.add(key);
+ }
+ if (decrementNumActive) {
+ pool.decrementActiveCount();
+ }
+ if (isClosed()) {
+ shouldDestroy = true;
} else {
- pool.queue.addLast(new ObjectTimestampPair(obj));
+ // if there's no space in the pool, flag the object for destruction
+ // else if we passivated successfully, return it to the pool
+ if(_maxIdle >= 0 && (pool.queue.size() >= _maxIdle)) {
+ shouldDestroy = true;
+ } else if(success) {
+ // borrowObject always takes the first element from the queue,
+ // so for LIFO, push on top, FIFO add to end
+ if (_lifo) {
+ pool.queue.addFirst(new ObjectTimestampPair(obj));
+ } else {
+ pool.queue.addLast(new ObjectTimestampPair(obj));
+ }
+ _totalIdle++;
+ }
}
- _totalIdle++;
+ notifyAll();
}
- notifyAll();
if(shouldDestroy) {
try {
@@ -1129,20 +1143,22 @@
}
}
- public synchronized void invalidateObject(Object key, Object obj) throws Exception {
+ public void invalidateObject(Object key, Object obj) throws Exception {
try {
_factory.destroyObject(key, obj);
} catch (Exception e) {
// swallowed
} finally {
- ObjectQueue pool = (ObjectQueue) (_poolMap.get(key));
- if(null == pool) {
- pool = new ObjectQueue();
- _poolMap.put(key, pool);
- _poolList.add(key);
+ synchronized (this) {
+ ObjectQueue pool = (ObjectQueue) (_poolMap.get(key));
+ if(null == pool) {
+ pool = new ObjectQueue();
+ _poolMap.put(key, pool);
+ _poolList.add(key);
+ }
+ pool.decrementActiveCount();
+ notifyAll(); // _totalActive has changed
}
- pool.decrementActiveCount();
- notifyAll(); // _totalActive has changed
}
}
@@ -1155,49 +1171,24 @@
* @throws Exception when {@link KeyedPoolableObjectFactory#makeObject} fails.
* @throws IllegalStateException when no {@link #setFactory factory} has been set or after {@link #close} has been called on this pool.
*/
- public synchronized void addObject(Object key) throws Exception {
+ public void addObject(Object key) throws Exception {
assertOpen();
if (_factory == null) {
throw new IllegalStateException("Cannot add objects without a factory.");
}
Object obj = _factory.makeObject(key);
-
- // if we need to validate this object, do so
- boolean success = true; // whether or not this object passed validation
- if(_testOnReturn && !_factory.validateObject(key, obj)) {
- success = false;
- _factory.destroyObject(key, obj);
- } else {
- _factory.passivateObject(key, obj);
- }
-
- boolean shouldDestroy = false;
- // grab the pool (list) of objects associated with the given key
- ObjectQueue pool = (ObjectQueue) (_poolMap.get(key));
- // if it doesn't exist, create it
- if(null == pool) {
- pool = new ObjectQueue();
- _poolMap.put(key, pool);
- _poolList.add(key);
- }
- // if there's no space in the pool, flag the object for destruction
- // else if we passivated succesfully, return it to the pool
- if(_maxIdle >= 0 && (pool.queue.size() >= _maxIdle)) {
- shouldDestroy = true;
- } else if(success) {
- // borrowObject always takes the first element from the queue,
- // so for LIFO, push on top, FIFO add to end
- if (_lifo) {
- pool.queue.addFirst(new ObjectTimestampPair(obj));
- } else {
- pool.queue.addLast(new ObjectTimestampPair(obj));
+ synchronized (this) {
+ try {
+ assertOpen();
+ addObjectToPool(key, obj, false);
+ } catch (IllegalStateException ex) { // Pool closed
+ try {
+ _factory.destroyObject(key, obj);
+ } catch (Exception ex2) {
+ // swallow
+ }
+ throw ex;
}
- _totalIdle++;
- }
- notifyAll();
-
- if(shouldDestroy) {
- _factory.destroyObject(key, obj);
}
}
Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericObjectPool.java?rev=602773&r1=602772&r2=602773&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericObjectPool.java (original)
+++ commons/proper/pool/trunk/src/java/org/apache/commons/pool/impl/GenericObjectPool.java Sun Dec 9 18:53:10 2007
@@ -831,62 +831,64 @@
//-- ObjectPool methods ------------------------------------------
- public synchronized Object borrowObject() throws Exception {
- assertOpen();
+ public Object borrowObject() throws Exception {
long starttime = System.currentTimeMillis();
for(;;) {
ObjectTimestampPair pair = null;
-
- // if there are any sleeping, just grab one of those
- try {
- pair = (ObjectTimestampPair)(_pool.removeFirst());
- } catch(NoSuchElementException e) {
- ; /* ignored */
- }
-
- // otherwise
- if(null == pair) {
- // check if we can create one
- // (note we know that the num sleeping is 0, else we wouldn't be here)
- if(_maxActive < 0 || _numActive < _maxActive) {
- // allow new object to be created
- } else {
- // the pool is exhausted
- switch(_whenExhaustedAction) {
- case WHEN_EXHAUSTED_GROW:
- // allow new object to be created
- break;
- case WHEN_EXHAUSTED_FAIL:
- throw new NoSuchElementException("Pool exhausted");
- case WHEN_EXHAUSTED_BLOCK:
- try {
- if(_maxWait <= 0) {
- wait();
- } else {
- // this code may be executed again after a notify then continue cycle
- // so, need to calculate the amount of time to wait
- final long elapsed = (System.currentTimeMillis() - starttime);
- final long waitTime = _maxWait - elapsed;
- if (waitTime > 0)
- {
- wait(waitTime);
+
+ synchronized (this) {
+ assertOpen();
+ // if there are any sleeping, just grab one of those
+ try {
+ pair = (ObjectTimestampPair)(_pool.removeFirst());
+ } catch(NoSuchElementException e) {
+ ; /* ignored */
+ }
+
+ // otherwise
+ if(null == pair) {
+ // check if we can create one
+ // (note we know that the num sleeping is 0, else we wouldn't be here)
+ if(_maxActive < 0 || _numActive < _maxActive) {
+ // allow new object to be created
+ } else {
+ // the pool is exhausted
+ switch(_whenExhaustedAction) {
+ case WHEN_EXHAUSTED_GROW:
+ // allow new object to be created
+ break;
+ case WHEN_EXHAUSTED_FAIL:
+ throw new NoSuchElementException("Pool exhausted");
+ case WHEN_EXHAUSTED_BLOCK:
+ try {
+ if(_maxWait <= 0) {
+ wait();
+ } else {
+ // this code may be executed again after a notify then continue cycle
+ // so, need to calculate the amount of time to wait
+ final long elapsed = (System.currentTimeMillis() - starttime);
+ final long waitTime = _maxWait - elapsed;
+ if (waitTime > 0)
+ {
+ wait(waitTime);
+ }
}
+ } catch(InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw e;
}
- } catch(InterruptedException e) {
- Thread.currentThread().interrupt();
- throw e;
- }
- if(_maxWait > 0 && ((System.currentTimeMillis() - starttime) >= _maxWait)) {
- throw new NoSuchElementException("Timeout waiting for idle object");
- } else {
- continue; // keep looping
- }
- default:
- throw new IllegalArgumentException("WhenExhaustedAction property " + _whenExhaustedAction + " not recognized.");
+ if(_maxWait > 0 && ((System.currentTimeMillis() - starttime) >= _maxWait)) {
+ throw new NoSuchElementException("Timeout waiting for idle object");
+ } else {
+ continue; // keep looping
+ }
+ default:
+ throw new IllegalArgumentException("WhenExhaustedAction property " + _whenExhaustedAction + " not recognized.");
+ }
}
}
+ _numActive++;
}
- _numActive++;
// create new object when needed
boolean newlyCreated = false;
@@ -899,8 +901,10 @@
} finally {
if (!newlyCreated) {
// object cannot be created
- _numActive--;
- notifyAll();
+ synchronized (this) {
+ _numActive--;
+ notifyAll();
+ }
}
}
}
@@ -915,8 +919,10 @@
}
catch (Throwable e) {
// object cannot be activated or is invalid
- _numActive--;
- notifyAll();
+ synchronized (this) {
+ _numActive--;
+ notifyAll();
+ }
try {
_factory.destroyObject(pair.value);
}
@@ -933,7 +939,7 @@
}
}
- public synchronized void invalidateObject(Object obj) throws Exception {
+ public void invalidateObject(Object obj) throws Exception {
try {
if (_factory != null) {
_factory.destroyObject(obj);
@@ -941,8 +947,10 @@
} catch (Exception e) {
// swallowed
} finally {
- _numActive--;
- notifyAll(); // _numActive has changed
+ synchronized (this) {
+ _numActive--;
+ notifyAll(); // _numActive has changed
+ }
}
}
@@ -990,7 +998,7 @@
* the same object appearing multiple times in the pool and pool counters
* (numActive, numIdle) returning incorrect values.</p>
*/
- public synchronized void returnObject(Object obj) throws Exception {
+ public void returnObject(Object obj) throws Exception {
try {
addObjectToPool(obj, true);
} catch (Exception e) {
@@ -1005,35 +1013,39 @@
}
private void addObjectToPool(Object obj, boolean decrementNumActive) throws Exception {
- boolean success;
+ boolean success = true;
if(_testOnReturn && !(_factory.validateObject(obj))) {
success = false;
} else {
- success = false;
_factory.passivateObject(obj);
- success = !isClosed();
}
- boolean shouldDestroy = !success;
+ boolean shouldDestroy = false;
- if (decrementNumActive) {
- _numActive--;
- }
- if((_maxIdle >= 0) && (_pool.size() >= _maxIdle)) {
- shouldDestroy = true;
- } else if(success) {
- // borrowObject always takes the first element from the queue,
- // so for LIFO, push on top, FIFO add to end
- if (_lifo) {
- _pool.addFirst(new ObjectTimestampPair(obj));
+ synchronized (this) {
+ if (decrementNumActive) {
+ _numActive--;
+ }
+ if (isClosed()) {
+ shouldDestroy = true;
} else {
- _pool.addLast(new ObjectTimestampPair(obj));
+ if((_maxIdle >= 0) && (_pool.size() >= _maxIdle)) {
+ shouldDestroy = true;
+ } else if(success) {
+ // borrowObject always takes the first element from the queue,
+ // so for LIFO, push on top, FIFO add to end
+ if (_lifo) {
+ _pool.addFirst(new ObjectTimestampPair(obj));
+ } else {
+ _pool.addLast(new ObjectTimestampPair(obj));
+ }
+ }
}
+ notifyAll(); // _numActive has changed
}
- notifyAll(); // _numActive has changed
if(shouldDestroy) {
- try { // TODO: remove try/catch
+ try {
_factory.destroyObject(obj);
} catch(Exception e) {
// ignored
@@ -1169,13 +1181,25 @@
* Create an object, and place it into the pool.
* addObject() is useful for "pre-loading" a pool with idle objects.
*/
- public synchronized void addObject() throws Exception {
+ public void addObject() throws Exception {
assertOpen();
if (_factory == null) {
throw new IllegalStateException("Cannot add objects without a factory.");
}
Object obj = _factory.makeObject();
- addObjectToPool(obj, false);
+ synchronized (this) {
+ try {
+ assertOpen();
+ addObjectToPool(obj, false);
+ } catch (IllegalStateException ex) { // Pool closed
+ try {
+ _factory.destroyObject(obj);
+ } catch (Exception ex2) {
+ // swallow
+ }
+ throw ex;
+ }
+ }
}
//--- non-public methods ----------------------------------------
Modified: commons/proper/pool/trunk/xdocs/changes.xml
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/xdocs/changes.xml?rev=602773&r1=602772&r2=602773&view=diff
==============================================================================
--- commons/proper/pool/trunk/xdocs/changes.xml (original)
+++ commons/proper/pool/trunk/xdocs/changes.xml Sun Dec 9 18:53:10 2007
@@ -96,6 +96,13 @@
environments, can lead to memory leads and/or prevent applications
from shutting down or reloading cleanly.
</action>
+ <action dev="psteitz" type="fix" issue="POOL-93"
+ due-to="Mark Thomas">
+ Reduced synchronization in GenericObjectPool, GenericKeyedObjectPool.
+ Factory method activations within synchronized blocks were causing
+ performance problems in DBCP and other applications where factory
+ methods could block. Fixes both POOL-93 and POOL-108.
+ </action>
</release>
<release version="1.3" date="2006-pending" description="1.x bugfix release">