Repository: netty/netty
Issue: Improve performance of FastThreadLocal slow path
Labels: improvement

Issues #4402 and #4417 gave some indication that there might be room for performance improvement on the slow path of `FastThreadLocal`.

The performance on the slow path might be critical if a user of Netty plugs in an `Executor` that does not use `FastThreadLocalThreads`.


Issue comments:
tip for you: current slow-path calls 2x Thread.currentThread(). have fun

@normanmaurer - Any updates on this issue? Didn't have have a PR related to this?

Nope lost track on this one. Should pock it up again

> Am 04.02.2016 um 12:57 schrieb Scott Mitchell notifications@github.com:
> 
> @normanmaurer - Any updates on this issue? Didn't have have a PR related to this?
> 
> —
> Reply to this email directly or view it on GitHub.

Fixed by https://github.com/netty/netty/pull/5012


Pull request review comments:
Modified to put the same data to jdkThreadLocals and fastThreadLocals.

`final` is the key of the improvement.

@windie thanks for highlight this!

+1

@trustin can you comment why you not did this before when implement this ? 

@normanmaurer It's to prevent `slowThreadLocalMap` from being initialized again after its destruction. Otherwise a user can 'resurrect' the destroyed `InternalThreadLocalMap`.

This breaks the behavior of `getIfSet()` which is expected to return `null` when no thread local was set. 

@windie interesting. I actually checked the assembly and it seems legit. When adding the final keyword, the `slowThreadLocalMap` is "inlined" into the jitted code and you avoid one load + a typecheck.

However, I wouldn't expect this to make a difference in real code. Still nice, as it seems to be one of the few examples where final affects code generation.

@windie I think it makes sense to move instance creation in `slowGet()` into the `ThreadLocal` then.

``` java
    static final ThreadLocal<InternalThreadLocalMap> slowThreadLocalMap = new ThreadLocal<InternalThreadLocalMap>() {
        @Override
        public InternalThreadLocalMap initialValue() {
            return new InternalThreadLocalMap();
        }
    }
```

@buchgr I didn't do that because it breaks `getIfSet`. @trustin FYI, when no thread local is set, slowThreadLocalMap.get will return null as its initialValue is not set.

But the user can still call `get` to create a new `slowThreadLocalMap` after its destruction.

Ah, you are right. My memory about this is fading :-)

I guess it's probably to make `getIfSet()` return `null` when `slowThreadLocalMap` is not created yet (or destroyed)

See [here](https://github.com/netty/netty/pull/5012/files/5dcc0c9d05b9c1a421619f0f4a5fb19e85f06ddc#r56955519)

slowThreadLocalMap.get() will return null if no thread local is set.

Indeed, because we did not override `initialValue()`. LGTM

So all good?

> Am 23.03.2016 um 03:14 schrieb Trustin Lee notifications@github.com:
> 
> In common/src/main/java/io/netty/util/internal/InternalThreadLocalMap.java:
> 
> > ```
> >      }
> > ```
> > -        return threadLocalMap;
> > -        return slowThreadLocalMap.get();
> >   Indeed, because we did not override initialValue(). LGTM
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub

Yeah, all good.

Ok let merge this FTW

> Am 23.03.2016 um 06:39 schrieb Trustin Lee notifications@github.com:
> 
> In common/src/main/java/io/netty/util/internal/InternalThreadLocalMap.java:
> 
> > ```
> >      }
> > ```
> > -        return threadLocalMap;
> > -        return slowThreadLocalMap.get();
> >   Yeah, all good.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub

@windie might be worth adding a comment about this to the field declaration, WDYT?


Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/internal/InternalThreadLocalMap.java b/common/src/main/java/io/netty/util/internal/InternalThreadLocalMap.java
index dbd4b3d6bdf..25bf5375563 100644
--- a/common/src/main/java/io/netty/util/internal/InternalThreadLocalMap.java
+++ b/common/src/main/java/io/netty/util/internal/InternalThreadLocalMap.java
@@ -41,18 +41,10 @@ public final class InternalThreadLocalMap extends UnpaddedInternalThreadLocalMap
 
     public static InternalThreadLocalMap getIfSet() {
         Thread thread = Thread.currentThread();
-        InternalThreadLocalMap threadLocalMap;
         if (thread instanceof FastThreadLocalThread) {
-            threadLocalMap = ((FastThreadLocalThread) thread).threadLocalMap();
-        } else {
-            ThreadLocal<InternalThreadLocalMap> slowThreadLocalMap = UnpaddedInternalThreadLocalMap.slowThreadLocalMap;
-            if (slowThreadLocalMap == null) {
-                threadLocalMap = null;
-            } else {
-                threadLocalMap = slowThreadLocalMap.get();
-            }
+            return ((FastThreadLocalThread) thread).threadLocalMap();
         }
-        return threadLocalMap;
+        return slowThreadLocalMap.get();
     }
 
     public static InternalThreadLocalMap get() {
@@ -74,11 +66,6 @@ private static InternalThreadLocalMap fastGet(FastThreadLocalThread thread) {
 
     private static InternalThreadLocalMap slowGet() {
         ThreadLocal<InternalThreadLocalMap> slowThreadLocalMap = UnpaddedInternalThreadLocalMap.slowThreadLocalMap;
-        if (slowThreadLocalMap == null) {
-            UnpaddedInternalThreadLocalMap.slowThreadLocalMap =
-                    slowThreadLocalMap = new ThreadLocal<InternalThreadLocalMap>();
-        }
-
         InternalThreadLocalMap ret = slowThreadLocalMap.get();
         if (ret == null) {
             ret = new InternalThreadLocalMap();
@@ -92,15 +79,12 @@ public static void remove() {
         if (thread instanceof FastThreadLocalThread) {
             ((FastThreadLocalThread) thread).setThreadLocalMap(null);
         } else {
-            ThreadLocal<InternalThreadLocalMap> slowThreadLocalMap = UnpaddedInternalThreadLocalMap.slowThreadLocalMap;
-            if (slowThreadLocalMap != null) {
-                slowThreadLocalMap.remove();
-            }
+            slowThreadLocalMap.remove();
         }
     }
 
     public static void destroy() {
-        slowThreadLocalMap = null;
+        slowThreadLocalMap.remove();
     }
 
     public static int nextVariableIndex() {
diff --git a/common/src/main/java/io/netty/util/internal/UnpaddedInternalThreadLocalMap.java b/common/src/main/java/io/netty/util/internal/UnpaddedInternalThreadLocalMap.java
index c75ac40ab53..2d0bb6cd156 100644
--- a/common/src/main/java/io/netty/util/internal/UnpaddedInternalThreadLocalMap.java
+++ b/common/src/main/java/io/netty/util/internal/UnpaddedInternalThreadLocalMap.java
@@ -32,7 +32,7 @@
  */
 class UnpaddedInternalThreadLocalMap {
 
-    static ThreadLocal<InternalThreadLocalMap> slowThreadLocalMap;
+    static final ThreadLocal<InternalThreadLocalMap> slowThreadLocalMap = new ThreadLocal<InternalThreadLocalMap>();
     static final AtomicInteger nextIndex = new AtomicInteger();
 
     /** Used by {@link FastThreadLocal} */
diff --git a/microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalBenchmark.java b/microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalFastPathBenchmark.java
similarity index 87%
rename from microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalBenchmark.java
rename to microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalFastPathBenchmark.java
index 2993a08be5f..c0073fb655c 100644
--- a/microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalBenchmark.java
+++ b/microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalFastPathBenchmark.java
@@ -24,11 +24,11 @@
 import java.util.Random;
 
 /**
- * This class benchmarks different allocators with different allocation sizes.
+ * This class benchmarks the fast path of FastThreadLocal and the JDK ThreadLocal.
  */
 @Threads(4)
 @Measurement(iterations = 10, batchSize = 100)
-public class FastThreadLocalBenchmark extends AbstractMicrobenchmark {
+public class FastThreadLocalFastPathBenchmark extends AbstractMicrobenchmark {
 
     private static final Random rand = new Random();
 
@@ -39,19 +39,17 @@ public class FastThreadLocalBenchmark extends AbstractMicrobenchmark {
 
     static {
         for (int i = 0; i < jdkThreadLocals.length; i ++) {
+            final int num = rand.nextInt();
             jdkThreadLocals[i] = new ThreadLocal<Integer>() {
                 @Override
                 protected Integer initialValue() {
-                    return rand.nextInt();
+                    return num;
                 }
             };
-        }
-
-        for (int i = 0; i < fastThreadLocals.length; i ++) {
             fastThreadLocals[i] = new FastThreadLocal<Integer>() {
                 @Override
                 protected Integer initialValue() {
-                    return rand.nextInt();
+                    return num;
                 }
             };
         }
diff --git a/microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalSlowPathBenchmark.java b/microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalSlowPathBenchmark.java
new file mode 100644
index 00000000000..20d414aa1f8
--- /dev/null
+++ b/microbench/src/main/java/io/netty/microbench/concurrent/FastThreadLocalSlowPathBenchmark.java
@@ -0,0 +1,79 @@
+/*
+ * Copyright 2016 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.microbench.concurrent;
+
+import io.netty.microbench.util.AbstractMicrobenchmark;
+import io.netty.util.concurrent.FastThreadLocal;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Threads;
+
+import java.util.Random;
+
+/**
+ * This class benchmarks the slow path of FastThreadLocal and the JDK ThreadLocal.
+ */
+@Threads(4)
+@Measurement(iterations = 10, batchSize = 100)
+public class FastThreadLocalSlowPathBenchmark extends AbstractMicrobenchmark {
+
+    private static final Random rand = new Random();
+
+    @SuppressWarnings("unchecked")
+    private static final ThreadLocal<Integer>[] jdkThreadLocals = new ThreadLocal[128];
+    @SuppressWarnings("unchecked")
+    private static final FastThreadLocal<Integer>[] fastThreadLocals = new FastThreadLocal[jdkThreadLocals.length];
+
+    static {
+        for (int i = 0; i < jdkThreadLocals.length; i ++) {
+            final int num = rand.nextInt();
+            jdkThreadLocals[i] = new ThreadLocal<Integer>() {
+                @Override
+                protected Integer initialValue() {
+                    return num;
+                }
+            };
+            fastThreadLocals[i] = new FastThreadLocal<Integer>() {
+                @Override
+                protected Integer initialValue() {
+                    return num;
+                }
+            };
+        }
+    }
+
+    public FastThreadLocalSlowPathBenchmark() {
+        super(false, true);
+    }
+
+    @Benchmark
+    public int jdkThreadLocalGet() {
+        int result = 0;
+        for (ThreadLocal<Integer> i: jdkThreadLocals) {
+            result += i.get();
+        }
+        return result;
+    }
+
+    @Benchmark
+    public int fastThreadLocal() {
+        int result = 0;
+        for (FastThreadLocal<Integer> i: fastThreadLocals) {
+            result += i.get();
+        }
+        return result;
+    }
+}
diff --git a/microbench/src/main/java/io/netty/microbench/util/AbstractMicrobenchmark.java b/microbench/src/main/java/io/netty/microbench/util/AbstractMicrobenchmark.java
index eea5b16e6f3..7c9dcf2d1ed 100644
--- a/microbench/src/main/java/io/netty/microbench/util/AbstractMicrobenchmark.java
+++ b/microbench/src/main/java/io/netty/microbench/util/AbstractMicrobenchmark.java
@@ -32,17 +32,6 @@
 public class AbstractMicrobenchmark extends AbstractMicrobenchmarkBase {
 
     protected static final int DEFAULT_FORKS = 2;
-    protected static final String[] JVM_ARGS;
-
-    static {
-        final String[] customArgs = {
-        "-Xms768m", "-Xmx768m", "-XX:MaxDirectMemorySize=768m", "-Djmh.executor=CUSTOM",
-        "-Djmh.executor.class=io.netty.microbench.util.AbstractMicrobenchmark$HarnessExecutor" };
-
-        JVM_ARGS = new String[BASE_JVM_ARGS.length + customArgs.length];
-        System.arraycopy(BASE_JVM_ARGS, 0, JVM_ARGS, 0, BASE_JVM_ARGS.length);
-        System.arraycopy(customArgs, 0, JVM_ARGS, BASE_JVM_ARGS.length, customArgs.length);
-    }
 
     public static final class HarnessExecutor extends ThreadPoolExecutor {
         public HarnessExecutor(int maxThreads, String prefix) {
@@ -52,27 +41,36 @@ public HarnessExecutor(int maxThreads, String prefix) {
         }
     }
 
-    private final boolean disableAssertions;
-    private String[] jvmArgsWithNoAssertions;
+    private final String[] jvmArgs;
 
     public AbstractMicrobenchmark() {
-        this(false);
+        this(false, false);
     }
 
     public AbstractMicrobenchmark(boolean disableAssertions) {
-        this.disableAssertions = disableAssertions;
+        this(disableAssertions, false);
     }
 
-    @Override
-    protected String[] jvmArgs() {
-        if (!disableAssertions) {
-            return JVM_ARGS;
+    public AbstractMicrobenchmark(boolean disableAssertions, boolean disableHarnessExecutor) {
+        final String[] customArgs;
+        if (disableHarnessExecutor) {
+            customArgs = new String[]{"-Xms768m", "-Xmx768m", "-XX:MaxDirectMemorySize=768m"};
+        } else {
+            customArgs = new String[]{"-Xms768m", "-Xmx768m", "-XX:MaxDirectMemorySize=768m", "-Djmh.executor=CUSTOM",
+                    "-Djmh.executor.class=io.netty.microbench.util.AbstractMicrobenchmark$HarnessExecutor"};
         }
-
-        if (jvmArgsWithNoAssertions == null) {
-            jvmArgsWithNoAssertions = removeAssertions(JVM_ARGS);
+        String[] jvmArgs = new String[BASE_JVM_ARGS.length + customArgs.length];
+        System.arraycopy(BASE_JVM_ARGS, 0, jvmArgs, 0, BASE_JVM_ARGS.length);
+        System.arraycopy(customArgs, 0, jvmArgs, BASE_JVM_ARGS.length, customArgs.length);
+        if (disableAssertions) {
+            jvmArgs = removeAssertions(jvmArgs);
         }
-        return jvmArgsWithNoAssertions;
+        this.jvmArgs = jvmArgs;
+    }
+
+    @Override
+    protected String[] jvmArgs() {
+        return jvmArgs;
     }
 
     @Override


Test patch: