Should acquired locks be released in the order they were obt

[manik]

From http://www.jboss.com/index.html?module=bb&op=viewtopic&p=3915487#3915487

General performance and concurrency improvements for TransactionEntry
=====================================================================
org.jboss.cache.TransactionEntry
- changed TransactionEntry.locks from List based implementation to
ConcurrentReaderHashMap. This avoids expensive linear scanning calls
to locks.contains() in addLock() and also improves concurrency by
avoiding blocking synchronized() 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.

[manik]

I made a quick change to my local checkout, changing TransactionEntry.locks to be a Set rather than a List, and TransactionEntry.getLocks() to return a Set.

All the unit tests passed, and I can't see a reason why the locks need to be released in the order in which they are created.

Still though, I've revert back to Bela's comment about whether this change is warranted by perf numbers.

[bdueck]

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

[belaban]

"bdueck" wrote:

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).

[belaban]

Yes, numbers would be interesting. Especially the perf improvements you get after implementing the suggested modifications.

[bdueck]

Agreed - the access pattern of my application (and benchmark) is well within the 80/20 read/write ratio or better.

Brian.

[bdueck]

Manik;

Assuming I can justifiy the change based on improved performance & scalability, are you comfortable with the change from a functional perspective?

i.e. Can you see any downsides to the change since it means we will no longer guarantee to release locks in the order they were acquired?

Brian.

[manik]

I can't see any real reason why the locks need to be released in the order in which they were created. Functionally, the unit tests pass as well. So yeah, I'm comfortable with it ...

[ben.wang]

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.

[bdueck]

Folks;

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.

Thanks,

Brian.

[manik]

I'm still ok with this, are you thinking of putting this in the 1.4.x branch? If so, I hope to release a beta on this branch pretty soon (next day or two...)