|
[
Permlink
| « Hide
]
Holger Hoffstätte added a comment - 24/Nov/06 03:24 PM
Patch for making returnObject & invalidateObject less synchronized. All unit tests still pass, even with massive slashdotting (100+ threads).
One note on the relaxing of returnObject():
_testOnReturn is now accessed without prior synchronization, which strictly speaking is wrong. In reality this does not matter because the value is only a configuration property and (very likely) not changed during runtime. The real fix here would be to make this and other boolean ivars volatile and the get/set methods unsynchronized; let me now if you would like another patch for that. The same can be done for the int ivars, though increment/decrement must still be properly synchronized for atomicity; with JDK 1.5 or backport these should be replaced with Atomic(Integer/Long). In any case using volatiles is a nice way to reduce synchronization blocks from the method level to the appropriate section of code that actually needs to be synchronized. Benchmark results. Summary: JDK 1.4 and 1.5 are really slow
 One more note on the benchmarks: all this was done on a single-core P4 2.4GHz. Experiments on a dual-core Opteron show that even with JDK6 not more than 120% CPU load was attained because of the overall contention, most likely borrowObject().
My guess is that more expensive validate/return factory methods will give more concrete results of increased throughput. First look at the patch looks good.
Any reason not to replace direct access of _testOnReturn with calls to getTestOnReturn() ? This would be thread-safe and allow newer HotSpot versions do the analysis to determine if the synchronization of getTestOnReturn() is really needed. The problem with volatile is we still need to support Java 1.4 and it wasn't until 1.5 JVMs that volatile was handled correctly in all JVMs. Thought about that too; AFAIK volatile is not really broken any more, at least not for volatile reads & synchronized writes as done in the backport (see e.g. Atomic*). Also found this: http://www.cs.umd.edu/~pugh/java/memoryModel/archive/1888.html
 Got to draw the line somewhere Maybe use the getter for 1.3.x and then assume a working volatile for 2.0? That way we can also start relaxing other areas where possible, step by step. Patch for GenericPool and performance comparison before/after applying
I have included a simmilar patch to Holger's, this time for the GenericObjectPool.
I have refrained from introducing volatile variables.I have also refrained from synchronizing the access to _testOnReturn, which is really not worth bothering. The performance improvement in borrow/return time after applying the patch reaches in some cases one order of magnitude. All unit tests passed. I think further improvement in the liveness of this class can be done, mainly by alleviating synchronization in the borrowObject() method. The proposed patches all look good to me.
I have attached a new patch that:
The new patch still passess all unit tests. I have attached a new patch that incorporates the proposed fix for
On a similar note, I forgot to mention that the previous version also included changes so the use of isClosed() was thread-safe. v2 of the patch should now be a complete patch for the sync improvements suggested here and for Patch applied (pool-93-markt-v2.patch ) in r602773. Thanks!
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Resolved
Major
Reduce contention by making borrow & return more independent from each other
