On 2 September 2011 01:23, Phil Steitz <[email protected]> wrote: > On 9/1/11 4:59 PM, Mark Thomas wrote: >> On 02/09/2011 00:40, Phil Steitz wrote: >>> On 9/1/11 5:41 AM, [email protected] wrote: >>>> Author: markt >>>> Date: Thu Sep 1 12:41:54 2011 >>>> New Revision: 1164051 >>>> >>>> URL: http://svn.apache.org/viewvc?rev=1164051&view=rev >>>> Log: >>>> Remove sync that wasn't helping. >>>> Make thread safe >>> I agree that the sync wasn't helping; but I am not sure exactly what >>> "thread safe" means for this method. >> For me, thread safe means that a local copy is taken for any value used >> more than once that needs to be consistent between uses. I'm not sure we >> can do much more without tying ourselves in synchronisation knots. >> >>> I have always worried about >>> this method. I have usually been able to convince myself that it's >>> incorrectness is harmless (i.e., there are guards to prevent pool >>> invariants being violated due to incorrect / inconsistent return >>> value), but I am not sure that is right. >> My analysis was that if you reduce the limits (maxTotal, maxIdle etc.) >> while this method is executing then there is a risk that it could exceed >> those limits. However, there is a fair chance that the pool will already >> be exceeding the limits when the limits are changed. Therefore, there is >> actually very little difference between these two scenarios. In both >> cases the pool will conform to the limits as soon as circumstances allow >> and once operating within the limits, will not exceed them. > > I have never worried much about races against setters for maxTotal, > MaxIdle, though it is good to fix this as you did. What I have > always worried about is that once we removed the sync on this and > the pool statistics getters, what it computes becomes suspect. For > example, if things enter / exit the idle object pool between the > time objectDeficit and growLimit are computed, the return value will > be incorrect. As I said, IIRC previous traces through the code, the > use of the return value has guards to prevent too much badness from > this; but since the correctness of the computation done inside this > method depends on the idle instance pool keeping the same size over > the duration of its execution, it is neither really threadsafe nor > correct. >> >>> It might be good to >>> temporarily make this method protected and run some concurrency >>> tests directly against it, so we can understand and document what >>> happens when instances come in and out of the pool while it is >>> executing. >> I'm not against that but I don't have the time to do it right now and am >> unlikely to any time soon. > > I will do some tracing and more analysis of the code to verify that > the problems that I think may exist in the correctness (or maybe > meaningfulness) of the return value are harmless.
There's another threading issue - the JVM can cache shared variables locally in a thread. i.e. a reader thread may not see the latest copies of of all variables written by another thread. (in theory it might even see a half-written long variable) This is easily fixed by making variables volatile, but that can have a performance impact. > Phil >> >> Mark >> >> >>> Phil >>>> Modified: >>>> >>>> commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.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=1164051&r1=1164050&r2=1164051&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 >>>> Thu Sep 1 12:41:54 2011 >>>> @@ -1577,25 +1577,30 @@ public class GenericKeyedObjectPool<K,T> >>>> * @param pool the ObjectPool to calculate the deficit for >>>> * @return The number of objects to be created >>>> */ >>>> - private synchronized int calculateDeficit(ObjectDeque<T> objectDeque) >>>> { >>>> + private int calculateDeficit(ObjectDeque<T> objectDeque) { >>>> >>>> if (objectDeque == null) { >>>> return getMinIdlePerKey(); >>>> } >>>> + >>>> + // Used more than once so keep a local copy so the value is >>>> consistent >>>> + int maxTotal = getMaxTotal(); >>>> + int maxTotalPerKey = getMaxTotalPerKey(); >>>> + >>>> int objectDefecit = 0; >>>> - >>>> + >>>> // Calculate no of objects needed to be created, in order to have >>>> // the number of pooled objects < maxTotalPerKey(); >>>> objectDefecit = getMinIdlePerKey() - >>>> objectDeque.getIdleObjects().size(); >>>> - if (getMaxTotalPerKey() > 0) { >>>> + if (maxTotalPerKey > 0) { >>>> int growLimit = Math.max(0, >>>> - getMaxTotalPerKey() - >>>> objectDeque.getIdleObjects().size()); >>>> + maxTotalPerKey - objectDeque.getIdleObjects().size()); >>>> objectDefecit = Math.min(objectDefecit, growLimit); >>>> } >>>> >>>> // Take the maxTotal limit into account >>>> - if (getMaxTotal() > 0) { >>>> - int growLimit = Math.max(0, getMaxTotal() - getNumActive() - >>>> getNumIdle()); >>>> + if (maxTotal > 0) { >>>> + int growLimit = Math.max(0, maxTotal - getNumActive() - >>>> getNumIdle()); >>>> objectDefecit = Math.min(objectDefecit, growLimit); >>>> } >>>> >>>> >>>> >>>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: [email protected] >>> For additional commands, e-mail: [email protected] >>> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> >> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
