[
https://issues.apache.org/jira/browse/ZOOKEEPER-955?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12969363#action_12969363
]
Henry Robinson commented on ZOOKEEPER-955:
------------------------------------------
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!" ...
> Use Atomic(Integer|Long) for (Z)Xid
> -----------------------------------
>
> Key: ZOOKEEPER-955
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-955
> Project: ZooKeeper
> Issue Type: Improvement
> Components: java client, server
> Reporter: Thomas Koch
> Assignee: Thomas Koch
> Priority: Trivial
> Attachments: ZOOKEEPER-955.patch
>
>
> 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)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
