commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1164051 - /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
Date Fri, 02 Sep 2011 01:31:15 GMT
On 2 September 2011 01:23, Phil Steitz <phil.steitz@gmail.com> 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, 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.
>
> 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: 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
>>
>>
>
>
> ---------------------------------------------------------------------
> 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


Mime
View raw message