Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: java client, server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      Atomic

      Description

      As I've read last weekend in the fantastic book "Clean Code", it'd be much faster to use AtomicInteger or AtomicLong instead of synchronization blocks around each access to an int or long.
      The key difference is, that a synchronization block will in any case acquire and release a lock. The atomic classes use "optimistic locking", a CPU operation that only changes a value if it still has not changed since the last read.
      In most cases the value has not changed since the last visit so the operation is just as fast as a normal operation. If it had changed, then we read again and try to change again.

      [1] Clean Code: A Handbook of Agile Software Craftsmanship (Robert C. Martin)

        Activity

        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12465683/ZOOKEEPER-955.patch
        against trunk revision 1040752.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 8 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/63//testReport/
        Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/63//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/63//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12465683/ZOOKEEPER-955.patch against trunk revision 1040752. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 8 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/63//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/63//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/63//console This message is automatically generated.
        Hide
        Benjamin Reed added a comment -

        +1 looks good. have you benchmarked to make sure that it improved performance? (or at least didn't make it worse

        Show
        Benjamin Reed added a comment - +1 looks good. have you benchmarked to make sure that it improved performance? (or at least didn't make it worse
        Hide
        Henry Robinson added a comment -

        Hi Thomas -

        Nice thought - definitely worth checking out. Please do benchmark this, however - it is not completely clear cut which is the better synchronization primitive.

        Atomic* objects usually call into a system-specific compare and swap method, usually (on x86) a CMPXCHG instruction which locks the bus whether or not the compare is successful. This is probably the same hardware synchronization primitive that Java's thin locks use.

        Java locks are usually some form of thin locks, at least in HotSpot [1], which are locks that are fast in the uncontended case - they are just as fast as CAS on words. In the contended case, the lock is inflated which is a one-time spinning step, after which it is a OS-based mutex which we would expect to be slower than Atomic primitives in general. However, when lock contention is very high, you might find that synchronized blocks perform better as optimistic locking implies a busy wait loop when a lock can't be taken.

        Henry

        [1] http://wikis.sun.com/display/HotSpotInternals/Synchronization

        Show
        Henry Robinson added a comment - Hi Thomas - Nice thought - definitely worth checking out. Please do benchmark this, however - it is not completely clear cut which is the better synchronization primitive. Atomic* objects usually call into a system-specific compare and swap method, usually (on x86) a CMPXCHG instruction which locks the bus whether or not the compare is successful. This is probably the same hardware synchronization primitive that Java's thin locks use. Java locks are usually some form of thin locks, at least in HotSpot [1] , which are locks that are fast in the uncontended case - they are just as fast as CAS on words. In the contended case, the lock is inflated which is a one-time spinning step, after which it is a OS-based mutex which we would expect to be slower than Atomic primitives in general. However, when lock contention is very high, you might find that synchronized blocks perform better as optimistic locking implies a busy wait loop when a lock can't be taken. Henry [1] http://wikis.sun.com/display/HotSpotInternals/Synchronization
        Hide
        Mahadev konar added a comment -

        nice explanation henry! never knew about this!

        Show
        Mahadev konar added a comment - nice explanation henry! never knew about this!
        Hide
        Thomas Koch added a comment -

        Unfortunately I don't have access to a cluster or even one big dedicated machine to run benchmarks that would make any sense. And frankly I'm afraid of stress testing ZooKeeper on my laptop, which would also mean stress testing my 3 years old hard disc...

        Show
        Thomas Koch added a comment - Unfortunately I don't have access to a cluster or even one big dedicated machine to run benchmarks that would make any sense. And frankly I'm afraid of stress testing ZooKeeper on my laptop, which would also mean stress testing my 3 years old hard disc...
        Hide
        Mahadev konar added a comment -

        ben/henry, any one of you can volunteer for running some benchmarks?

        Show
        Mahadev konar added a comment - ben/henry, any one of you can volunteer for running some benchmarks?
        Hide
        Henry Robinson added a comment -

        Based on a very quick look at the code, I'd be surprised if this were a bottleneck. There are roughly two important call sites for these methods: one is in the PrepRequestProcessor and one is in the ResponderThread during responses to leader requests. I don't think there's a reasonable benchmark that would show any difference between the two threads based on their behaviour alone.

        The question, then, is whether synchronizing on the ZooKeeperServer is fine-grained enough as other methods are taking the 'this' lock on instances of ZKS. Moving to AtomicLong would begin to partition the locking strategy for ZKS, and since AtomicLong can't be used in a thread-unsafe way there's none of the normal concerns with fine-grained locking about making the locking strategy more complex and therefore error-prone. However in general we should be very cautious about re-partitioning locks; this can cause all sorts of pain. We should certainly understand the contention profiles for certain locks before coming up with a strategy.

        Given that, I'm +0 on this patch. I'm not sure it really solves a significant problem, but it has the potential to ease synchronization costs, and doesn't cost much in terms of resources. It would also fix the problem in my version of trunk where getZxid is synchronized, but has this comment: "This should be called from a synchronized block on this!" ...

        Show
        Henry Robinson added a comment - Based on a very quick look at the code, I'd be surprised if this were a bottleneck. There are roughly two important call sites for these methods: one is in the PrepRequestProcessor and one is in the ResponderThread during responses to leader requests. I don't think there's a reasonable benchmark that would show any difference between the two threads based on their behaviour alone. The question, then, is whether synchronizing on the ZooKeeperServer is fine-grained enough as other methods are taking the 'this' lock on instances of ZKS. Moving to AtomicLong would begin to partition the locking strategy for ZKS, and since AtomicLong can't be used in a thread-unsafe way there's none of the normal concerns with fine-grained locking about making the locking strategy more complex and therefore error-prone. However in general we should be very cautious about re-partitioning locks; this can cause all sorts of pain. We should certainly understand the contention profiles for certain locks before coming up with a strategy. Given that, I'm +0 on this patch. I'm not sure it really solves a significant problem, but it has the potential to ease synchronization costs, and doesn't cost much in terms of resources. It would also fix the problem in my version of trunk where getZxid is synchronized, but has this comment: "This should be called from a synchronized block on this!" ...
        Hide
        Thomas Koch added a comment -

        I'm myself +0 on this patch after what I've learned about synchronization now. It was just a WTF moment when I saw this code and asked myself why the author didn't use Atomic classes from the beginning. The comment you mentioned ("This should be called from a synchronized block on this!") is obsolete, I believe since the function is already synchronized.

        Show
        Thomas Koch added a comment - I'm myself +0 on this patch after what I've learned about synchronization now. It was just a WTF moment when I saw this code and asked myself why the author didn't use Atomic classes from the beginning. The comment you mentioned ("This should be called from a synchronized block on this!") is obsolete, I believe since the function is already synchronized.
        Hide
        Benjamin Reed added a comment -

        following up on henry's comment, it appears that we are indeed calling some of these from synchronized blocks, but they are really only synchronized for the call itself. we should remove those as well since it would be confusing to mix synchronization and AtomicX. it would also allow us to make sure there isn't a side effect of the synchronization that we are missing.

        Show
        Benjamin Reed added a comment - following up on henry's comment, it appears that we are indeed calling some of these from synchronized blocks, but they are really only synchronized for the call itself. we should remove those as well since it would be confusing to mix synchronization and AtomicX. it would also allow us to make sure there isn't a side effect of the synchronization that we are missing.
        Hide
        Mahadev konar added a comment -

        cancelling the patch, since we arent clear if we need it...

        Show
        Mahadev konar added a comment - cancelling the patch, since we arent clear if we need it...
        Hide
        Mahadev konar added a comment -

        marking it for 3.4 if someone can run some performance numbers with this, we can include it in 3.4 release.

        Show
        Mahadev konar added a comment - marking it for 3.4 if someone can run some performance numbers with this, we can include it in 3.4 release.
        Hide
        Mahadev konar added a comment -

        not a blocker. Moving it out of 3.4 release.

        Show
        Mahadev konar added a comment - not a blocker. Moving it out of 3.4 release.
        Hide
        Michi Mutsuzaki added a comment -

        Please let me know if this is something we still want to do. Otherwise I'll close it as wontfix.

        Show
        Michi Mutsuzaki added a comment - Please let me know if this is something we still want to do. Otherwise I'll close it as wontfix.

          People

          • Assignee:
            Thomas Koch
            Reporter:
            Thomas Koch
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development