Reduce contention by making borrow & return more independent from each other

• Key: POOL-93
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Holger Hoffstätte
• Affects Version/s: 1.1, 1.2, 1.3
• Fix Version/s: 1.4

Description

Currently borrow & return are completely blocked from each other, while at least the factory-based validation & destruction can be handled independently. A few simple changes narrow the synchronization blocks yet retain overall correctness.

Comments

[Holger Hoffstätte]

Patch for making returnObject & invalidateObject less synchronized. All unit tests still pass, even with massive slashdotting (100+ threads).

[Holger Hoffstätte]

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.

[Holger Hoffstätte]

Benchmark results. Summary: JDK 1.4 and 1.5 are really slow

[Holger Hoffstätte]

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.

[Sandy McArthur]

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.

[Holger Hoffstätte]

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.

[Marcos Sanz]

Patch for GenericPool and performance comparison before/after applying

[Marcos Sanz]

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.

[Mark Thomas]

The proposed patches all look good to me.

I have attached a new patch that:

• combines the two previous patches proposed
• aligns the GOP and GKOP code for the methods impacted by the patches
• implements all sync fixes identified by each patch for both GOP and GKOP
• narrows the syncs on borrowObject() for both

The new patch still passess all unit tests.

[Mark Thomas]

I have attached a new patch that incorporates the proposed fix for POOL-108. This makes the assertOpen() test occur in a thread safe manner. POOL-108 patched GOP, this patch ports the same changes to GKOP.

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 POOL-108.

[Phil Steitz]

Patch applied (pool-93-markt-v2.patch ) in r602773. Thanks!