Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Received-SPF: pass (athena.apache.org: domain of sebbaz@gmail.com designates
 209.85.212.43 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <4E602207.8090309@gmail.com>
References: <20110901124154.EB1A72388A02@eris.apache.org>
	<4E6017FA.2000706@gmail.com>
	<4E601C6C.9070904@apache.org>
	<4E602207.8090309@gmail.com>
Date: Fri, 2 Sep 2011 02:31:15 +0100
Message-ID: 
 <CAOGo0VajN607uUiKivuQt6Z-EOAqs_X87g8Q3OMchtE=w4_MWw@mail.gmail.com>
Subject: Re: svn commit: r1164051 -
 /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
From: sebb <sebbaz@gmail.com>
To: Commons Developers List <dev@commons.apache.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

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 =A01 12:41:54 2011
>>>> New Revision: 1164051
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=3D1164051&view=3Drev
>>>> 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. =A0I 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. =A0What I have
> always worried about is that once we removed the sync on this and
> the pool statistics getters, what it computes becomes suspect. =A0For
> example, if things enter / exit the idle object pool between the
> time objectDeficit and growLimit are computed, the return value will
> be incorrect. =A0As 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:
>>>> =A0 =A0 commons/proper/pool/trunk/src/java/org/apache/commons/pool2/im=
pl/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/o=
rg/apache/commons/pool2/impl/GenericKeyedObjectPool.java?rev=3D1164051&r1=
=3D1164050&r2=3D1164051&view=3Ddiff
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
>>>> --- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/G=
enericKeyedObjectPool.java (original)
>>>> +++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/G=
enericKeyedObjectPool.java Thu Sep =A01 12:41:54 2011
>>>> @@ -1577,25 +1577,30 @@ public class GenericKeyedObjectPool<K,T>
>>>> =A0 =A0 =A0 * @param pool the ObjectPool to calculate the deficit for
>>>> =A0 =A0 =A0 * @return The number of objects to be created
>>>> =A0 =A0 =A0 */
>>>> - =A0 =A0private synchronized int calculateDeficit(ObjectDeque<T> obje=
ctDeque) {
>>>> + =A0 =A0private int calculateDeficit(ObjectDeque<T> objectDeque) {
>>>>
>>>> =A0 =A0 =A0 =A0 =A0if (objectDeque =3D=3D null) {
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0return getMinIdlePerKey();
>>>> =A0 =A0 =A0 =A0 =A0}
>>>> +
>>>> + =A0 =A0 =A0 =A0// Used more than once so keep a local copy so the va=
lue is consistent
>>>> + =A0 =A0 =A0 =A0int maxTotal =3D getMaxTotal();
>>>> + =A0 =A0 =A0 =A0int maxTotalPerKey =3D getMaxTotalPerKey();
>>>> +
>>>> =A0 =A0 =A0 =A0 =A0int objectDefecit =3D 0;
>>>> -
>>>> +
>>>> =A0 =A0 =A0 =A0 =A0// Calculate no of objects needed to be created, in=
 order to have
>>>> =A0 =A0 =A0 =A0 =A0// the number of pooled objects < maxTotalPerKey();
>>>> =A0 =A0 =A0 =A0 =A0objectDefecit =3D getMinIdlePerKey() - objectDeque.=
getIdleObjects().size();
>>>> - =A0 =A0 =A0 =A0if (getMaxTotalPerKey() > 0) {
>>>> + =A0 =A0 =A0 =A0if (maxTotalPerKey > 0) {
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0int growLimit =3D Math.max(0,
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0getMaxTotalPerKey() - objectD=
eque.getIdleObjects().size());
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0maxTotalPerKey - objectDeque.=
getIdleObjects().size());
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0objectDefecit =3D Math.min(objectDefecit, g=
rowLimit);
>>>> =A0 =A0 =A0 =A0 =A0}
>>>>
>>>> =A0 =A0 =A0 =A0 =A0// Take the maxTotal limit into account
>>>> - =A0 =A0 =A0 =A0if (getMaxTotal() > 0) {
>>>> - =A0 =A0 =A0 =A0 =A0 =A0int growLimit =3D Math.max(0, getMaxTotal() -=
 getNumActive() - getNumIdle());
>>>> + =A0 =A0 =A0 =A0if (maxTotal > 0) {
>>>> + =A0 =A0 =A0 =A0 =A0 =A0int growLimit =3D Math.max(0, maxTotal - getN=
umActive() - getNumIdle());
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0objectDefecit =3D Math.min(objectDefecit, g=
rowLimit);
>>>> =A0 =A0 =A0 =A0 =A0}
>>>>
>>>>
>>>>
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> 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