From 20d9e0cfab5a8dde64e9e7f96a344f4c77a6d410 Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Mon, 19 Feb 2018 20:26:22 -0800 Subject: [PATCH] Prefer reflection over method handles due to memory leak (fixes #222, see JDK-8174749) --- .../cache/LocalCacheFactoryGenerator.java | 12 ------- .../cache/LocalCacheSelectorCode.java | 35 +++++++++++-------- .../caffeine/cache/NodeFactoryGenerator.java | 10 ------ .../caffeine/cache/NodeSelectorCode.java | 22 ++++++------ .../caffeine/cache/Specifications.java | 12 +++---- .../benmanes/caffeine/FactoryBenchmark.java | 2 +- 6 files changed, 35 insertions(+), 58 deletions(-) diff --git a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheFactoryGenerator.java b/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheFactoryGenerator.java index f5c7215315..e91aa5c73f 100644 --- a/caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/LocalCacheFactoryGenerator.java +++ b/caffeine/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 index 3143235cb9..48fef1efb4 100644 --- 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 index b88e8b5d96..69b3ddf535 100644 --- 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 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 index 01310ea3ff..2d66d384d0 100644 --- 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 index 6dbf9d4235..4055fcc6b8 100644 --- 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 index ba9eff99ce..4ca392c011 100644 --- 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++); }