commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pste...@apache.org
Subject 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
Date Mon, 10 Dec 2007 02:53:14 GMT
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">



Mime
View raw message