[ https://issues.apache.org/jira/browse/HADOOP-12475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14958308#comment-14958308 ]
Sangjin Lee commented on HADOOP-12475: -------------------------------------- The latest patch looks good for the most part. A couple of nits: IMO it's generally a good practice to declare types with interfaces rather than concrete implementations: {code} 129 private ConcurrentHashMap<ConnectionId, Connection> connections = 130 new ConcurrentHashMap<>(); {code} I would use {{ConcurrentMap}} instead of {{ConcurrentHashMap}} as the type. Also line 130 has the checkstyle violation. It'd be good to fix it too. > Replace guava Cache with ConcurrentHashMap for caching Connection in ipc > Client > ------------------------------------------------------------------------------- > > Key: HADOOP-12475 > URL: https://issues.apache.org/jira/browse/HADOOP-12475 > Project: Hadoop Common > Issue Type: Sub-task > Components: conf, io, ipc > Reporter: Walter Su > Assignee: Walter Su > Attachments: HADOOP-12475.01.patch, HADOOP-12475.02.patch > > > quote [~daryn] from HADOOP-11772: > {quote} > CacheBuilder is obscenely expensive for concurrent map, and it requires > generating unnecessary garbage even just to look up a key. Replace it with > ConcurrentHashMap. > I identified this issue that impaired my own perf testing under load. The > slowdown isn't just the sync. It's the expensive of Connection's ctor > stalling other connections. The expensive of ConnectionId#equals causes > delays. Synch'ing on connections causes unfair contention unlike a sync'ed > method. Concurrency simply hides this. > {quote} > BTW, guava Cache is heavyweight. Per local test, ConcurrentHashMap has better > overal performance. -- This message was sent by Atlassian JIRA (v6.3.4#6332)