From c6a13e28b93b7b4d1f99f556cc894765a4c8e2f1 Mon Sep 17 00:00:00 2001
From: Dmitriy Dumanskiy <doom369@gmail.com>
Date: Tue, 2 Aug 2016 12:44:48 +0300
Subject: [PATCH] Improvement : constant pool now less concurrent

Current constant pool holds all data within HashMap and all access to this HashMap is done via synchronized blocks. Thus CuncurrentHashMap will be here more efficient as it designed for higher throughput and will use less locks. Also valueOf method was not very efficient as it performed get operation 2 times.

Modifications :

HashMap -> PlatformDependent.newConcurrentHashMap().
ValueOf is more efficient now, threadsafe and uses less locks. Downside is that final T tempConstant = newConstant(nextId(), name); could be called more than 1 time during high contention.

Result :

Less contention, cleaner code.
---
 .../main/java/io/netty/util/ConstantPool.java | 74 ++++++++++---------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/common/src/main/java/io/netty/util/ConstantPool.java b/common/src/main/java/io/netty/util/ConstantPool.java
index c5b376c32c..66e723438b 100644
--- a/common/src/main/java/io/netty/util/ConstantPool.java
+++ b/common/src/main/java/io/netty/util/ConstantPool.java
@@ -17,9 +17,10 @@
 package io.netty.util;
 
 import io.netty.util.internal.ObjectUtil;
+import io.netty.util.internal.PlatformDependent;
 
-import java.util.HashMap;
-import java.util.Map;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicInteger;
 
 /**
  * A pool of {@link Constant}s.
@@ -28,9 +29,9 @@ import java.util.Map;
  */
 public abstract class ConstantPool<T extends Constant<T>> {
 
-    private final Map<String, T> constants = new HashMap<String, T>();
+    private final ConcurrentMap<String, T> constants = PlatformDependent.newConcurrentHashMap();
 
-    private int nextId = 1;
+    private AtomicInteger nextId = new AtomicInteger(1);
 
     /**
      * Shortcut of {@link #valueOf(String) valueOf(firstNameComponent.getName() + "#" + secondNameComponent)}.
@@ -55,17 +56,26 @@ public abstract class ConstantPool<T extends Constant<T>> {
      * @param name the name of the {@link Constant}
      */
     public T valueOf(String name) {
-        T c;
+        checkNotNullAndNotEmpty(name);
+        return getOrCreate(name);
+    }
 
-        synchronized (constants) {
-            if (exists(name)) {
-                c = constants.get(name);
-            } else {
-                c = newInstance0(name);
+    /**
+     * Get existing constant by name or creates new one if not exists. Threadsafe
+     *
+     * @param name the name of the {@link Constant}
+     */
+    private T getOrCreate(String name) {
+        T constant = constants.get(name);
+        if (constant == null) {
+            final T tempConstant = newConstant(nextId(), name);
+            constant = constants.putIfAbsent(name, tempConstant);
+            if (constant == null) {
+                return tempConstant;
             }
         }
 
-        return c;
+        return constant;
     }
 
     /**
@@ -73,34 +83,34 @@ public abstract class ConstantPool<T extends Constant<T>> {
      */
     public boolean exists(String name) {
         checkNotNullAndNotEmpty(name);
-        synchronized (constants) {
-            return constants.containsKey(name);
-        }
+        return constants.containsKey(name);
     }
 
     /**
      * Creates a new {@link Constant} for the given {@code name} or fail with an
      * {@link IllegalArgumentException} if a {@link Constant} for the given {@code name} exists.
      */
-    @SuppressWarnings("unchecked")
     public T newInstance(String name) {
-        if (exists(name)) {
-            throw new IllegalArgumentException(String.format("'%s' is already in use", name));
-        }
-
-        T c = newInstance0(name);
-
-        return c;
+        checkNotNullAndNotEmpty(name);
+        return createOrThrow(name);
     }
 
-    // Be careful that this dose not check whether the argument is null or empty.
-    private T newInstance0(String name) {
-        synchronized (constants) {
-            T c = newConstant(nextId, name);
-            constants.put(name, c);
-            nextId++;
-            return c;
+    /**
+     * Creates constant by name or throws exception. Threadsafe
+     *
+     * @param name the name of the {@link Constant}
+     */
+    private T createOrThrow(String name) {
+        T constant = constants.get(name);
+        if (constant == null) {
+            final T tempConstant = newConstant(nextId(), name);
+            constant = constants.putIfAbsent(name, tempConstant);
+            if (constant == null) {
+                return tempConstant;
+            }
         }
+
+        throw new IllegalArgumentException(String.format("'%s' is already in use", name));
     }
 
     private String checkNotNullAndNotEmpty(String name) {
@@ -117,10 +127,6 @@ public abstract class ConstantPool<T extends Constant<T>> {
 
     @Deprecated
     public final int nextId() {
-        synchronized (constants) {
-            int id = nextId;
-            nextId++;
-            return id;
-        }
+        return nextId.getAndIncrement();
     }
 }