Performance improvement to default mcp.

[whitingjr]

Hi,

I've been looking at the default mcp with the aim of improving performance. It's the class org.jboss.jca.core.connectionmanager.pool.mcp.SemaphoreArrayListManagedConnectionPool

By re-jigging some code in the returnConnection method I've been able to improve response times on average by 15%. Using a jee benchmark that loads a system using the JCA (IronJacamar) to manage JMS and datastore resources.

There are three changes:

a) Moved the condition check on the 'kill' variable outside the synchronized block.

b) Moved the call to ConnectionListener.used() outside the synchronized block.

c) Removed the assignment of a ConnectionListener local variable.

Running the testsuite for the core module didn't show any test failures.

Which code branch should I apply the changes and then share for you to review them ?

Regards,

Jeremy

[jesper.pedersen]

a) Moved the condition check on the 'kill' variable outside the synchronized block.

Modifications of 'cls' needs to be under a lock - or maybe you are looking at old code ?

b) Moved the call to ConnectionListener.used() outside the synchronized block.

That needs further investigation, as it has side effects for the ManagedConnectionPool contract.

c) Removed the assignment of a ConnectionListener local variable.

Possible, yes.

Which code branch should I apply the changes and then share for you to review them ?

Pull requests are against either the 1.2 branch or master.

[whitingjr]

Modifications of 'cls' needs to be under a lock - or maybe you are looking at old code ?

The change retains the modification of cls under a lock.

I'll create a branch off 1.2 and push to my fork.

[whitingjr]

I created a branch called "sem-armcp-reduced-work-during-lock" from 1.2 tip.

whitingjr/ironjacamar at sem-armcp-reduced-work-during-lock · GitHub

[whitingjr]

Modifications of 'cls' needs to be under a lock - or maybe you are looking at old code ?

So modifying the cls is still under a lock for both branches of the if statement

https://github.com/whitingjr/ironjacamar/blob/59694f15ca35421675cbb27d778298e9e599a25c/core/src/main/java/org/jboss/jca/core/connectionmanager/pool/mcp/SemaphoreArrayListManagedConnectionPool.java#L662

and

https://github.com/whitingjr/ironjacamar/blob/59694f15ca35421675cbb27d778298e9e599a25c/core/src/main/java/org/jboss/jca/core/connectionmanager/pool/mcp/SemaphoreArrayListManagedConnectionPool.java#L680

How does that look to you ?

b) Moved the call to ConnectionListener.used() outside the synchronized block.

That needs further investigation, as it has side effects for the ManagedConnectionPool contract.

Which part of the contract does it affect ?

Possible, yes.

ironjacamar/core/src/main/java/org/jboss/jca/core/connectionmanager/pool/mcp/SemaphoreArrayListManagedConnectionPool.jav…

That should achieve the same outcome unless I am mistaken. Or have I missed something.

Pull requests are against either the 1.2 branch or master.

Ok I branched off 1.2 to keep the change as close to 1.x as possible.

[jesper.pedersen]

I have investigated this issue. The cl.used() call can happen outside any possible lock controlling the free connections data structure. It is up to the actual implementation of the ManagedConnectionPool interface to account for this.

For SemaphoreArrayListManagedConnectionPool this gives an improvement when there is no load on the XAResource instance, so I have merged the change plus a developer guide update into the 1.2 branch.

NoLoad: DO_RAMPUP=true

Load: DO_RAMPUP=false, TX_BEGIN_DURATION=2L, TX_COMMIT_DURATION=10L

The rest of the performance test suite parameters left at default.