From 847484cd476216365e2370c1c9ea960314ee4ffc Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 6 Aug 2018 13:50:48 +1000 Subject: [PATCH 1/7] Use WeakConcurrentMap Instead of `Collections.synchronizedMap(new WeakHashMap<>())`. --- .../agent-bootstrap/agent-bootstrap.gradle | 3 + .../datadog/trace/bootstrap/JDBCMaps.java | 13 ++-- .../trace/bootstrap/WeakMapManager.java | 73 +++++++++++++++++++ .../trace/bootstrap/WeakMapManagerTest.groovy | 41 +++++++++++ .../agent/tooling/ClassLoaderMatcher.java | 17 ++--- .../tooling/muzzle/ReferenceMatcher.java | 9 +-- .../HttpUrlConnectionInstrumentation.java | 7 +- .../datadog/trace/agent/test/TestUtils.java | 10 +++ .../java/datadog/opentracing/DDTracer.java | 4 +- .../datadog/opentracing/PendingTrace.java | 1 + 10 files changed, 152 insertions(+), 26 deletions(-) create mode 100644 dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java create mode 100644 dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/WeakMapManagerTest.groovy diff --git a/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle b/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle index facd73c5fa..6a96636434 100644 --- a/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle +++ b/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle @@ -9,10 +9,13 @@ excludedClassesConverage += ['datadog.trace.bootstrap.*'] dependencies { compile project(':dd-trace-api') + compile group: 'com.blogspot.mydailyjava', name: 'weak-lock-free', version: '0.13' compile deps.opentracing compile deps.slf4j compile group: 'org.slf4j', name: 'slf4j-simple', version: versions.slf4j // ^ Generally a bad idea for libraries, but we're shadowing. + + testCompile project(':dd-java-agent:testing') } jar { diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java index 0e3a17234f..4fdc53874d 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java @@ -1,10 +1,10 @@ package datadog.trace.bootstrap; +import static datadog.trace.bootstrap.WeakMapManager.newWeakMap; + +import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import java.sql.Connection; import java.sql.PreparedStatement; -import java.util.Collections; -import java.util.Map; -import java.util.WeakHashMap; import lombok.Data; /** @@ -13,10 +13,9 @@ import lombok.Data; *

In the bootstrap project to ensure visibility by all classes. */ public class JDBCMaps { - public static final Map connectionInfo = - Collections.synchronizedMap(new WeakHashMap()); - public static final Map preparedStatements = - Collections.synchronizedMap(new WeakHashMap()); + public static final WeakConcurrentMap connectionInfo = newWeakMap(); + public static final WeakConcurrentMap preparedStatements = + newWeakMap(); public static final String DB_QUERY = "DB Query"; diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java new file mode 100644 index 0000000000..6dd8a7465d --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java @@ -0,0 +1,73 @@ +package datadog.trace.bootstrap; + +import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; + +public class WeakMapManager { + private static final long CLEAN_FREQUENCY_SECONDS = 1; + private static final List maps = new CopyOnWriteArrayList<>(); + + private static final ThreadFactory weakMapCleanerThreadFactory = + new ThreadFactory() { + @Override + public Thread newThread(final Runnable r) { + final Thread thread = new Thread(r, "dd-weak-ref-cleaner"); + thread.setDaemon(true); + thread.setPriority(Thread.MIN_PRIORITY); + return thread; + } + }; + + private static final ScheduledExecutorService cleaner = + Executors.newScheduledThreadPool(1, weakMapCleanerThreadFactory); + + private static final Runnable runnable = new Cleaner(); + + static { + cleaner.scheduleAtFixedRate( + runnable, CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); + + try { + Runtime.getRuntime() + .addShutdownHook( + new Thread() { + @Override + public void run() { + try { + cleaner.shutdownNow(); + cleaner.awaitTermination(5, TimeUnit.SECONDS); + } catch (final InterruptedException e) { + // Don't bother waiting then... + } + } + }); + } catch (final IllegalStateException ex) { + // The JVM is already shutting down. + } + } + + public static WeakConcurrentMap newWeakMap() { + final WeakConcurrentMap map = new WeakConcurrentMap<>(false); + maps.add(map); + return map; + } + + public static void cleanMaps() { + for (final WeakConcurrentMap map : maps) { + map.expungeStaleEntries(); + } + } + + private static class Cleaner implements Runnable { + + @Override + public void run() { + cleanMaps(); + } + } +} diff --git a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/WeakMapManagerTest.groovy b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/WeakMapManagerTest.groovy new file mode 100644 index 0000000000..5428aae8a2 --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/WeakMapManagerTest.groovy @@ -0,0 +1,41 @@ +package datadog.trace.bootstrap + +import datadog.trace.agent.test.TestUtils +import spock.lang.Specification + +class WeakMapManagerTest extends Specification { + + def setup() { + WeakMapManager.maps.clear() + } + + def "calling new adds to the list"() { + when: + def map1 = WeakMapManager.newWeakMap() + + then: + WeakMapManager.maps == [map1] + + when: + def map2 = WeakMapManager.newWeakMap() + + then: + WeakMapManager.maps == [map1, map2] + } + + def "calling cleanMaps does cleanup"() { + setup: + def map = WeakMapManager.newWeakMap() + map.put(new Object(), "value") + TestUtils.awaitGC() + + expect: + map.approximateSize() == 1 + + when: + WeakMapManager.cleanMaps() + + then: + map.approximateSize() == 0 + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java index 1dc632faa3..a8cba595f9 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java @@ -1,13 +1,14 @@ package datadog.trace.agent.tooling; +import static datadog.trace.bootstrap.WeakMapManager.newWeakMap; + +import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import datadog.trace.bootstrap.DatadogClassLoader; import datadog.trace.bootstrap.PatchLogger; import io.opentracing.util.GlobalTracer; import java.util.Collections; import java.util.HashSet; -import java.util.Map; import java.util.Set; -import java.util.WeakHashMap; import lombok.extern.slf4j.Slf4j; import net.bytebuddy.matcher.ElementMatcher; @@ -43,8 +44,7 @@ public class ClassLoaderMatcher { extends ElementMatcher.Junction.AbstractBase { public static final SkipClassLoaderMatcher INSTANCE = new SkipClassLoaderMatcher(); /* Cache of classloader-instance -> (true|false). True = skip instrumentation. False = safe to instrument. */ - private static final Map SKIP_CACHE = - Collections.synchronizedMap(new WeakHashMap()); + private static final WeakConcurrentMap SKIP_CACHE = newWeakMap(); private static final Set CLASSLOADER_CLASSES_TO_SKIP; static { @@ -131,8 +131,7 @@ public class ClassLoaderMatcher { public static class ClassLoaderHasClassMatcher extends ElementMatcher.Junction.AbstractBase { - private final Map cache = - Collections.synchronizedMap(new WeakHashMap()); + private final WeakConcurrentMap cache = newWeakMap(); private final String[] names; @@ -164,8 +163,7 @@ public class ClassLoaderMatcher { public static class ClassLoaderHasClassWithFieldMatcher extends ElementMatcher.Junction.AbstractBase { - private final Map cache = - Collections.synchronizedMap(new WeakHashMap()); + private final WeakConcurrentMap cache = newWeakMap(); private final String className; private final String fieldName; @@ -203,8 +201,7 @@ public class ClassLoaderMatcher { public static class ClassLoaderHasClassWithMethodMatcher extends ElementMatcher.Junction.AbstractBase { - private final Map cache = - Collections.synchronizedMap(new WeakHashMap()); + private final WeakConcurrentMap cache = newWeakMap(); private final String className; private final String methodName; diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index 3a6a25bf5f..d1baf9db0c 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -1,23 +1,22 @@ package datadog.trace.agent.tooling.muzzle; +import static datadog.trace.bootstrap.WeakMapManager.newWeakMap; import static net.bytebuddy.dynamic.loading.ClassLoadingStrategy.BOOTSTRAP_LOADER; +import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import datadog.trace.agent.tooling.Utils; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; -import java.util.WeakHashMap; import lombok.extern.slf4j.Slf4j; /** Matches a set of references against a classloader. */ @Slf4j public class ReferenceMatcher { - private final Map> mismatchCache = - Collections.synchronizedMap(new WeakHashMap>()); + private final WeakConcurrentMap> mismatchCache = + newWeakMap(); private final Reference[] references; private final Set helperClassNames; diff --git a/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java index c5e82c4d88..7a07081633 100644 --- a/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java +++ b/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java @@ -1,6 +1,7 @@ package datadog.trace.instrumentation.http_url_connection; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.bootstrap.WeakMapManager.newWeakMap; import static io.opentracing.log.Fields.ERROR_OBJECT; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; @@ -8,6 +9,7 @@ import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.not; +import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.api.DDSpanTypes; @@ -24,7 +26,6 @@ import java.net.URL; import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.WeakHashMap; import javax.net.ssl.HttpsURLConnection; import net.bytebuddy.asm.Advice; import net.bytebuddy.matcher.ElementMatcher; @@ -174,8 +175,8 @@ public class HttpUrlConnectionInstrumentation extends Instrumenter.Default { } public static class HttpURLState { - private static final Map STATE_MAP = - Collections.synchronizedMap(new WeakHashMap()); + private static final WeakConcurrentMap STATE_MAP = + newWeakMap(); public static HttpURLState get(final HttpURLConnection connection) { HttpURLState state = STATE_MAP.get(connection); diff --git a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/TestUtils.java b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/TestUtils.java index 8db4c4a094..c4d4cd9356 100644 --- a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/TestUtils.java +++ b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/TestUtils.java @@ -13,6 +13,7 @@ import java.io.File; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; +import java.lang.ref.WeakReference; import java.lang.reflect.Field; import java.net.ServerSocket; import java.net.URL; @@ -204,4 +205,13 @@ public class TestUtils { return -1; } } + + public static void awaitGC() { + Object obj = new Object(); + final WeakReference ref = new WeakReference<>(obj); + obj = null; + while (ref.get() != null) { + System.gc(); + } + } } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java index badd45c715..e3a43fc2a8 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -29,6 +29,7 @@ import io.opentracing.ScopeManager; import io.opentracing.Span; import io.opentracing.SpanContext; import io.opentracing.propagation.Format; +import java.io.Closeable; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -48,7 +49,7 @@ import lombok.extern.slf4j.Slf4j; /** DDTracer makes it easy to send traces and span to DD using the OpenTracing API. */ @Slf4j -public class DDTracer implements io.opentracing.Tracer { +public class DDTracer implements io.opentracing.Tracer, Closeable { public static final String UNASSIGNED_DEFAULT_SERVICE_NAME = "unnamed-java-app"; @@ -302,6 +303,7 @@ public class DDTracer implements io.opentracing.Tracer { } } + @Override public void close() { PendingTrace.close(); writer.close(); diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java b/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java index 11e7e13557..ff4bf8371d 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java @@ -239,6 +239,7 @@ public class PendingTrace extends ConcurrentLinkedDeque { public Thread newThread(final Runnable r) { final Thread thread = new Thread(r, "dd-span-cleaner"); thread.setDaemon(true); + thread.setPriority(Thread.MIN_PRIORITY); return thread; } }; From 9b00a27dce18b7e6c002b04f68085aeabc1942e0 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 7 Aug 2018 12:42:14 +1000 Subject: [PATCH 2/7] add javadoc and comparison --- .../trace/bootstrap/WeakMapManager.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java index 6dd8a7465d..2c752bb52d 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java @@ -8,6 +8,26 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; +/** + * Provides instances of {@link WeakConcurrentMap} and retains reference to them to allow a single + * thread to clean void weak references out for all instances. Cleaning is done every second. + */ + +// Comparison with using WeakConcurrentMap vs Guava's implementation: +// Cleaning: +// * `WeakConcurrentMap`: centralized but we have to maintain out own code and thread for it +// * `Guava`: inline on application's thread, with constant max delay +// Jar Size: +// * `WeakConcurrentMap`: small +// * `Guava`: large, but we may use other features, like immutable collections - and we already ship +// Guava as part of distribution now, so using Guava for this doesn’t increase size. +// Must go on bootstrap classpath: +// * `WeakConcurrentMap`: version conflict is unlikely, so we can directly inject for now +// * `Guava`: need to implement shadow copy (might eventually be necessary for other dependencies) +// Used by other javaagents for similar purposes: +// * `WeakConcurrentMap`: anecdotally used by other agents +// * `Guava`: specifically agent use is unknown at the moment, but Guava is a well known library +// backed by big company with many-many users public class WeakMapManager { private static final long CLEAN_FREQUENCY_SECONDS = 1; private static final List maps = new CopyOnWriteArrayList<>(); From 5e55defeb359887f0a69aeabb26a0962b250c45c Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 7 Aug 2018 13:22:19 +1000 Subject: [PATCH 3/7] Fix bootstrap classpath. --- .../src/main/java/datadog/trace/agent/tooling/Utils.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java index f2ad09cd6b..9fbc0defbd 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java @@ -8,6 +8,7 @@ import java.net.URL; public class Utils { /* packages which will be loaded on the bootstrap classloader*/ public static final String[] BOOTSTRAP_PACKAGE_PREFIXES = { + "com.blogspot.mydailyjava.weaklockfree", "io.opentracing", "datadog.slf4j", "datadog.trace.bootstrap", @@ -34,13 +35,13 @@ public class Utils { private static Method findLoadedClassMethod = null; - private static BootstrapClassLoaderProxy unitTestBootstrapProxy = + private static final BootstrapClassLoaderProxy unitTestBootstrapProxy = new BootstrapClassLoaderProxy(new URL[0], null); static { try { findLoadedClassMethod = ClassLoader.class.getDeclaredMethod("findLoadedClass", String.class); - } catch (NoSuchMethodException | SecurityException e) { + } catch (final NoSuchMethodException | SecurityException e) { throw new IllegalStateException(e); } } From eacc2d6402a2974b4534e4065534bcd30975ee2b Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 8 Aug 2018 16:54:44 +1000 Subject: [PATCH 4/7] Restructure how WeakMap is implemented Use an interface. Use a provider/supplier to avoid loading more onto the bootstrap classpath. --- .../agent-bootstrap/agent-bootstrap.gradle | 3 - .../datadog/trace/bootstrap/JDBCMaps.java | 8 +- .../java/datadog/trace/bootstrap/WeakMap.java | 77 +++++++ .../trace/bootstrap/WeakMapManager.java | 93 -------- .../trace/bootstrap/WeakMapManagerTest.groovy | 41 ---- .../agent-tooling/agent-tooling.gradle | 2 + .../agent/tooling/ClassLoaderMatcher.java | 12 +- .../trace/agent/tooling/TracerInstaller.java | 9 + .../trace/agent/tooling/WeakMapSuppliers.java | 209 ++++++++++++++++++ .../tooling/muzzle/ReferenceMatcher.java | 7 +- .../WeakConcurrentMapSupplierTest.groovy | 45 ++++ .../HttpUrlConnectionInstrumentation.java | 7 +- 12 files changed, 357 insertions(+), 156 deletions(-) create mode 100644 dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java delete mode 100644 dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java delete mode 100644 dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/WeakMapManagerTest.groovy create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java create mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentMapSupplierTest.groovy diff --git a/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle b/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle index 6a96636434..facd73c5fa 100644 --- a/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle +++ b/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle @@ -9,13 +9,10 @@ excludedClassesConverage += ['datadog.trace.bootstrap.*'] dependencies { compile project(':dd-trace-api') - compile group: 'com.blogspot.mydailyjava', name: 'weak-lock-free', version: '0.13' compile deps.opentracing compile deps.slf4j compile group: 'org.slf4j', name: 'slf4j-simple', version: versions.slf4j // ^ Generally a bad idea for libraries, but we're shadowing. - - testCompile project(':dd-java-agent:testing') } jar { diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java index 4fdc53874d..99b8193278 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java @@ -1,8 +1,7 @@ package datadog.trace.bootstrap; -import static datadog.trace.bootstrap.WeakMapManager.newWeakMap; +import static datadog.trace.bootstrap.WeakMap.Provider.newWeakMap; -import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import java.sql.Connection; import java.sql.PreparedStatement; import lombok.Data; @@ -13,9 +12,8 @@ import lombok.Data; *

In the bootstrap project to ensure visibility by all classes. */ public class JDBCMaps { - public static final WeakConcurrentMap connectionInfo = newWeakMap(); - public static final WeakConcurrentMap preparedStatements = - newWeakMap(); + public static final WeakMap connectionInfo = newWeakMap(); + public static final WeakMap preparedStatements = newWeakMap(); public static final String DB_QUERY = "DB Query"; diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java new file mode 100644 index 0000000000..edcb349c8c --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java @@ -0,0 +1,77 @@ +package datadog.trace.bootstrap; + +import java.util.Collections; +import java.util.Map; +import java.util.WeakHashMap; +import java.util.concurrent.atomic.AtomicReference; + +public interface WeakMap { + + int size(); + + boolean containsKey(K target); + + V get(K key); + + void put(K key, V value); + + class Provider { + private static final AtomicReference provider = + new AtomicReference<>(Supplier.DEFAULT); + + /* The interface would be better defined as a Supplier, because we don't want throw an exception, + * but that wasn't introduced until Java 8 and we must be compatible with 7. + */ + public static void registerIfAbsent(final Supplier provider) { + if (provider != null && provider != Supplier.DEFAULT) { + Provider.provider.compareAndSet(Supplier.DEFAULT, provider); + } + } + + public static WeakMap newWeakMap() { + return provider.get().get(); + } + } + + interface Supplier { + WeakMap get(); + + Supplier DEFAULT = new Default(); + + class Default implements Supplier { + + @Override + public WeakMap get() { + return new Adapter<>(Collections.synchronizedMap(new WeakHashMap())); + } + + private static class Adapter implements WeakMap { + private final Map map; + + private Adapter(final Map map) { + this.map = map; + } + + @Override + public int size() { + return map.size(); + } + + @Override + public boolean containsKey(final K key) { + return map.containsKey(key); + } + + @Override + public V get(final K key) { + return map.get(key); + } + + @Override + public void put(final K key, final V value) { + map.put(key, value); + } + } + } + } +} diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java deleted file mode 100644 index 2c752bb52d..0000000000 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMapManager.java +++ /dev/null @@ -1,93 +0,0 @@ -package datadog.trace.bootstrap; - -import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; -import java.util.List; -import java.util.concurrent.CopyOnWriteArrayList; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ThreadFactory; -import java.util.concurrent.TimeUnit; - -/** - * Provides instances of {@link WeakConcurrentMap} and retains reference to them to allow a single - * thread to clean void weak references out for all instances. Cleaning is done every second. - */ - -// Comparison with using WeakConcurrentMap vs Guava's implementation: -// Cleaning: -// * `WeakConcurrentMap`: centralized but we have to maintain out own code and thread for it -// * `Guava`: inline on application's thread, with constant max delay -// Jar Size: -// * `WeakConcurrentMap`: small -// * `Guava`: large, but we may use other features, like immutable collections - and we already ship -// Guava as part of distribution now, so using Guava for this doesn’t increase size. -// Must go on bootstrap classpath: -// * `WeakConcurrentMap`: version conflict is unlikely, so we can directly inject for now -// * `Guava`: need to implement shadow copy (might eventually be necessary for other dependencies) -// Used by other javaagents for similar purposes: -// * `WeakConcurrentMap`: anecdotally used by other agents -// * `Guava`: specifically agent use is unknown at the moment, but Guava is a well known library -// backed by big company with many-many users -public class WeakMapManager { - private static final long CLEAN_FREQUENCY_SECONDS = 1; - private static final List maps = new CopyOnWriteArrayList<>(); - - private static final ThreadFactory weakMapCleanerThreadFactory = - new ThreadFactory() { - @Override - public Thread newThread(final Runnable r) { - final Thread thread = new Thread(r, "dd-weak-ref-cleaner"); - thread.setDaemon(true); - thread.setPriority(Thread.MIN_PRIORITY); - return thread; - } - }; - - private static final ScheduledExecutorService cleaner = - Executors.newScheduledThreadPool(1, weakMapCleanerThreadFactory); - - private static final Runnable runnable = new Cleaner(); - - static { - cleaner.scheduleAtFixedRate( - runnable, CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); - - try { - Runtime.getRuntime() - .addShutdownHook( - new Thread() { - @Override - public void run() { - try { - cleaner.shutdownNow(); - cleaner.awaitTermination(5, TimeUnit.SECONDS); - } catch (final InterruptedException e) { - // Don't bother waiting then... - } - } - }); - } catch (final IllegalStateException ex) { - // The JVM is already shutting down. - } - } - - public static WeakConcurrentMap newWeakMap() { - final WeakConcurrentMap map = new WeakConcurrentMap<>(false); - maps.add(map); - return map; - } - - public static void cleanMaps() { - for (final WeakConcurrentMap map : maps) { - map.expungeStaleEntries(); - } - } - - private static class Cleaner implements Runnable { - - @Override - public void run() { - cleanMaps(); - } - } -} diff --git a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/WeakMapManagerTest.groovy b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/WeakMapManagerTest.groovy deleted file mode 100644 index 5428aae8a2..0000000000 --- a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/WeakMapManagerTest.groovy +++ /dev/null @@ -1,41 +0,0 @@ -package datadog.trace.bootstrap - -import datadog.trace.agent.test.TestUtils -import spock.lang.Specification - -class WeakMapManagerTest extends Specification { - - def setup() { - WeakMapManager.maps.clear() - } - - def "calling new adds to the list"() { - when: - def map1 = WeakMapManager.newWeakMap() - - then: - WeakMapManager.maps == [map1] - - when: - def map2 = WeakMapManager.newWeakMap() - - then: - WeakMapManager.maps == [map1, map2] - } - - def "calling cleanMaps does cleanup"() { - setup: - def map = WeakMapManager.newWeakMap() - map.put(new Object(), "value") - TestUtils.awaitGC() - - expect: - map.approximateSize() == 1 - - when: - WeakMapManager.cleanMaps() - - then: - map.approximateSize() == 0 - } -} diff --git a/dd-java-agent/agent-tooling/agent-tooling.gradle b/dd-java-agent/agent-tooling/agent-tooling.gradle index b44748192e..0c28a3bd4f 100644 --- a/dd-java-agent/agent-tooling/agent-tooling.gradle +++ b/dd-java-agent/agent-tooling/agent-tooling.gradle @@ -9,6 +9,7 @@ configurations { dependencies { compile project(':dd-java-agent:agent-bootstrap') + compile group: 'com.blogspot.mydailyjava', name: 'weak-lock-free', version: '0.13' compile deps.bytebuddy compile deps.bytebuddyagent annotationProcessor deps.autoservice @@ -17,6 +18,7 @@ dependencies { compileOnly project(':dd-trace-ot') testCompile deps.opentracing + testCompile project(':dd-java-agent:testing') instrumentationMuzzle sourceSets.main.output instrumentationMuzzle configurations.compile diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java index a8cba595f9..068916e882 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java @@ -1,10 +1,10 @@ package datadog.trace.agent.tooling; -import static datadog.trace.bootstrap.WeakMapManager.newWeakMap; +import static datadog.trace.bootstrap.WeakMap.Provider.newWeakMap; -import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import datadog.trace.bootstrap.DatadogClassLoader; import datadog.trace.bootstrap.PatchLogger; +import datadog.trace.bootstrap.WeakMap; import io.opentracing.util.GlobalTracer; import java.util.Collections; import java.util.HashSet; @@ -44,7 +44,7 @@ public class ClassLoaderMatcher { extends ElementMatcher.Junction.AbstractBase { public static final SkipClassLoaderMatcher INSTANCE = new SkipClassLoaderMatcher(); /* Cache of classloader-instance -> (true|false). True = skip instrumentation. False = safe to instrument. */ - private static final WeakConcurrentMap SKIP_CACHE = newWeakMap(); + private static final WeakMap SKIP_CACHE = newWeakMap(); private static final Set CLASSLOADER_CLASSES_TO_SKIP; static { @@ -131,7 +131,7 @@ public class ClassLoaderMatcher { public static class ClassLoaderHasClassMatcher extends ElementMatcher.Junction.AbstractBase { - private final WeakConcurrentMap cache = newWeakMap(); + private final WeakMap cache = newWeakMap(); private final String[] names; @@ -163,7 +163,7 @@ public class ClassLoaderMatcher { public static class ClassLoaderHasClassWithFieldMatcher extends ElementMatcher.Junction.AbstractBase { - private final WeakConcurrentMap cache = newWeakMap(); + private final WeakMap cache = newWeakMap(); private final String className; private final String fieldName; @@ -201,7 +201,7 @@ public class ClassLoaderMatcher { public static class ClassLoaderHasClassWithMethodMatcher extends ElementMatcher.Junction.AbstractBase { - private final WeakConcurrentMap cache = newWeakMap(); + private final WeakMap cache = newWeakMap(); private final String className; private final String methodName; diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/TracerInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/TracerInstaller.java index 50ff3ab104..b08486e926 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/TracerInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/TracerInstaller.java @@ -1,6 +1,7 @@ package datadog.trace.agent.tooling; import datadog.opentracing.DDTracer; +import datadog.trace.bootstrap.WeakMap; import io.opentracing.Tracer; import io.opentracing.util.GlobalTracer; import lombok.extern.slf4j.Slf4j; @@ -9,6 +10,8 @@ import lombok.extern.slf4j.Slf4j; public class TracerInstaller { /** Register a global tracer if no global tracer is already registered. */ public static synchronized void installGlobalTracer() { + registerWeakMapProvider(); + if (!GlobalTracer.isRegistered()) { final Tracer resolved = new DDTracer(); try { @@ -27,4 +30,10 @@ public class TracerInstaller { log.debug( AgentInstaller.class.getName() + " loaded on " + AgentInstaller.class.getClassLoader()); } + + private static void registerWeakMapProvider() { + WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WCMManaged()); + // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WCMInline()); + // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); + } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java new file mode 100644 index 0000000000..b3d03a5b1e --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java @@ -0,0 +1,209 @@ +package datadog.trace.agent.tooling; + +import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; +import com.google.common.collect.MapMaker; +import datadog.trace.bootstrap.WeakMap; +import java.lang.ref.WeakReference; +import java.util.Iterator; +import java.util.Map; +import java.util.Queue; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; + +class WeakMapSuppliers { + // Comparison with using WeakConcurrentMap vs Guava's implementation: + // Cleaning: + // * `WeakConcurrentMap`: centralized but we have to maintain out own code and thread for it + // * `Guava`: inline on application's thread, with constant max delay + // Jar Size: + // * `WeakConcurrentMap`: small + // * `Guava`: large, but we may use other features, like immutable collections - and we already + // ship Guava as part of distribution now, so using Guava for this doesn’t increase size. + // Must go on bootstrap classpath: + // * `WeakConcurrentMap`: version conflict is unlikely, so we can directly inject for now + // * `Guava`: need to implement shadow copy (might eventually be necessary for other dependencies) + // Used by other javaagents for similar purposes: + // * `WeakConcurrentMap`: anecdotally used by other agents + // * `Guava`: specifically agent use is unknown at the moment, but Guava is a well known library + // backed by big company with many-many users + + /** + * Provides instances of {@link WeakConcurrentMap} and retains weak reference to them to allow a + * single thread to clean void weak references out for all instances. Cleaning is done every + * second. + */ + static class WCMManaged implements WeakMap.Supplier { + private static final long CLEAN_FREQUENCY_SECONDS = 1; + private static final Queue> maps = + new ConcurrentLinkedQueue<>(); + + private static final ThreadFactory weakMapCleanerThreadFactory = + new ThreadFactory() { + @Override + public Thread newThread(final Runnable r) { + final Thread thread = new Thread(r, "dd-weak-ref-cleaner"); + thread.setDaemon(true); + thread.setPriority(Thread.MIN_PRIORITY); + return thread; + } + }; + + private static final ScheduledExecutorService cleaner = + Executors.newScheduledThreadPool(1, weakMapCleanerThreadFactory); + + private static final Runnable runnable = new Cleaner(); + + static { + cleaner.scheduleAtFixedRate( + runnable, CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); + + try { + Runtime.getRuntime() + .addShutdownHook( + new Thread() { + @Override + public void run() { + try { + cleaner.shutdownNow(); + cleaner.awaitTermination(5, TimeUnit.SECONDS); + } catch (final InterruptedException e) { + // Don't bother waiting then... + } + } + }); + } catch (final IllegalStateException ex) { + // The JVM is already shutting down. + } + } + + public static void cleanMaps() { + for (final Iterator> iterator = maps.iterator(); + iterator.hasNext(); ) { + final WeakConcurrentMap map = iterator.next().get(); + if (map == null) { + iterator.remove(); + } else { + map.expungeStaleEntries(); + } + } + } + + @Override + public WeakMap get() { + final WeakConcurrentMap map = new WeakConcurrentMap<>(false); + maps.add(new WeakReference(map)); + return new Adapter<>(map); + } + + private static class Cleaner implements Runnable { + + @Override + public void run() { + cleanMaps(); + } + } + + private static class Adapter implements WeakMap { + private final WeakConcurrentMap map; + + private Adapter(final WeakConcurrentMap map) { + this.map = map; + } + + @Override + public int size() { + return map.approximateSize(); + } + + @Override + public boolean containsKey(final K key) { + return map.containsKey(key); + } + + @Override + public V get(final K key) { + return map.get(key); + } + + @Override + public void put(final K key, final V value) { + map.put(key, value); + } + } + } + + static class WCMInline implements WeakMap.Supplier { + + @Override + public WeakMap get() { + return new Adapter<>(new WeakConcurrentMap.WithInlinedExpunction()); + } + + private static class Adapter implements WeakMap { + private final WeakConcurrentMap map; + + private Adapter(final WeakConcurrentMap map) { + this.map = map; + } + + @Override + public int size() { + return map.approximateSize(); + } + + @Override + public boolean containsKey(final K key) { + return map.containsKey(key); + } + + @Override + public V get(final K key) { + return map.get(key); + } + + @Override + public void put(final K key, final V value) { + map.put(key, value); + } + } + } + + static class Guava implements WeakMap.Supplier { + + @Override + public WeakMap get() { + return new Adapter<>(new MapMaker().weakKeys().makeMap()); + } + + private static class Adapter implements WeakMap { + private final Map map; + + private Adapter(final Map map) { + this.map = map; + } + + @Override + public int size() { + return map.size(); + } + + @Override + public boolean containsKey(final K key) { + return map.containsKey(key); + } + + @Override + public V get(final K key) { + return map.get(key); + } + + @Override + public void put(final K key, final V value) { + map.put(key, value); + } + } + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index d1baf9db0c..e5ed74906d 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -1,10 +1,10 @@ package datadog.trace.agent.tooling.muzzle; -import static datadog.trace.bootstrap.WeakMapManager.newWeakMap; +import static datadog.trace.bootstrap.WeakMap.Provider.newWeakMap; import static net.bytebuddy.dynamic.loading.ClassLoadingStrategy.BOOTSTRAP_LOADER; -import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import datadog.trace.agent.tooling.Utils; +import datadog.trace.bootstrap.WeakMap; import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; @@ -15,8 +15,7 @@ import lombok.extern.slf4j.Slf4j; /** Matches a set of references against a classloader. */ @Slf4j public class ReferenceMatcher { - private final WeakConcurrentMap> mismatchCache = - newWeakMap(); + private final WeakMap> mismatchCache = newWeakMap(); private final Reference[] references; private final Set helperClassNames; diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentMapSupplierTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentMapSupplierTest.groovy new file mode 100644 index 0000000000..749a0f4c98 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentMapSupplierTest.groovy @@ -0,0 +1,45 @@ +package datadog.trace.agent.tooling + +import datadog.trace.agent.test.TestUtils +import datadog.trace.bootstrap.WeakMap +import spock.lang.Specification + +class WeakConcurrentMapSupplierTest extends Specification { + def supplier = new WeakMapSuppliers.WCMManaged() + + def setup() { + WeakMap.Provider.provider.set(supplier) + } + + def "calling new adds to the list"() { + when: + def map1 = WeakMap.Provider.newWeakMap().map + + then: + supplier.maps.iterator().next().get() == map1 + + when: + def map2 = WeakMap.Provider.newWeakMap().map + def iterator = supplier.maps.iterator() + + then: + iterator.next().get() == map1 + iterator.next().get() == map2 + } + + def "calling cleanMaps does cleanup"() { + setup: + def map = WeakMap.Provider.newWeakMap() + map.put(new Object(), "value") + TestUtils.awaitGC() + + expect: + map.size() == 1 + + when: + supplier.cleanMaps() + + then: + map.size() == 0 + } +} diff --git a/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java index 7a07081633..1a21ee281d 100644 --- a/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java +++ b/dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java @@ -1,7 +1,7 @@ package datadog.trace.instrumentation.http_url_connection; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static datadog.trace.bootstrap.WeakMapManager.newWeakMap; +import static datadog.trace.bootstrap.WeakMap.Provider.newWeakMap; import static io.opentracing.log.Fields.ERROR_OBJECT; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; @@ -9,12 +9,12 @@ import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.not; -import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.api.DDSpanTypes; import datadog.trace.api.DDTags; import datadog.trace.bootstrap.CallDepthThreadLocalMap; +import datadog.trace.bootstrap.WeakMap; import io.opentracing.Scope; import io.opentracing.Span; import io.opentracing.Tracer; @@ -175,8 +175,7 @@ public class HttpUrlConnectionInstrumentation extends Instrumenter.Default { } public static class HttpURLState { - private static final WeakConcurrentMap STATE_MAP = - newWeakMap(); + private static final WeakMap STATE_MAP = newWeakMap(); public static HttpURLState get(final HttpURLConnection connection) { HttpURLState state = STATE_MAP.get(connection); From b220309a6892c2994d6d5f78be7c3a1f59214572 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 9 Aug 2018 16:31:09 +1000 Subject: [PATCH 5/7] PR review changes. --- .../java/datadog/trace/bootstrap/WeakMap.java | 64 ++++++------- .../trace/agent/tooling/AgentInstaller.java | 9 ++ .../trace/agent/tooling/TracerInstaller.java | 9 -- .../datadog/trace/agent/tooling/Utils.java | 3 +- .../trace/agent/tooling/WeakMapSuppliers.java | 91 ++++--------------- ...oovy => WeakConcurrentSupplierTest.groovy} | 10 +- .../datadog/opentracing/PendingTrace.java | 1 - 7 files changed, 66 insertions(+), 121 deletions(-) rename dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/{WeakConcurrentMapSupplierTest.groovy => WeakConcurrentSupplierTest.groovy} (69%) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java index edcb349c8c..070bc4391f 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java @@ -4,6 +4,7 @@ import java.util.Collections; import java.util.Map; import java.util.WeakHashMap; import java.util.concurrent.atomic.AtomicReference; +import lombok.extern.slf4j.Slf4j; public interface WeakMap { @@ -19,9 +20,6 @@ public interface WeakMap { private static final AtomicReference provider = new AtomicReference<>(Supplier.DEFAULT); - /* The interface would be better defined as a Supplier, because we don't want throw an exception, - * but that wasn't introduced until Java 8 and we must be compatible with 7. - */ public static void registerIfAbsent(final Supplier provider) { if (provider != null && provider != Supplier.DEFAULT) { Provider.provider.compareAndSet(Supplier.DEFAULT, provider); @@ -38,40 +36,42 @@ public interface WeakMap { Supplier DEFAULT = new Default(); + @Slf4j class Default implements Supplier { @Override public WeakMap get() { - return new Adapter<>(Collections.synchronizedMap(new WeakHashMap())); - } - - private static class Adapter implements WeakMap { - private final Map map; - - private Adapter(final Map map) { - this.map = map; - } - - @Override - public int size() { - return map.size(); - } - - @Override - public boolean containsKey(final K key) { - return map.containsKey(key); - } - - @Override - public V get(final K key) { - return map.get(key); - } - - @Override - public void put(final K key, final V value) { - map.put(key, value); - } + log.warn("WeakMap.Supplier not registered. Returning a synchronized WeakHashMap."); + return new MapAdapter<>(Collections.synchronizedMap(new WeakHashMap())); } } } + + class MapAdapter implements WeakMap { + private final Map map; + + public MapAdapter(final Map map) { + this.map = map; + } + + @Override + public int size() { + return map.size(); + } + + @Override + public boolean containsKey(final K key) { + return map.containsKey(key); + } + + @Override + public V get(final K key) { + return map.get(key); + } + + @Override + public void put(final K key, final V value) { + map.put(key, value); + } + } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index 059feb96bd..08f19f8072 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -8,6 +8,7 @@ import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.not; +import datadog.trace.bootstrap.WeakMap; import java.lang.instrument.Instrumentation; import java.util.ServiceLoader; import lombok.extern.slf4j.Slf4j; @@ -38,6 +39,8 @@ public class AgentInstaller { public static ResettableClassFileTransformer installBytebuddyAgent( final Instrumentation inst, final AgentBuilder.Listener... listeners) { INSTRUMENTATION = inst; + registerWeakMapProvider(); + AgentBuilder agentBuilder = new AgentBuilder.Default() .disableClassFormatChanges() @@ -80,6 +83,12 @@ public class AgentInstaller { return agentBuilder.installOn(inst); } + private static void registerWeakMapProvider() { + WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent()); + // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent.Inline()); + // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); + } + @Slf4j static class LoggingListener implements AgentBuilder.Listener { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/TracerInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/TracerInstaller.java index b08486e926..50ff3ab104 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/TracerInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/TracerInstaller.java @@ -1,7 +1,6 @@ package datadog.trace.agent.tooling; import datadog.opentracing.DDTracer; -import datadog.trace.bootstrap.WeakMap; import io.opentracing.Tracer; import io.opentracing.util.GlobalTracer; import lombok.extern.slf4j.Slf4j; @@ -10,8 +9,6 @@ import lombok.extern.slf4j.Slf4j; public class TracerInstaller { /** Register a global tracer if no global tracer is already registered. */ public static synchronized void installGlobalTracer() { - registerWeakMapProvider(); - if (!GlobalTracer.isRegistered()) { final Tracer resolved = new DDTracer(); try { @@ -30,10 +27,4 @@ public class TracerInstaller { log.debug( AgentInstaller.class.getName() + " loaded on " + AgentInstaller.class.getClassLoader()); } - - private static void registerWeakMapProvider() { - WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WCMManaged()); - // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WCMInline()); - // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); - } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java index 9fbc0defbd..6ce61707c4 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java @@ -8,7 +8,6 @@ import java.net.URL; public class Utils { /* packages which will be loaded on the bootstrap classloader*/ public static final String[] BOOTSTRAP_PACKAGE_PREFIXES = { - "com.blogspot.mydailyjava.weaklockfree", "io.opentracing", "datadog.slf4j", "datadog.trace.bootstrap", @@ -23,6 +22,8 @@ public class Utils { "com.google.auto", "com.google.common", "com.google.thirdparty.publicsuffix", + // WeakConcurrentMap + "com.blogspot.mydailyjava.weaklockfree", // bytebuddy "net.bytebuddy", // OT contribs for dd trace resolver diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java index b3d03a5b1e..af1e994fa8 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java @@ -5,7 +5,6 @@ import com.google.common.collect.MapMaker; import datadog.trace.bootstrap.WeakMap; import java.lang.ref.WeakReference; import java.util.Iterator; -import java.util.Map; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.Executors; @@ -35,12 +34,14 @@ class WeakMapSuppliers { * single thread to clean void weak references out for all instances. Cleaning is done every * second. */ - static class WCMManaged implements WeakMap.Supplier { + static class WeakConcurrent implements WeakMap.Supplier { private static final long CLEAN_FREQUENCY_SECONDS = 1; - private static final Queue> maps = + private static final long SHUTDOWN_WAIT_SECONDS = 5; + + private static final Queue> SUPPLIED_MAPS = new ConcurrentLinkedQueue<>(); - private static final ThreadFactory weakMapCleanerThreadFactory = + private static final ThreadFactory THREAD_FACTORY = new ThreadFactory() { @Override public Thread newThread(final Runnable r) { @@ -51,14 +52,14 @@ class WeakMapSuppliers { } }; - private static final ScheduledExecutorService cleaner = - Executors.newScheduledThreadPool(1, weakMapCleanerThreadFactory); + private static final ScheduledExecutorService CLEANER = + Executors.newScheduledThreadPool(1, THREAD_FACTORY); - private static final Runnable runnable = new Cleaner(); + private static final Runnable RUNNABLE = new Cleaner(); static { - cleaner.scheduleAtFixedRate( - runnable, CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); + CLEANER.scheduleAtFixedRate( + RUNNABLE, CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); try { Runtime.getRuntime() @@ -67,8 +68,8 @@ class WeakMapSuppliers { @Override public void run() { try { - cleaner.shutdownNow(); - cleaner.awaitTermination(5, TimeUnit.SECONDS); + CLEANER.shutdownNow(); + CLEANER.awaitTermination(SHUTDOWN_WAIT_SECONDS, TimeUnit.SECONDS); } catch (final InterruptedException e) { // Don't bother waiting then... } @@ -80,7 +81,7 @@ class WeakMapSuppliers { } public static void cleanMaps() { - for (final Iterator> iterator = maps.iterator(); + for (final Iterator> iterator = SUPPLIED_MAPS.iterator(); iterator.hasNext(); ) { final WeakConcurrentMap map = iterator.next().get(); if (map == null) { @@ -94,7 +95,7 @@ class WeakMapSuppliers { @Override public WeakMap get() { final WeakConcurrentMap map = new WeakConcurrentMap<>(false); - maps.add(new WeakReference(map)); + SUPPLIED_MAPS.add(new WeakReference(map)); return new Adapter<>(map); } @@ -133,40 +134,12 @@ class WeakMapSuppliers { map.put(key, value); } } - } - static class WCMInline implements WeakMap.Supplier { - - @Override - public WeakMap get() { - return new Adapter<>(new WeakConcurrentMap.WithInlinedExpunction()); - } - - private static class Adapter implements WeakMap { - private final WeakConcurrentMap map; - - private Adapter(final WeakConcurrentMap map) { - this.map = map; - } + static class Inline implements WeakMap.Supplier { @Override - public int size() { - return map.approximateSize(); - } - - @Override - public boolean containsKey(final K key) { - return map.containsKey(key); - } - - @Override - public V get(final K key) { - return map.get(key); - } - - @Override - public void put(final K key, final V value) { - map.put(key, value); + public WeakMap get() { + return new Adapter<>(new WeakConcurrentMap.WithInlinedExpunction()); } } } @@ -175,35 +148,7 @@ class WeakMapSuppliers { @Override public WeakMap get() { - return new Adapter<>(new MapMaker().weakKeys().makeMap()); - } - - private static class Adapter implements WeakMap { - private final Map map; - - private Adapter(final Map map) { - this.map = map; - } - - @Override - public int size() { - return map.size(); - } - - @Override - public boolean containsKey(final K key) { - return map.containsKey(key); - } - - @Override - public V get(final K key) { - return map.get(key); - } - - @Override - public void put(final K key, final V value) { - map.put(key, value); - } + return new WeakMap.MapAdapter<>(new MapMaker().weakKeys().makeMap()); } } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentMapSupplierTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy similarity index 69% rename from dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentMapSupplierTest.groovy rename to dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy index 749a0f4c98..82cad32484 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentMapSupplierTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy @@ -4,8 +4,8 @@ import datadog.trace.agent.test.TestUtils import datadog.trace.bootstrap.WeakMap import spock.lang.Specification -class WeakConcurrentMapSupplierTest extends Specification { - def supplier = new WeakMapSuppliers.WCMManaged() +class WeakConcurrentSupplierTest extends Specification { + def supplier = new WeakMapSuppliers.WeakConcurrent() def setup() { WeakMap.Provider.provider.set(supplier) @@ -16,11 +16,11 @@ class WeakConcurrentMapSupplierTest extends Specification { def map1 = WeakMap.Provider.newWeakMap().map then: - supplier.maps.iterator().next().get() == map1 + supplier.SUPPLIED_MAPS.iterator().next().get() == map1 when: def map2 = WeakMap.Provider.newWeakMap().map - def iterator = supplier.maps.iterator() + def iterator = supplier.SUPPLIED_MAPS.iterator() then: iterator.next().get() == map1 @@ -34,7 +34,7 @@ class WeakConcurrentMapSupplierTest extends Specification { TestUtils.awaitGC() expect: - map.size() == 1 + map.size() == 1 // This might result in an error if supplier's cleaner thread is activated. when: supplier.cleanMaps() diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java b/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java index ff4bf8371d..11e7e13557 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java @@ -239,7 +239,6 @@ public class PendingTrace extends ConcurrentLinkedDeque { public Thread newThread(final Runnable r) { final Thread thread = new Thread(r, "dd-span-cleaner"); thread.setDaemon(true); - thread.setPriority(Thread.MIN_PRIORITY); return thread; } }; From 6cef9b89b07e26c110fb44aed4c11b299abd38d8 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 10 Aug 2018 13:26:07 +1000 Subject: [PATCH 6/7] Make some things non-static --- .../trace/agent/tooling/WeakMapSuppliers.java | 38 +++++++++---------- .../tooling/WeakConcurrentSupplierTest.groovy | 4 +- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java index af1e994fa8..484adeac75 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java @@ -38,9 +38,6 @@ class WeakMapSuppliers { private static final long CLEAN_FREQUENCY_SECONDS = 1; private static final long SHUTDOWN_WAIT_SECONDS = 5; - private static final Queue> SUPPLIED_MAPS = - new ConcurrentLinkedQueue<>(); - private static final ThreadFactory THREAD_FACTORY = new ThreadFactory() { @Override @@ -55,12 +52,7 @@ class WeakMapSuppliers { private static final ScheduledExecutorService CLEANER = Executors.newScheduledThreadPool(1, THREAD_FACTORY); - private static final Runnable RUNNABLE = new Cleaner(); - static { - CLEANER.scheduleAtFixedRate( - RUNNABLE, CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); - try { Runtime.getRuntime() .addShutdownHook( @@ -80,8 +72,24 @@ class WeakMapSuppliers { } } - public static void cleanMaps() { - for (final Iterator> iterator = SUPPLIED_MAPS.iterator(); + private final Queue> suppliedMaps = + new ConcurrentLinkedQueue<>(); + + private final Runnable runnable = + new Runnable() { + @Override + public void run() { + cleanMaps(); + } + }; + + WeakConcurrent() { + CLEANER.scheduleAtFixedRate( + runnable, CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); + } + + public void cleanMaps() { + for (final Iterator> iterator = suppliedMaps.iterator(); iterator.hasNext(); ) { final WeakConcurrentMap map = iterator.next().get(); if (map == null) { @@ -95,18 +103,10 @@ class WeakMapSuppliers { @Override public WeakMap get() { final WeakConcurrentMap map = new WeakConcurrentMap<>(false); - SUPPLIED_MAPS.add(new WeakReference(map)); + suppliedMaps.add(new WeakReference(map)); return new Adapter<>(map); } - private static class Cleaner implements Runnable { - - @Override - public void run() { - cleanMaps(); - } - } - private static class Adapter implements WeakMap { private final WeakConcurrentMap map; diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy index 82cad32484..84ac195f79 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy @@ -16,11 +16,11 @@ class WeakConcurrentSupplierTest extends Specification { def map1 = WeakMap.Provider.newWeakMap().map then: - supplier.SUPPLIED_MAPS.iterator().next().get() == map1 + supplier.suppliedMaps.iterator().next().get() == map1 when: def map2 = WeakMap.Provider.newWeakMap().map - def iterator = supplier.SUPPLIED_MAPS.iterator() + def iterator = supplier.suppliedMaps.iterator() then: iterator.next().get() == map1 From fc8cc47f850ab13d5b92e1f2d4b0dbb2435cdf2e Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 9 Aug 2018 22:58:24 -0700 Subject: [PATCH 7/7] WeakMap improvements * Improve tests to not depend on thread schedulting * Test all underlying implementations * Reduce number of static values --- .../java/datadog/trace/bootstrap/WeakMap.java | 5 + .../trace/agent/tooling/WeakMapSuppliers.java | 74 ++++++------ .../tooling/WeakConcurrentSupplierTest.groovy | 111 ++++++++++++++---- 3 files changed, 130 insertions(+), 60 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java index 070bc4391f..d1fdc6aff8 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java @@ -73,5 +73,10 @@ public interface WeakMap { public void put(final K key, final V value) { map.put(key, value); } + + @Override + public String toString() { + return map.toString(); + } } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java index 484adeac75..9f948e2228 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java @@ -1,6 +1,7 @@ package datadog.trace.agent.tooling; import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.MapMaker; import datadog.trace.bootstrap.WeakMap; import java.lang.ref.WeakReference; @@ -35,9 +36,11 @@ class WeakMapSuppliers { * second. */ static class WeakConcurrent implements WeakMap.Supplier { - private static final long CLEAN_FREQUENCY_SECONDS = 1; + private static final long SHUTDOWN_WAIT_SECONDS = 5; + @VisibleForTesting static final long CLEAN_FREQUENCY_SECONDS = 1; + private static final ThreadFactory THREAD_FACTORY = new ThreadFactory() { @Override @@ -49,28 +52,7 @@ class WeakMapSuppliers { } }; - private static final ScheduledExecutorService CLEANER = - Executors.newScheduledThreadPool(1, THREAD_FACTORY); - - static { - try { - Runtime.getRuntime() - .addShutdownHook( - new Thread() { - @Override - public void run() { - try { - CLEANER.shutdownNow(); - CLEANER.awaitTermination(SHUTDOWN_WAIT_SECONDS, TimeUnit.SECONDS); - } catch (final InterruptedException e) { - // Don't bother waiting then... - } - } - }); - } catch (final IllegalStateException ex) { - // The JVM is already shutting down. - } - } + private final ScheduledExecutorService cleanerExecutorService; private final Queue> suppliedMaps = new ConcurrentLinkedQueue<>(); @@ -79,24 +61,41 @@ class WeakMapSuppliers { new Runnable() { @Override public void run() { - cleanMaps(); + for (final Iterator> iterator = + suppliedMaps.iterator(); + iterator.hasNext(); ) { + final WeakConcurrentMap map = iterator.next().get(); + if (map == null) { + iterator.remove(); + } else { + map.expungeStaleEntries(); + } + } } }; WeakConcurrent() { - CLEANER.scheduleAtFixedRate( + cleanerExecutorService = Executors.newScheduledThreadPool(1, THREAD_FACTORY); + cleanerExecutorService.scheduleAtFixedRate( runnable, CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); - } - public void cleanMaps() { - for (final Iterator> iterator = suppliedMaps.iterator(); - iterator.hasNext(); ) { - final WeakConcurrentMap map = iterator.next().get(); - if (map == null) { - iterator.remove(); - } else { - map.expungeStaleEntries(); - } + try { + Runtime.getRuntime() + .addShutdownHook( + new Thread() { + @Override + public void run() { + try { + cleanerExecutorService.shutdownNow(); + cleanerExecutorService.awaitTermination( + SHUTDOWN_WAIT_SECONDS, TimeUnit.SECONDS); + } catch (final InterruptedException e) { + // Don't bother waiting then... + } + } + }); + } catch (final IllegalStateException ex) { + // The JVM is already shutting down. } } @@ -150,5 +149,10 @@ class WeakMapSuppliers { public WeakMap get() { return new WeakMap.MapAdapter<>(new MapMaker().weakKeys().makeMap()); } + + public WeakMap get(int concurrencyLevel) { + return new WeakMap.MapAdapter<>( + new MapMaker().concurrencyLevel(concurrencyLevel).weakKeys().makeMap()); + } } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy index 84ac195f79..a8245c5570 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy @@ -2,44 +2,105 @@ package datadog.trace.agent.tooling import datadog.trace.agent.test.TestUtils import datadog.trace.bootstrap.WeakMap +import spock.lang.Shared import spock.lang.Specification +import java.lang.ref.WeakReference +import java.util.concurrent.TimeUnit + class WeakConcurrentSupplierTest extends Specification { - def supplier = new WeakMapSuppliers.WeakConcurrent() + @Shared + def weakConcurrentSupplier = new WeakMapSuppliers.WeakConcurrent() + @Shared + def weakInlineSupplier = new WeakMapSuppliers.WeakConcurrent.Inline() + @Shared + def guavaSupplier = new WeakMapSuppliers.Guava() - def setup() { - WeakMap.Provider.provider.set(supplier) - } - - def "calling new adds to the list"() { - when: - def map1 = WeakMap.Provider.newWeakMap().map - - then: - supplier.suppliedMaps.iterator().next().get() == map1 - - when: - def map2 = WeakMap.Provider.newWeakMap().map - def iterator = supplier.suppliedMaps.iterator() - - then: - iterator.next().get() == map1 - iterator.next().get() == map2 - } - - def "calling cleanMaps does cleanup"() { + def "Calling newWeakMap on #name creates independent maps"() { setup: + WeakMap.Provider.provider.set(supplier) + def key = new Object() + def map1 = WeakMap.Provider.newWeakMap() + def map2 = WeakMap.Provider.newWeakMap() + + when: + map1.put(key, "value1") + map2.put(key, "value2") + + then: + map1.get(key) == "value1" + map2.get(key) == "value2" + + where: + name | supplier + "WeakConcurrent" | weakConcurrentSupplier + "WeakInline" | weakInlineSupplier + "Guava" | guavaSupplier + } + + def "Unreferenced map get cleaned up on #name"() { + setup: + WeakMap.Provider.provider.set(supplier) def map = WeakMap.Provider.newWeakMap() - map.put(new Object(), "value") + def ref = new WeakReference(map) + + when: + map = null + TestUtils.awaitGC() + + then: + ref.get() == null + + where: + name | supplier + "WeakConcurrent" | weakConcurrentSupplier + "WeakInline" | weakInlineSupplier + "Guava" | guavaSupplier + } + + def "Unreferenced keys get cleaned up on #name"() { + setup: + def key = new Object() + map.put(key, "value") TestUtils.awaitGC() expect: - map.size() == 1 // This might result in an error if supplier's cleaner thread is activated. + map.size() == 1 when: - supplier.cleanMaps() + key = null + TestUtils.awaitGC() + + if (name == "WeakConcurrent") { + // Sleep enough time for cleanup thread to get scheduled. + // But on a very slow box (or high load) scheduling may not be exactly predictable + // so we try a few times. + int count = 0 + while (map.size() != 0 && count < 10) { + Thread.sleep(TimeUnit.SECONDS.toMillis(WeakMapSuppliers.WeakConcurrent.CLEAN_FREQUENCY_SECONDS)) + count++ + } + } + + // Hit map a few times to trigger unreferenced entries cleanup. + // Exact number of times that we need to hit map is implementation dependent. + // For Guava it i specified in + // com.google.common.collect.MapMakerInternalMap.DRAIN_THRESHOLD = 0x3F + if (name == "Guava" || name == "WeakInline") { + for (int i = 0; i <= 0x3F; i++) { + map.get("test") + } + } then: map.size() == 0 + + where: + name | map + "WeakConcurrent" | weakConcurrentSupplier.get() + "WeakInline" | weakInlineSupplier.get() + // Guava's cleanup process depends on concurrency level, + // and in order to be able to test it we need to set concurrency to 1 + "Guava" | guavaSupplier.get(1) } }