You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@commons.apache.org by Lee Breisacher <LB...@seagullsoftware.com> on 2008/07/07 18:46:12 UTC
[pool] addObject/makeObject synchronization again
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?
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? Maintaining a minIdle works fabulously for our application. The objects in our pool are rather heavyweight and can take several 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 having 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.
> 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.
Anyway, thanks for your time...
Lee
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
For additional commands, e-mail: user-help@commons.apache.org
Re: [pool] addObject/makeObject synchronization again
Posted by Phil Steitz <ph...@steitz.com>.
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
RE: [pool] addObject/makeObject synchronization again
Posted by Lee Breisacher <LB...@seagullsoftware.com>.
Oops - never mind -- I had the wrong configuration on the pool. But all the discussion about synchronization still applies. Sorry...
Lee
> -----Original Message-----
> From: Lee Breisacher [mailto:LBreisacher@seagullsoftware.com]
> Sent: Monday, July 07, 2008 10:58 AM
> To: Jakarta Commons Users List
> Subject: RE: [pool] addObject/makeObject synchronization again
>
> Hm, now I've just into something else that's quite puzzling.
> When I set the WhenExhaustedAction to WHEN_EXHAUSTED_BLOCK,
> doing a returnObject() does not seem to unblock the borrower.
> Am I completely misunderstanding how WHEN_EXHAUSTED_BLOCK is
> supposed to work? Here's the test:
>
> - in thread A, exhaust the pool by borrowing all the
> available objects, then call borrowObject() once more. That waits.
> - in thread B, return one of the borrowed objects. That
> should unblock thread A, right? It does not.
>
> Is this a gaping bug - hard to believe it's never been
> encountered before.
>
> Thanks,
>
> Lee
>
> > -----Original Message-----
> > From: Lee Breisacher [mailto:LBreisacher@seagullsoftware.com]
> > Sent: Monday, July 07, 2008 9:46 AM
> > To: Jakarta Commons Users List
> > Subject: [pool] addObject/makeObject synchronization again
> >
> > 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?
> >
> > 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?
> > Maintaining a minIdle works fabulously for our application.
> > The objects in our pool are rather heavyweight and can take several
> > 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 having 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.
> >
> > > 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.
> >
> > Anyway, thanks for your time...
> >
> > Lee
> >
> >
> ---------------------------------------------------------------------
> > To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
> > For additional commands, e-mail: user-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
> For additional commands, e-mail: user-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
For additional commands, e-mail: user-help@commons.apache.org
RE: [pool] addObject/makeObject synchronization again
Posted by Lee Breisacher <LB...@seagullsoftware.com>.
Hm, now I've just into something else that's quite puzzling. When I set the WhenExhaustedAction to WHEN_EXHAUSTED_BLOCK, doing a returnObject() does not seem to unblock the borrower. Am I completely misunderstanding how WHEN_EXHAUSTED_BLOCK is supposed to work? Here's the test:
- in thread A, exhaust the pool by borrowing all the available objects, then call borrowObject() once more. That waits.
- in thread B, return one of the borrowed objects. That should unblock thread A, right? It does not.
Is this a gaping bug - hard to believe it's never been encountered before.
Thanks,
Lee
> -----Original Message-----
> From: Lee Breisacher [mailto:LBreisacher@seagullsoftware.com]
> Sent: Monday, July 07, 2008 9:46 AM
> To: Jakarta Commons Users List
> Subject: [pool] addObject/makeObject synchronization again
>
> 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?
>
> 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?
> Maintaining a minIdle works fabulously for our application.
> The objects in our pool are rather heavyweight and can take
> several 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
> having 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.
>
> > 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.
>
> Anyway, thanks for your time...
>
> Lee
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
> For additional commands, e-mail: user-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
For additional commands, e-mail: user-help@commons.apache.org