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.

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 @@
  */
 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,52 +56,61 @@ public T valueOf(Class<?> firstNameComponent, String secondNameComponent) {
      * @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;
     }
 
     /**
      * Returns {@code true} if a {@link AttributeKey} exists for the given {@code name}.
      */
     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 @@ private String checkNotNullAndNotEmpty(String name) {
 
     @Deprecated
     public final int nextId() {
-        synchronized (constants) {
-            int id = nextId;
-            nextId++;
-            return id;
-        }
+        return nextId.getAndIncrement();
     }
 }