commons-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <p...@steitz.com>
Subject Re: [pool] addObject/makeObject synchronization again
Date Sat, 12 Jul 2008 15:18:09 GMT
Lee Breisacher wrote:
> Hi all. Referring to POOL-108, recall late last year there was a problem with GenericObjectPool
being overly synchronized. In particular, I'm concerned with addObject() and the fact that
the call to factory.makeObject() was synchronized and causing performance problems.
>
> At that time, Phil Steitz made some excellent suggestions for fixing it - see this JIRA
entry: http://issues.apache.org/jira/browse/POOL-108?focusedCommentId=12547740#action_12547740,
and the patches attached to POOL-108 indeed fix the problem. Related to this is POOL-93 which
has some similar patches attached to it. However, after the various patches were applied,
version 1.4 went out like this:
>
>     public synchronized void addObject() throws Exception {
>         . . . .
>         Object obj = _factory.makeObject();
>         . . . .
>
> Which means POOL-108 is not actually fixed! The performance problem is still there.
>
> I believe the call to makeObject should be outside of the lock (as seen in the patch
attached to POOL-108), so  it should look like this:
>
>     public void addObject() throws Exception {
>         assertOpen();
>         if (_factory == null) {
>             throw new IllegalStateException("Cannot add objects without a factory.");
>         }
>         Object obj = _factory.makeObject();
>         synchronized(this) {
>         try {
>             assertOpen();
>             addObjectToPool(obj, false);
>         } catch (IllegalStateException ex) { // Pool closed
>             try {
>                 _factory.destroyObject(obj);
>             } catch (Exception ex2) {
>                 // swallow
>             }
>             throw ex;
>         }
>       }
>     }
>
> I pulled over the sources and made this change in my local copy. One of the unit tests
failed, but closer examination revealed that the test was not quite right, so I fixed that
and now all the unit tests pass (both with the synchronized change and without it).  I'm working
on a new unit test right now that will demonstrate the problem with the current 1.4 (overly
synchronized) version.
>
> So, my primary question is: Does everyone agree with this fix? Should I re-open POOL-108,
or create a new JIRA, or is commons-pool 1.x at end-of-life and I should just use my locally
edited copy and be done with it?
>   
I would say reopen POOL-108 and attach your patehes.  We should also 
reopen POOL-120, as synchronization of addObject was restored to address 
this bug.
> Note that I discussed this briefly off-list with Phil and his response (with further
comments and questions from me) are below...
>
>   
>> 1) The fixes in 1.4 solve the bulk of the real practical
>> problem.  The hot-and-heavy access to a production pool
>> should not involve addObject.
>> This method is only used to pre-fill a pool or by the
>> (dreaded) Evictor when trying to maintain minIdle (a bad
>> practice in most cases).  The basic borrow-return sequence
>> that most apps hammer in production has the factory methods
>> outside of synxhronized scope now.
>>     
>
> I don't understand why the Evictor is "dreaded" and why minIdle is a "bad practice".
Can someone elaborate on that?
Sorry for the late response here.  The evictor is "dreaded" by me 
because it makes maintaining the component very tricky.  That's just my 
HO.  Maintaining minIdle may be a bad practice in some cases because it 
can lead to lots of object creation churn, especially if maxIdle is also 
set and the values are not far apart.
>  Maintaining a minIdle works fabulously for our application. The objects in our pool
are rather heavyweight and can take seve ral seconds to create. Also, our usage pattern is
such that a small number of objects in the pool is usually sufficient, but occasional spikes
require extra objects, so haviing the background Evictor (obviously not a great name for it
now that it also maintains the minIdle level) keeping the pool filled is a good thing. 
What I worry about there is that the Evictor just runs on a timer and 
requires an exclusive lock on the pool while it runs.  There is no 
guarantee that it is not going to kick in during a load spike and make 
things potentially much worse.  It is possible to "keep the pool filled" 
in the sense of making sure that maxActive instances are available or in 
use at any time by not setting maxIdle (so the pool is never trimmed 
back) and/or setting initialSize.  An alternative strategy to keep ahead 
of demand used by some other pool implementations is to support a 
configurable increment of instances to add when the pool is growing 
(i.e., when numActive < maxActive and numIdle = 0).  I would be open to 
adding such a feature to commons pool.
> at 
>
>   
>> 2) addObject is broken in another way that I did not want to
>> make worse in 1.4 - it does no accounting, so it is possible
>> to add more than maxActive objects to the pool using this
>> method.  Forcing it to lock the pool while it does its work
>> makes it less likely that the pool will grow way beyond
>> maxActive when invoking this method.  This issue raises a
>> larger problem that is hard to address with backward
>> compatibility in mind, which is what exactly does maxActive
>> mean?  The documentation is not chrystal clear and any way to
>> make it clear requires either a convoluted explanation of all
>> of the strange corner cases that people may be depending on
>> in their applications or cleaning things up in a way that may
>> break some people.
>>     
>
> I'm not sure this problem really exists (I found the JIRA for it - POOL-120). Like I
said, when I tweaked the unit tests slightly, I could no longer reproduce this problem (with
the "weaker" synchronization like I'm suggesting).  And yeah, we don't use maxActive (does
anyone?) and it does seem like a bit of an odd feature. We do have a notion of maxTotal (idle
+ active), but that's outside the pool implementation.
>   
Many users depend on maxActive to keep the number of instances created 
by the pool bounded.  The documentation is unclear and needs to be made 
definitive here.  We can track and resolve this issue via POOL-120.
> Anyway, thanks for your time...
>   

Thank YOU for your feedback and suggestions.

Phil


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


Mime
View raw message