Prefer reflection over method handles due to memory leak (fixes #222,…

… see JDK-8174749)
ben-manes · Feb 20, 2018 · 20d9e0c · 20d9e0c · jschlather · Feb 20, 2018
1 parent f8cc328
commit 20d9e0c
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 58 deletions.
diff --git a/...eine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheFactoryGenerator.java b/...eine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheFactoryGenerator.java
@@ -16,18 +16,14 @@
 package com.github.benmanes.caffeine.cache;
 
 import static com.github.benmanes.caffeine.cache.Specifications.BOUNDED_LOCAL_CACHE;
-import static com.github.benmanes.caffeine.cache.Specifications.BUILDER;
 import static com.github.benmanes.caffeine.cache.Specifications.BUILDER_PARAM;
-import static com.github.benmanes.caffeine.cache.Specifications.CACHE_LOADER;
 import static com.github.benmanes.caffeine.cache.Specifications.CACHE_LOADER_PARAM;
-import static com.github.benmanes.caffeine.cache.Specifications.LOOKUP;
 import static com.github.benmanes.caffeine.cache.Specifications.kTypeVar;
 import static com.github.benmanes.caffeine.cache.Specifications.vTypeVar;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Objects.requireNonNull;
 
 import java.io.IOException;
-import java.lang.invoke.MethodType;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.time.Year;
@@ -78,12 +74,6 @@
  * @author ben.manes@gmail.com (Ben Manes)
  */
 public final class LocalCacheFactoryGenerator {
-  static final FieldSpec FACTORY = FieldSpec.builder(MethodType.class, "FACTORY")
-      .initializer("$T.methodType($T.class, $T.class, $T.class, $T.class)",
-          MethodType.class, void.class, BUILDER, CACHE_LOADER.rawType, TypeName.BOOLEAN)
-      .addModifiers(Modifier.PUBLIC, Modifier.STATIC, Modifier.FINAL)
-      .build();
-
   final Feature[] featureByIndex = new Feature[] {null, null, Feature.LISTENING,
       Feature.STATS, Feature.MAXIMUM_SIZE, Feature.MAXIMUM_WEIGHT, Feature.EXPIRE_ACCESS,
       Feature.EXPIRE_WRITE, Feature.REFRESH_WRITE};
@@ -165,8 +155,6 @@ private void addConstants() {
               .initializer("$S", constant)
               .build());
     }
-    factory.addField(LOOKUP);
-    factory.addField(FACTORY);
   }
 
   private void generateLocalCaches() {

diff --git a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheSelectorCode.java b/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheSelectorCode.java
@@ -15,15 +15,16 @@
  */
 package com.github.benmanes.caffeine.cache;
 
-import com.squareup.javapoet.ClassName;
-import com.squareup.javapoet.CodeBlock;
-
-import java.lang.invoke.MethodHandle;
-
-import static com.github.benmanes.caffeine.cache.LocalCacheFactoryGenerator.FACTORY;
 import static com.github.benmanes.caffeine.cache.Specifications.BOUNDED_LOCAL_CACHE;
+import static com.github.benmanes.caffeine.cache.Specifications.BUILDER;
+import static com.github.benmanes.caffeine.cache.Specifications.CACHE_LOADER;
 import static com.github.benmanes.caffeine.cache.Specifications.LOCAL_CACHE_FACTORY;
-import static com.github.benmanes.caffeine.cache.Specifications.LOOKUP;
+
+import java.lang.reflect.Constructor;
+
+import com.squareup.javapoet.ClassName;
+import com.squareup.javapoet.CodeBlock;
+import com.squareup.javapoet.TypeName;
 
 /**
  * @author ben.manes@gmail.com (Ben Manes)
@@ -34,7 +35,8 @@ public final class LocalCacheSelectorCode {
 
   private LocalCacheSelectorCode() {
     block = CodeBlock.builder()
-        .addStatement("$1T sb = new $1T(\"$2N.\")", StringBuilder.class, ((ClassName)LOCAL_CACHE_FACTORY).packageName());
+        .addStatement("$1T sb = new $1T(\"$2N.\")", StringBuilder.class,
+            ((ClassName)LOCAL_CACHE_FACTORY).packageName());
   }
 
   private LocalCacheSelectorCode keys() {
@@ -95,16 +97,19 @@ private LocalCacheSelectorCode expires() {
   }
 
   private LocalCacheSelectorCode selector() {
-    block.beginControlFlow("try")
+    block
+        .beginControlFlow("try")
             .addStatement("$T<?> clazz = $T.class.getClassLoader().loadClass(sb.toString())",
                 Class.class, LOCAL_CACHE_FACTORY)
-            .addStatement("$T handle = $N.findConstructor(clazz, $N)",
-                MethodHandle.class, LOOKUP, FACTORY)
-            .addStatement("return ($T) handle.invoke(builder, cacheLoader, async)",
+            .addStatement("$T<?> ctor = clazz.getDeclaredConstructor($T.class, $T.class, $T.class)",
+                Constructor.class, BUILDER, CACHE_LOADER.rawType, TypeName.BOOLEAN)
+            .add("@SuppressWarnings($S)\n", "unchecked")
+            .addStatement("$1T factory = ($1T) ctor.newInstance(builder, cacheLoader, async)",
                 BOUNDED_LOCAL_CACHE)
-          .nextControlFlow("catch ($T t)", Throwable.class)
-            .addStatement("throw new $T(sb.toString(), t)", IllegalStateException.class)
-          .endControlFlow();
+            .addStatement("return factory")
+        .nextControlFlow("catch ($T e)", ReflectiveOperationException.class)
+            .addStatement("throw new $T(sb.toString(), e)", IllegalStateException.class)
+      .endControlFlow();
     return this;
   }
 

diff --git a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/NodeFactoryGenerator.java b/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/NodeFactoryGenerator.java
@@ -18,7 +18,6 @@
 import static com.github.benmanes.caffeine.cache.Specifications.BUILDER_PARAM;
 import static com.github.benmanes.caffeine.cache.Specifications.DEAD_STRONG_KEY;
 import static com.github.benmanes.caffeine.cache.Specifications.DEAD_WEAK_KEY;
-import static com.github.benmanes.caffeine.cache.Specifications.LOOKUP;
 import static com.github.benmanes.caffeine.cache.Specifications.NODE;
 import static com.github.benmanes.caffeine.cache.Specifications.NODE_FACTORY;
 import static com.github.benmanes.caffeine.cache.Specifications.PACKAGE_NAME;
@@ -39,7 +38,6 @@
 import static java.util.Objects.requireNonNull;
 
 import java.io.IOException;
-import java.lang.invoke.MethodType;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.time.Year;
@@ -97,11 +95,6 @@
  */
 @SuppressWarnings("PMD.AvoidDuplicateLiterals")
 public final class NodeFactoryGenerator {
-  static final FieldSpec FACTORY = FieldSpec.builder(MethodType.class, "FACTORY")
-      .initializer("$T.methodType($T.class)", MethodType.class, void.class)
-      .addModifiers(Modifier.PUBLIC, Modifier.STATIC, Modifier.FINAL)
-      .build();
-
   final List<NodeRule> rules = ImmutableList.of(new AddSubtype(), new AddConstructors(),
       new AddKey(), new AddValue(), new AddMaximum(), new AddExpiration(), new AddDeques(),
       new AddFactoryMethods(),  new AddHealth(), new Finalize());
@@ -168,9 +161,6 @@ private void addConstants() {
     nodeFactory.addField(FieldSpec.builder(rawReferenceKeyType, DEAD_WEAK_KEY, modifiers)
         .initializer("new $T(null, null)", rawReferenceKeyType)
         .build());
-
-    nodeFactory.addField(LOOKUP);
-    nodeFactory.addField(FACTORY);
   }
 
   private void addKeyMethods() {

diff --git a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/NodeSelectorCode.java b/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/NodeSelectorCode.java
@@ -15,14 +15,10 @@
  */
 package com.github.benmanes.caffeine.cache;
 
-import com.squareup.javapoet.CodeBlock;
-
-import java.lang.invoke.MethodHandle;
-
-import static com.github.benmanes.caffeine.cache.LocalCacheFactoryGenerator.FACTORY;
-import static com.github.benmanes.caffeine.cache.Specifications.LOOKUP;
 import static com.github.benmanes.caffeine.cache.Specifications.NODE_FACTORY;
 
+import com.squareup.javapoet.CodeBlock;
+
 /**
  * @author ben.manes@gmail.com (Ben Manes)
  */
@@ -32,7 +28,8 @@ public final class NodeSelectorCode {
 
   private NodeSelectorCode() {
     block = CodeBlock.builder()
-        .addStatement("$1T sb = new $1T(\"$2N.\")", StringBuilder.class, NODE_FACTORY.rawType.packageName());
+        .addStatement("$1T sb = new $1T(\"$2N.\")",
+            StringBuilder.class, NODE_FACTORY.rawType.packageName());
   }
 
   private NodeSelectorCode keys() {
@@ -98,11 +95,12 @@ private NodeSelectorCode selector() {
         .beginControlFlow("try")
             .addStatement("$T<?> clazz = $T.class.getClassLoader().loadClass(sb.toString())",
                 Class.class, NODE_FACTORY.rawType)
-            .addStatement("$T handle = $N.findConstructor(clazz, $N)",
-                MethodHandle.class, LOOKUP, FACTORY)
-            .addStatement("return ($T) handle.invoke()", NODE_FACTORY)
-        .nextControlFlow("catch ($T t)", Throwable.class)
-            .addStatement("throw new $T(sb.toString(), t)", IllegalStateException.class)
+            .add("@SuppressWarnings($S)\n", "unchecked")
+            .addStatement("$1T factory = ($1T) clazz.getDeclaredConstructor().newInstance()",
+                NODE_FACTORY)
+            .addStatement("return factory")
+        .nextControlFlow("catch ($T e)", ReflectiveOperationException.class)
+            .addStatement("throw new $T(sb.toString(), e)", IllegalStateException.class)
         .endControlFlow();
     return this;
   }

diff --git a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/Specifications.java b/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/Specifications.java
@@ -15,6 +15,10 @@
  */
 package com.github.benmanes.caffeine.cache;
 
+import java.lang.ref.ReferenceQueue;
+
+import javax.lang.model.element.Modifier;
+
 import com.google.common.base.CaseFormat;
 import com.squareup.javapoet.ClassName;
 import com.squareup.javapoet.FieldSpec;
@@ -23,10 +27,6 @@
 import com.squareup.javapoet.TypeName;
 import com.squareup.javapoet.TypeVariableName;
 
-import javax.lang.model.element.Modifier;
-import java.lang.invoke.MethodHandles;
-import java.lang.ref.ReferenceQueue;
-
 /**
  * Shared constants for a code generation specification.
  *
@@ -65,10 +65,6 @@ public final class Specifications {
 
   public static final TypeName UNSAFE_ACCESS =
       ClassName.get("com.github.benmanes.caffeine.base", "UnsafeAccess");
-  public static final FieldSpec LOOKUP = FieldSpec.builder(MethodHandles.Lookup.class, "LOOKUP")
-      .initializer("$T.lookup()", MethodHandles.class)
-      .addModifiers(Modifier.PUBLIC, Modifier.STATIC, Modifier.FINAL)
-      .build();
 
   public static final TypeName LOCAL_CACHE_FACTORY =
       ClassName.get(PACKAGE_NAME, "LocalCacheFactory");

diff --git a/caffeine/src/jmh/java/com/github/benmanes/caffeine/FactoryBenchmark.java b/caffeine/src/jmh/java/com/github/benmanes/caffeine/FactoryBenchmark.java
@@ -53,7 +53,7 @@ public Alpha methodHandle_invokeExact(ThreadState state) {
   }
 
   @Benchmark
-  public Alpha reflectionFactory(ThreadState state) {
+  public Alpha reflection(ThreadState state) {
     return reflectionFactory.newInstance(state.i++);
   }