General performance and concurrency improvements for TransactionEntry=====================================================================org.jboss.cache.TransactionEntry- changed TransactionEntry.locks from List based implementation toConcurrentReaderHashMap. This avoids expensive linear scanning callsto locks.contains() in addLock() and also improves concurrency byavoiding blocking synchronized() calls where most of the access tolocks is read-only. I'm using ConcurrentReaderHashMap here more asa Set than a Map since there is no value to store. We're really justinterested in efficient searches in locks and highly concurrent reads.

- I'm not sure of the impact of this change onorg.jboss.cache.interceptors.OptimisticLockingInterceptor andorg.jboss.cache.interceptors.PessimisticLockInterceptorwhich might be relying on some implementation specifics of the order ofinsertion of entries into what getLocks() returns. Seems to me thoughthat if they do have some sort of dependency on ordering that thereshould be a better way to deal with this...

- this change impacts one public method - getLocks() which previouslyreturned a java.util.List and now returns a java.util.Set. Only one testis affected by this (org.jboss.cache.cache.optimistic.OpLockingInterceptorTest)which depends on List.get(int) method that isn't available in Set.

Agreed that all changes should be justified based on quantified improvements.

My methodology for identifying this and the other changes I've made was by running a performance and concurrency stress test I've developed for JBossCache that simulates the way my application uses the cache.

That test gives me performance numbers that let me determine the throughput I'm getting on the cache across all threads as well as on a per thread basis. I also run JProbe to help pinpoint problems and issue kill -3 and analyze the output to try and pinpoint thread contention.

I have access to some pretty decent performance testing hardware, ranging from 2 to 8 processor Wintel machines, to 24 way Sun and HP boxes.

So, I run the test on different hardware with different numbers of CPUs and OSs to validate that I get consistent benefit (or at least no drawback) on all systems.

I'll try and provide a breakdown that quantifies each improvement I'm suggesting

My methodology for identifying this and the other changes I've made was by running a performance and concurrency stress test I've developed for JBossCache that simulates the way my application uses the cache.

We must also avoid tuning JBossCache for access patterns that are outside the designed-for patterns. Not that I suggest your application does that, looks like most of your suggestions make sense in general. But we always have to keep the usual apps in mind (e.g. 80% R 20% W).

Just to think out loud, for the pessimistic locking, I am trying to think whether a different release ordering will cause any semantics breakdown or deadlock? Nothing I can think of now but maybe something to keep in mind.

This is a bit of a stale thread, but I do now have some data that confirms the change I proposed to TransactionEntry does indeed have a positive effect on scalability. My tests show it improves overall scalability by up to 6%. Some tests show no improvement, but none show degredation.

Can everyone please re-review the proposal below as well as the earlier posts in this thread? If there are no objections, I'll make the change this week.

org.jboss.cache.TransactionEntry - changed TransactionEntry.locks from List based implementation to ConcurrentReaderHashMap. This avoids expensive linear scannng calls to locks.contains() in addLock() and also improves concurrency by avoiding blocking synchroized() calls where most of the access to locks is read-only. I'm using ConcurrentReaderHashMap here more as a Set than a Map since there is no value to store. We're really just interested in efficient searches in locks and highly concurrent reads.

- I'm not sure of the impact of this change on org.jboss.cache.interceptors.OptimisticLockingInterceptor and org.jboss.cache.interceptors.PessimisticLockInterceptor which might be relying on some implementation specifics of the order of insertion of entries into what getLocks() returns. Seems to me though that if they do have some sort of dependency on ordering that there should be a better way to deal with this...

- this change impacts one public method - getLocks() which previously returned a java.util.List and now returns a java.util.Set. Only one test is affected by this (org.jboss.cache.cache.optimistic.OpLockingInterceptorTest) which depends on List.get(int) method that isn't available in Set.