You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by David Graham <gr...@yahoo.com> on 2003/08/13 21:32:10 UTC
Re: [pool] faulty change of synchronization? - Re: cvs commit: jakarta-commons/pool/src/java/org/apache/commons/pool/impl GenericObjectPool.java
--- Dirk Verbeeck <di...@pandora.be> wrote:
> The pool manipulation was synchronized, only the isEmpty() test is not.
> I didn't realize that _numActive++ should be synchronized, I thought the
>
> ++ operator was atomic.
The datatype the operator is applied to is what matters. Assignments to
ints are atomic, assignments to longs aren't because they are 64 bits.
So, if _numActive is an int, I don't think it needs to be synchronized.
David
> I'll put these also in a synchronized block.
>
>
http://cvs.apache.org/viewcvs/jakarta-commons/pool/src/java/org/apache/commons/pool/impl/GenericObjectPool.java?rev=1.23&content-type=text/vnd.viewcvs-markup
>
> How does this new version looks?
>
> Dirk
>
> John McNally wrote:
>
> >My quick read of this change is not favorable. It looks like changes
> to
> >an unsynchronized list have been moved outside any synchronization.
> >Also integer math like numActive++ outside a synchronization block has
> >got me into problems in the past. A test with two threads using i++
> and
> >i-- ran quite some time (couple hours) on a single processor box, but
> >did eventually fail. Then the test was run on a dual processor box, it
> >failed pretty much immediately, repeatedly.
> >
> >I'm -1 on this change.
> >
> >john mcnally
> >
> >
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
__________________________________
Do you Yahoo!?
Yahoo! SiteBuilder - Free, easy-to-use web site design software
http://sitebuilder.yahoo.com
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [pool] faulty change of synchronization? - Re: cvs commit:
jakarta-commons/pool/src/java/org/apache/commons/pool/impl
GenericObjectPool.java
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
Hmm..are you sure that simply relying on "atomic" behaviour is what you
want here?
I'm not an expert on java's "memory model", but suggest you give some
serious thought before removing any "synchronized" keywords.
As I understand it, "atomic" simply means that no two threads will ever
see intermediate values of the variable as it changes.
eg for a 32 bit int (made of 2 16-bit words), you can never see a state
where one of the 16-bit words has been updated but not the other.
Example: an int = 0x0000FFFF, add 1. No thread can ever see the low word
roll over to all zeros before the high word rolls over to 0x0001.
It says nothing about whether other threads will see the new value, or
an old cached version of the value. That is what synchronized or
volatile keywords do (see also: "memory barrier"). In practice, you will
never see a problem on a single-cpu machine; therefore any unit tests
checking for thread-safety will never fail on a single-cpu machine, but
will on a multi-cpu machine.
Example:
thread 1 on CPU 1 reads an int, and it is cached in the cpu's cache.
thread 2 on CPU 2 reads the same int, increments it, and writes it back
to memory, without using synchronized/volatile.
thread 1 on CPU 1 now increments its *cached* version & writes it to
memory.
result: a "missing" increment.
With synchronized or volatile, when thread 2 writes to memory, the cache
of CPU1 is forced to invalidate the cached data containing the written
address. When thread1 tries to access that variable again, it therefore
is forced to fetch the new value from memory.
The above should be taken with a grain of salt - I haven't got time to
double-check this with the java specs at the moment. Hope I'm not
misleading you..
Regards,
Simon
On Thu, 2003-08-14 at 09:15, Dirk Verbeeck wrote:
> _numActive is an int so the synchronized block wasn't needed.
> But the extra "synchronized" is in the case where a new object
> has to be created and not in the critical "get from pool" code path.
>
> All the success code paths have now one synchronized block.
> I'll leave it as is for now.
>
> Dirk
>
>
> David Graham wrote:
>
> >--- Dirk Verbeeck <di...@pandora.be> wrote:
> >
> >
> >>The pool manipulation was synchronized, only the isEmpty() test is not.
> >>I didn't realize that _numActive++ should be synchronized, I thought the
> >>
> >>++ operator was atomic.
> >>
> >>
> >
> >The datatype the operator is applied to is what matters. Assignments to
> >ints are atomic, assignments to longs aren't because they are 64 bits.
> >So, if _numActive is an int, I don't think it needs to be synchronized.
> >
> >David
> >
> >
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
Re: [pool] faulty change of synchronization? - Re: cvs commit: jakarta-commons/pool/src/java/org/apache/commons/pool/impl
GenericObjectPool.java
Posted by Dirk Verbeeck <di...@pandora.be>.
Hi Marc
Very nice link you gave, very interesting.
The changes I'm doing is a work in progress. Before releasing it we will
do a formal review including a vote,
beta test on release canidate and then the actual release. You will get
enough time to review.
But the code is not very good yet. There should be a simpler way to
solve these synchronization issues.
I'll have another go at it...
Dirk
Marc Slemko wrote:
>jOn Wed, 13 Aug 2003, Dirk Verbeeck wrote:
>
>
>
>>_numActive is an int so the synchronized block wasn't needed.
>>But the extra "synchronized" is in the case where a new object
>>has to be created and not in the critical "get from pool" code path.
>>
>>All the success code paths have now one synchronized block.
>>I'll leave it as is for now.
>>
>>
>
>No, you need the synchronization (or some other thread safe approach),
>otherwise you risk all sorts of errors. There are a very limited
>set of cases where you can avoid synchronization when you need concurrent
>access from multiple threads.
>
>Synchronization in java handles both mutual exclusion and visibility.
>There is no guarantee that a change made in one thread to a variable
>is visible to another without synchronization.
>
>See some of the references linked to from:
>
> http://www.cs.umd.edu/~pugh/java/memoryModel/
>
>There appear to be all sorts of race conditions in borrowObject() now...
>_numActive, accesses to _pool, _maxActive... you can't check the value of
>a counter outside a synchronized block then only synchronize to modify it!
>
>I think you need to step back and reconsider what problems these changes
>are trying to fix. You need to find the real operation that is
>bottlenecking things (which for connection pools would normally be calling
>into the JDBC driver to open a connection), and move that operation
>outside the synchronized block without doing unsafe things.
>
>Now, ideally that whole thing would be reworked even further to have the
>connection opener in a seperate thread to allow the getConnection() to
>time out after a specified timeout, while it keeps trying to open the
>connection in the background. In fact, if you do that you may be able to
>just leave the whole method synchronized and wait on the opener thread,
>but there are some extra efforts required to ensure fairness, etc. that
>may make that more complex.
>
>I strongly recommend you roll back the synchronization changes unless
>someone who fully understands what has to be synchronized and why can
>review possible fixes, I don't have time just now, maybe next week; the
>current state is _NOT_ good enough.
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
>
>
>
>
Re: [pool] faulty change of synchronization? - Re: cvs commit: jakarta-commons/pool/src/java/org/apache/commons/pool/impl
GenericObjectPool.java
Posted by Dirk Verbeeck <di...@pandora.be>.
Hi Marc
Very nice link you gave, very interesting.
The changes I'm doing is a work in progress. Before releasing it we will
do a formal review including a vote,
beta test on release canidate and then the actual release. You will get
enough time to review.
But the code is not very good yet. There should be a simpler way to
solve these synchronization issues.
I'll have another go at it...
Dirk
Marc Slemko wrote:
>jOn Wed, 13 Aug 2003, Dirk Verbeeck wrote:
>
>
>
>>_numActive is an int so the synchronized block wasn't needed.
>>But the extra "synchronized" is in the case where a new object
>>has to be created and not in the critical "get from pool" code path.
>>
>>All the success code paths have now one synchronized block.
>>I'll leave it as is for now.
>>
>>
>
>No, you need the synchronization (or some other thread safe approach),
>otherwise you risk all sorts of errors. There are a very limited
>set of cases where you can avoid synchronization when you need concurrent
>access from multiple threads.
>
>Synchronization in java handles both mutual exclusion and visibility.
>There is no guarantee that a change made in one thread to a variable
>is visible to another without synchronization.
>
>See some of the references linked to from:
>
> http://www.cs.umd.edu/~pugh/java/memoryModel/
>
>There appear to be all sorts of race conditions in borrowObject() now...
>_numActive, accesses to _pool, _maxActive... you can't check the value of
>a counter outside a synchronized block then only synchronize to modify it!
>
>I think you need to step back and reconsider what problems these changes
>are trying to fix. You need to find the real operation that is
>bottlenecking things (which for connection pools would normally be calling
>into the JDBC driver to open a connection), and move that operation
>outside the synchronized block without doing unsafe things.
>
>Now, ideally that whole thing would be reworked even further to have the
>connection opener in a seperate thread to allow the getConnection() to
>time out after a specified timeout, while it keeps trying to open the
>connection in the background. In fact, if you do that you may be able to
>just leave the whole method synchronized and wait on the opener thread,
>but there are some extra efforts required to ensure fairness, etc. that
>may make that more complex.
>
>I strongly recommend you roll back the synchronization changes unless
>someone who fully understands what has to be synchronized and why can
>review possible fixes, I don't have time just now, maybe next week; the
>current state is _NOT_ good enough.
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [pool] faulty change of synchronization? - Re: cvs commit:
jakarta-commons/pool/src/java/org/apache/commons/pool/impl GenericObjectPool.java
Posted by Marc Slemko <ma...@znep.com>.
jOn Wed, 13 Aug 2003, Dirk Verbeeck wrote:
> _numActive is an int so the synchronized block wasn't needed.
> But the extra "synchronized" is in the case where a new object
> has to be created and not in the critical "get from pool" code path.
>
> All the success code paths have now one synchronized block.
> I'll leave it as is for now.
No, you need the synchronization (or some other thread safe approach),
otherwise you risk all sorts of errors. There are a very limited
set of cases where you can avoid synchronization when you need concurrent
access from multiple threads.
Synchronization in java handles both mutual exclusion and visibility.
There is no guarantee that a change made in one thread to a variable
is visible to another without synchronization.
See some of the references linked to from:
http://www.cs.umd.edu/~pugh/java/memoryModel/
There appear to be all sorts of race conditions in borrowObject() now...
_numActive, accesses to _pool, _maxActive... you can't check the value of
a counter outside a synchronized block then only synchronize to modify it!
I think you need to step back and reconsider what problems these changes
are trying to fix. You need to find the real operation that is
bottlenecking things (which for connection pools would normally be calling
into the JDBC driver to open a connection), and move that operation
outside the synchronized block without doing unsafe things.
Now, ideally that whole thing would be reworked even further to have the
connection opener in a seperate thread to allow the getConnection() to
time out after a specified timeout, while it keeps trying to open the
connection in the background. In fact, if you do that you may be able to
just leave the whole method synchronized and wait on the opener thread,
but there are some extra efforts required to ensure fairness, etc. that
may make that more complex.
I strongly recommend you roll back the synchronization changes unless
someone who fully understands what has to be synchronized and why can
review possible fixes, I don't have time just now, maybe next week; the
current state is _NOT_ good enough.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [pool] faulty change of synchronization? - Re: cvs commit:
jakarta-commons/pool/src/java/org/apache/commons/pool/impl
GenericObjectPool.java
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
Hmm..are you sure that simply relying on "atomic" behaviour is what you
want here?
I'm not an expert on java's "memory model", but suggest you give some
serious thought before removing any "synchronized" keywords.
As I understand it, "atomic" simply means that no two threads will ever
see intermediate values of the variable as it changes.
eg for a 32 bit int (made of 2 16-bit words), you can never see a state
where one of the 16-bit words has been updated but not the other.
Example: an int = 0x0000FFFF, add 1. No thread can ever see the low word
roll over to all zeros before the high word rolls over to 0x0001.
It says nothing about whether other threads will see the new value, or
an old cached version of the value. That is what synchronized or
volatile keywords do (see also: "memory barrier"). In practice, you will
never see a problem on a single-cpu machine; therefore any unit tests
checking for thread-safety will never fail on a single-cpu machine, but
will on a multi-cpu machine.
Example:
thread 1 on CPU 1 reads an int, and it is cached in the cpu's cache.
thread 2 on CPU 2 reads the same int, increments it, and writes it back
to memory, without using synchronized/volatile.
thread 1 on CPU 1 now increments its *cached* version & writes it to
memory.
result: a "missing" increment.
With synchronized or volatile, when thread 2 writes to memory, the cache
of CPU1 is forced to invalidate the cached data containing the written
address. When thread1 tries to access that variable again, it therefore
is forced to fetch the new value from memory.
The above should be taken with a grain of salt - I haven't got time to
double-check this with the java specs at the moment. Hope I'm not
misleading you..
Regards,
Simon
On Thu, 2003-08-14 at 09:15, Dirk Verbeeck wrote:
> _numActive is an int so the synchronized block wasn't needed.
> But the extra "synchronized" is in the case where a new object
> has to be created and not in the critical "get from pool" code path.
>
> All the success code paths have now one synchronized block.
> I'll leave it as is for now.
>
> Dirk
>
>
> David Graham wrote:
>
> >--- Dirk Verbeeck <di...@pandora.be> wrote:
> >
> >
> >>The pool manipulation was synchronized, only the isEmpty() test is not.
> >>I didn't realize that _numActive++ should be synchronized, I thought the
> >>
> >>++ operator was atomic.
> >>
> >>
> >
> >The datatype the operator is applied to is what matters. Assignments to
> >ints are atomic, assignments to longs aren't because they are 64 bits.
> >So, if _numActive is an int, I don't think it needs to be synchronized.
> >
> >David
> >
> >
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [pool] faulty change of synchronization? - Re: cvs commit:
jakarta-commons/pool/src/java/org/apache/commons/pool/impl GenericObjectPool.java
Posted by Marc Slemko <ma...@znep.com>.
jOn Wed, 13 Aug 2003, Dirk Verbeeck wrote:
> _numActive is an int so the synchronized block wasn't needed.
> But the extra "synchronized" is in the case where a new object
> has to be created and not in the critical "get from pool" code path.
>
> All the success code paths have now one synchronized block.
> I'll leave it as is for now.
No, you need the synchronization (or some other thread safe approach),
otherwise you risk all sorts of errors. There are a very limited
set of cases where you can avoid synchronization when you need concurrent
access from multiple threads.
Synchronization in java handles both mutual exclusion and visibility.
There is no guarantee that a change made in one thread to a variable
is visible to another without synchronization.
See some of the references linked to from:
http://www.cs.umd.edu/~pugh/java/memoryModel/
There appear to be all sorts of race conditions in borrowObject() now...
_numActive, accesses to _pool, _maxActive... you can't check the value of
a counter outside a synchronized block then only synchronize to modify it!
I think you need to step back and reconsider what problems these changes
are trying to fix. You need to find the real operation that is
bottlenecking things (which for connection pools would normally be calling
into the JDBC driver to open a connection), and move that operation
outside the synchronized block without doing unsafe things.
Now, ideally that whole thing would be reworked even further to have the
connection opener in a seperate thread to allow the getConnection() to
time out after a specified timeout, while it keeps trying to open the
connection in the background. In fact, if you do that you may be able to
just leave the whole method synchronized and wait on the opener thread,
but there are some extra efforts required to ensure fairness, etc. that
may make that more complex.
I strongly recommend you roll back the synchronization changes unless
someone who fully understands what has to be synchronized and why can
review possible fixes, I don't have time just now, maybe next week; the
current state is _NOT_ good enough.
Re: [pool] faulty change of synchronization? - Re: cvs commit: jakarta-commons/pool/src/java/org/apache/commons/pool/impl
GenericObjectPool.java
Posted by Dirk Verbeeck <di...@pandora.be>.
_numActive is an int so the synchronized block wasn't needed.
But the extra "synchronized" is in the case where a new object
has to be created and not in the critical "get from pool" code path.
All the success code paths have now one synchronized block.
I'll leave it as is for now.
Dirk
David Graham wrote:
>--- Dirk Verbeeck <di...@pandora.be> wrote:
>
>
>>The pool manipulation was synchronized, only the isEmpty() test is not.
>>I didn't realize that _numActive++ should be synchronized, I thought the
>>
>>++ operator was atomic.
>>
>>
>
>The datatype the operator is applied to is what matters. Assignments to
>ints are atomic, assignments to longs aren't because they are 64 bits.
>So, if _numActive is an int, I don't think it needs to be synchronized.
>
>David
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [pool] faulty change of synchronization? - Re: cvs commit:
jakarta-commons/pool/src/java/org/apache/commons/pool/impl
GenericObjectPool.java
Posted by John McNally <jm...@collab.net>.
public class TestAtomicIncrement
{
int j;
public TestAtomicIncrement()
{
j = 10;
}
public static void main(String[] args)
throws Exception
{
TestAtomicIncrement tai = new TestAtomicIncrement();
tai.j++;
int i = 10;
i++;
}
}
[jmcnally@localhost small-tests]$ javac TestAtomicIncrement.java
[jmcnally@localhost small-tests]$ javap -c TestAtomicIncrement
Compiled from TestAtomicIncrement.java
public class TestAtomicIncrement extends java.lang.Object {
int j;
public TestAtomicIncrement();
public static void main(java.lang.String[]) throws
java.lang.Exception;
}
Method TestAtomicIncrement()
0 aload_0
1 invokespecial #1 <Method java.lang.Object()>
4 aload_0
5 bipush 10
7 putfield #2 <Field int j>
10 return
Method void main(java.lang.String[])
0 new #3 <Class TestAtomicIncrement>
3 dup
4 invokespecial #4 <Method TestAtomicIncrement()>
7 astore_1
8 aload_1
9 dup
10 getfield #2 <Field int j>
13 iconst_1
14 iadd
15 putfield #2 <Field int j>
18 bipush 10
20 istore_2
21 iinc 2 1
24 return
I think the getfield bytecode is supposed to implemented in such a way
that it is atomic for an int field. It is possible the putfield
bytecode is as well, i don't know the jls that well. But it is doubtful
that the method#'s 10-15 comprise an atomic operation.
john mcnally
On Wed, 2003-08-13 at 12:32, David Graham wrote:
> --- Dirk Verbeeck <di...@pandora.be> wrote:
> > The pool manipulation was synchronized, only the isEmpty() test is not.
> > I didn't realize that _numActive++ should be synchronized, I thought the
> >
> > ++ operator was atomic.
>
> The datatype the operator is applied to is what matters. Assignments to
> ints are atomic, assignments to longs aren't because they are 64 bits.
> So, if _numActive is an int, I don't think it needs to be synchronized.
>
> David
>
> > I'll put these also in a synchronized block.
> >
> >
> http://cvs.apache.org/viewcvs/jakarta-commons/pool/src/java/org/apache/commons/pool/impl/GenericObjectPool.java?rev=1.23&content-type=text/vnd.viewcvs-markup
> >
> > How does this new version looks?
> >
> > Dirk
> >
> > John McNally wrote:
> >
> > >My quick read of this change is not favorable. It looks like changes
> > to
> > >an unsynchronized list have been moved outside any synchronization.
> > >Also integer math like numActive++ outside a synchronization block has
> > >got me into problems in the past. A test with two threads using i++
> > and
> > >i-- ran quite some time (couple hours) on a single processor box, but
> > >did eventually fail. Then the test was run on a dual processor box, it
> > >failed pretty much immediately, repeatedly.
> > >
> > >I'm -1 on this change.
> > >
> > >john mcnally
> > >
> > >
> >
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> > For additional commands, e-mail: commons-dev-help@jakarta.apache.org
> >
>
>
> __________________________________
> Do you Yahoo!?
> Yahoo! SiteBuilder - Free, easy-to-use web site design software
> http://sitebuilder.yahoo.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [pool] faulty change of synchronization? - Re: cvs commit: jakarta-commons/pool/src/java/org/apache/commons/pool/impl
GenericObjectPool.java
Posted by Dirk Verbeeck <di...@pandora.be>.
_numActive is an int so the synchronized block wasn't needed.
But the extra "synchronized" is in the case where a new object
has to be created and not in the critical "get from pool" code path.
All the success code paths have now one synchronized block.
I'll leave it as is for now.
Dirk
David Graham wrote:
>--- Dirk Verbeeck <di...@pandora.be> wrote:
>
>
>>The pool manipulation was synchronized, only the isEmpty() test is not.
>>I didn't realize that _numActive++ should be synchronized, I thought the
>>
>>++ operator was atomic.
>>
>>
>
>The datatype the operator is applied to is what matters. Assignments to
>ints are atomic, assignments to longs aren't because they are 64 bits.
>So, if _numActive is an int, I don't think it needs to be synchronized.
>
>David
>
>
Re: [pool] faulty change of synchronization? - Re: cvs commit:
jakarta-commons/pool/src/java/org/apache/commons/pool/impl
GenericObjectPool.java
Posted by John McNally <jm...@collab.net>.
public class TestAtomicIncrement
{
int j;
public TestAtomicIncrement()
{
j = 10;
}
public static void main(String[] args)
throws Exception
{
TestAtomicIncrement tai = new TestAtomicIncrement();
tai.j++;
int i = 10;
i++;
}
}
[jmcnally@localhost small-tests]$ javac TestAtomicIncrement.java
[jmcnally@localhost small-tests]$ javap -c TestAtomicIncrement
Compiled from TestAtomicIncrement.java
public class TestAtomicIncrement extends java.lang.Object {
int j;
public TestAtomicIncrement();
public static void main(java.lang.String[]) throws
java.lang.Exception;
}
Method TestAtomicIncrement()
0 aload_0
1 invokespecial #1 <Method java.lang.Object()>
4 aload_0
5 bipush 10
7 putfield #2 <Field int j>
10 return
Method void main(java.lang.String[])
0 new #3 <Class TestAtomicIncrement>
3 dup
4 invokespecial #4 <Method TestAtomicIncrement()>
7 astore_1
8 aload_1
9 dup
10 getfield #2 <Field int j>
13 iconst_1
14 iadd
15 putfield #2 <Field int j>
18 bipush 10
20 istore_2
21 iinc 2 1
24 return
I think the getfield bytecode is supposed to implemented in such a way
that it is atomic for an int field. It is possible the putfield
bytecode is as well, i don't know the jls that well. But it is doubtful
that the method#'s 10-15 comprise an atomic operation.
john mcnally
On Wed, 2003-08-13 at 12:32, David Graham wrote:
> --- Dirk Verbeeck <di...@pandora.be> wrote:
> > The pool manipulation was synchronized, only the isEmpty() test is not.
> > I didn't realize that _numActive++ should be synchronized, I thought the
> >
> > ++ operator was atomic.
>
> The datatype the operator is applied to is what matters. Assignments to
> ints are atomic, assignments to longs aren't because they are 64 bits.
> So, if _numActive is an int, I don't think it needs to be synchronized.
>
> David
>
> > I'll put these also in a synchronized block.
> >
> >
> http://cvs.apache.org/viewcvs/jakarta-commons/pool/src/java/org/apache/commons/pool/impl/GenericObjectPool.java?rev=1.23&content-type=text/vnd.viewcvs-markup
> >
> > How does this new version looks?
> >
> > Dirk
> >
> > John McNally wrote:
> >
> > >My quick read of this change is not favorable. It looks like changes
> > to
> > >an unsynchronized list have been moved outside any synchronization.
> > >Also integer math like numActive++ outside a synchronization block has
> > >got me into problems in the past. A test with two threads using i++
> > and
> > >i-- ran quite some time (couple hours) on a single processor box, but
> > >did eventually fail. Then the test was run on a dual processor box, it
> > >failed pretty much immediately, repeatedly.
> > >
> > >I'm -1 on this change.
> > >
> > >john mcnally
> > >
> > >
> >
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> > For additional commands, e-mail: commons-dev-help@jakarta.apache.org
> >
>
>
> __________________________________
> Do you Yahoo!?
> Yahoo! SiteBuilder - Free, easy-to-use web site design software
> http://sitebuilder.yahoo.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org