10 Replies Latest reply on Nov 8, 2006 10:15 AM by Manik Surtani

    Should acquired locks be released in the order they were obt

    Manik Surtani Master

      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.


        • 1. Re: Should acquired locks be released in the order they were
          Manik Surtani Master

          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.

          • 2. Re: Should acquired locks be released in the order they were
            Brian Dueck Newbie

            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

            • 3. Re: Should acquired locks be released in the order they were
              Bela Ban Master

               

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

              • 4. Re: Should acquired locks be released in the order they were
                Bela Ban Master

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

                • 5. Re: Should acquired locks be released in the order they were
                  Brian Dueck Newbie

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

                  Brian.

                  • 6. Re: Should acquired locks be released in the order they were
                    Brian Dueck Newbie

                    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.

                    • 7. Re: Should acquired locks be released in the order they were
                      Manik Surtani Master

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

                      • 8. Re: Should acquired locks be released in the order they were
                        Ben Wang Master

                        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.

                        • 9. Re: Should acquired locks be released in the order they were
                          Brian Dueck Newbie

                          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.

                          • 10. Re: Should acquired locks be released in the order they were
                            Manik Surtani Master

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