zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Mollitor (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (ZOOKEEPER-3365) Use Concurrent HashMap in NettyServerCnxnFactory
Date Sun, 14 Apr 2019 15:20:00 GMT

     [ https://issues.apache.org/jira/browse/ZOOKEEPER-3365?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

David Mollitor updated ZOOKEEPER-3365:
--------------------------------------
    Description: 
{code:java|title=NettyServerCnxnFactory.java}
    // Access to ipMap or to any Set contained in the map needs to be
    // protected with synchronized (ipMap) { ... }
    private final Map<InetAddress, Set<NettyServerCnxn>> ipMap = new HashMap<>();

    private void addCnxn(NettyServerCnxn cnxn) {
        cnxns.add(cnxn);
        synchronized (ipMap){
            InetAddress addr =
                ((InetSocketAddress)cnxn.getChannel().remoteAddress()).getAddress();
            Set<NettyServerCnxn> s = ipMap.get(addr);
            if (s == null) {
                s = new HashSet<>();
                ipMap.put(addr, s);
            }
            s.add(cnxn);
        }
    }
{code}

This can be done better (less code, less contention) with Java 8 Map API.  Although, as I
look at this, the only thing this is used for is a count of the number of connections from
each address.  Maybe this should just store a count instead of a collection.

https://github.com/apache/zookeeper/blob/f69ad1b0fed88da3c1b67fd73031e7248c0564f7/zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java

Also note that an exclusive lock is required with each interaction of the table.  By moving
to a {{ConcurrentHashMap}}:

bq. Retrieval operations (including get) generally do not block, so may overlap with update
operations (including put and remove).

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html

Removing this lock should improve ZK's performance for highly concurrent client workloads,
especially since its Async Netty operations, unless of course there are other locks elsewhere.

  was:
{code:java|title=NettyServerCnxnFactory.java}
    // Access to ipMap or to any Set contained in the map needs to be
    // protected with synchronized (ipMap) { ... }
    private final Map<InetAddress, Set<NettyServerCnxn>> ipMap = new HashMap<>();

    private void addCnxn(NettyServerCnxn cnxn) {
        cnxns.add(cnxn);
        synchronized (ipMap){
            InetAddress addr =
                ((InetSocketAddress)cnxn.getChannel().remoteAddress()).getAddress();
            Set<NettyServerCnxn> s = ipMap.get(addr);
            if (s == null) {
                s = new HashSet<>();
                ipMap.put(addr, s);
            }
            s.add(cnxn);
        }
    }
{code}

This can be done better (less code, less contention) with Java 8 Map API.  Although, as I
look at this, the only thing this is used for is a count of the number of connections from
each address.  Maybe this should just store a count instead of a collection.

https://github.com/apache/zookeeper/blob/f69ad1b0fed88da3c1b67fd73031e7248c0564f7/zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java


> Use Concurrent HashMap in NettyServerCnxnFactory 
> -------------------------------------------------
>
>                 Key: ZOOKEEPER-3365
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-3365
>             Project: ZooKeeper
>          Issue Type: Improvement
>          Components: server
>    Affects Versions: 3.6.0
>            Reporter: David Mollitor
>            Assignee: David Mollitor
>            Priority: Minor
>              Labels: pull-request-available
>             Fix For: 3.6.0
>
>          Time Spent: 1.5h
>  Remaining Estimate: 0h
>
> {code:java|title=NettyServerCnxnFactory.java}
>     // Access to ipMap or to any Set contained in the map needs to be
>     // protected with synchronized (ipMap) { ... }
>     private final Map<InetAddress, Set<NettyServerCnxn>> ipMap = new HashMap<>();
>     private void addCnxn(NettyServerCnxn cnxn) {
>         cnxns.add(cnxn);
>         synchronized (ipMap){
>             InetAddress addr =
>                 ((InetSocketAddress)cnxn.getChannel().remoteAddress()).getAddress();
>             Set<NettyServerCnxn> s = ipMap.get(addr);
>             if (s == null) {
>                 s = new HashSet<>();
>                 ipMap.put(addr, s);
>             }
>             s.add(cnxn);
>         }
>     }
> {code}
> This can be done better (less code, less contention) with Java 8 Map API.  Although,
as I look at this, the only thing this is used for is a count of the number of connections
from each address.  Maybe this should just store a count instead of a collection.
> https://github.com/apache/zookeeper/blob/f69ad1b0fed88da3c1b67fd73031e7248c0564f7/zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java
> Also note that an exclusive lock is required with each interaction of the table.  By
moving to a {{ConcurrentHashMap}}:
> bq. Retrieval operations (including get) generally do not block, so may overlap with
update operations (including put and remove).
> https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html
> Removing this lock should improve ZK's performance for highly concurrent client workloads,
especially since its Async Netty operations, unless of course there are other locks elsewhere.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message