Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Message-ID: <4E601C6C.9070904@apache.org>
Date: Fri, 02 Sep 2011 00:59:40 +0100
From: Mark Thomas <markt@apache.org>
User-Agent: Mozilla/5.0 (Windows NT 5.2; WOW64;
 rv:6.0.1) Gecko/20110830 Thunderbird/6.0.1
MIME-Version: 1.0
To: Commons Developers List <dev@commons.apache.org>
Subject: Re: svn commit: r1164051 -
 /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
References: <20110901124154.EB1A72388A02@eris.apache.org>
 <4E6017FA.2000706@gmail.com>
In-Reply-To: <4E6017FA.2000706@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 02/09/2011 00:40, Phil Steitz wrote:
> On 9/1/11 5:41 AM, markt@apache.org 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.

> 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.

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: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org