From 17af9b752cec09fbc200e498883ed52c2efb7c54 Mon Sep 17 00:00:00 2001 From: dougqh Date: Thu, 23 Jan 2020 10:40:48 -0500 Subject: [PATCH 01/11] Fix typo in test name --- .../agent/integration/classloading/ClassLoadingTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy b/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy index 0a24d3b322..c4ba7012fa 100644 --- a/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy +++ b/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy @@ -78,7 +78,7 @@ class ClassLoadingTest extends Specification { loader.count == countAfterFirstLoad } - def "make sure that ByteBuddy doesn't resue cached type descriptions between different classloaders"() { + def "make sure that ByteBuddy doesn't reuse cached type descriptions between different classloaders"() { setup: CountingClassLoader loader1 = new CountingClassLoader(classpath) CountingClassLoader loader2 = new CountingClassLoader(classpath) From 726236bd64d76da9770df52022b89bf623d880a2 Mon Sep 17 00:00:00 2001 From: dougqh Date: Thu, 23 Jan 2020 10:55:16 -0500 Subject: [PATCH 02/11] Type cache overhaul This change overhauls the core type cache The new approach aims to achieve several things... 1 - cache is strictly bounded -- no variance for number of classes of ClassLoaders 2 - cache is significantly smaller 3 - cache doesn't compromise start-up time 4 - primary eviction policy isn't driven by time 5 - primary eviction policy isn't driven by GC There are some slight compromises here. In practice, start-up does increase slightly in a memory rich environment; however, start-up improves considerably in a memory poor environment. The basic approcach is to have a single unified Guava cache for all ClassLoaders -- nominally keyed a composite of ClassLoader & class name The ByteBuddy CacheProvider are simply thin wrappers around the Guava cache associated to a particular ClassLoader However rather than having a large number of WeakReferences floating around. The cache assigns an ID to each ClassLoader. To further avoid, consuming memory the cache only preserves a small map of Loader / ID assignments. This means a ClassLoader may have more than one active ID. This introduce the possibility for ID exhaustion. That unlikely case is handle by retiring the internal CacheInstance and starting anew. --- .../trace/agent/tooling/AgentTooling.java | 2 +- .../agent/tooling/DDCachingPoolStrategy.java | 329 +++++++++++++----- .../agent/tooling/CacheProviderTest.groovy | 223 ++++++++++++ .../tooling/EvictingCacheProviderTest.groovy | 102 ------ 4 files changed, 457 insertions(+), 199 deletions(-) create mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy delete mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/EvictingCacheProviderTest.groovy diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java index ad744a15fd..c20192b383 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java @@ -16,7 +16,7 @@ public class AgentTooling { } private static final DDLocationStrategy LOCATION_STRATEGY = new DDLocationStrategy(); - private static final DDCachingPoolStrategy POOL_STRATEGY = new DDCachingPoolStrategy(CLEANER); + private static final DDCachingPoolStrategy POOL_STRATEGY = new DDCachingPoolStrategy(); public static void init() { // Only need to trigger static initializers for now. diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index 886d742c78..a1ad033970 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -1,146 +1,283 @@ package datadog.trace.agent.tooling; -import static datadog.trace.agent.tooling.ClassLoaderMatcher.BOOTSTRAP_CLASSLOADER; -import static datadog.trace.agent.tooling.ClassLoaderMatcher.skipClassLoader; import static net.bytebuddy.agent.builder.AgentBuilder.PoolStrategy; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; -import datadog.trace.bootstrap.WeakMap; -import java.security.SecureClassLoader; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import lombok.extern.slf4j.Slf4j; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.ClassFileLocator; import net.bytebuddy.pool.TypePool; /** - * Custom Pool strategy. + * NEW (Jan 2020) Custom Pool strategy. * - *

Here we are using WeakMap.Provider as the backing ClassLoader -> CacheProvider lookup. + *

* - *

We also use our bootstrap proxy when matching against the bootstrap loader. + *

* - *

The CacheProvider is a custom implementation that uses guava's cache to expire and limit size. + *

This design was chosen to create a single limited size cache that can be adjusted + * for the entire application -- without having to create a large number of WeakReference objects. * - *

By evicting from the cache we are able to reduce the memory overhead of the agent for apps - * that have many classes. + *

The ID assignment mostly assigns a single ID to each ClassLoader, but the maximumSize + * restriction means that an evicted ClassLoader could be assigned another ID later on. * - *

See eviction policy below. + *

For the validity of the cache, the important part is that ID assignment guarantees that + * no two ClassLoaders share the same ID. + * + *

NOTE: As an additional safe-guard, a new CacheInstance can be created if the original loader ID + * sequence is exhausted. */ -public class DDCachingPoolStrategy - implements PoolStrategy, WeakMap.ValueSupplier { - - // Need this because we can't put null into the typePoolCache map. - private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER = - new SecureClassLoader(null) {}; - - private final WeakMap typePoolCache = - WeakMap.Provider.newWeakMap(); - private final Cleaner cleaner; - - public DDCachingPoolStrategy(final Cleaner cleaner) { - this.cleaner = cleaner; - } +@Slf4j +public class DDCachingPoolStrategy implements PoolStrategy { + /** + * Most of the logic exists in CacheInstance This volatile + exhaustion checking is defense + * against loader ID exhaustion + */ + volatile CacheInstance cacheInstance = new CacheInstance(); @Override public TypePool typePool(final ClassFileLocator classFileLocator, final ClassLoader classLoader) { - final ClassLoader key = - BOOTSTRAP_CLASSLOADER == classLoader ? BOOTSTRAP_CLASSLOADER_PLACEHOLDER : classLoader; - final TypePool.CacheProvider cache = typePoolCache.computeIfAbsent(key, this); + CacheInstance cacheInstance = this.cacheInstance; - return new TypePool.Default.WithLazyResolution( - cache, classFileLocator, TypePool.Default.ReaderMode.FAST); + TypePool typePool = cacheInstance.typePool(classFileLocator, classLoader); + if (cacheInstance.exhaustedLoaderIdSeq()) { + // If the loader ID sequence is exhausted, drop the prior cache & start over + // The ID space is so large that this shouldn't occur + log.error("cacheInstance exhausted - rebuilding cache"); + + this.cacheInstance = new CacheInstance(); + } + return typePool; } - @Override - public TypePool.CacheProvider get(final ClassLoader key) { - if (BOOTSTRAP_CLASSLOADER_PLACEHOLDER != key && skipClassLoader().matches(key)) { - // Don't bother creating a cache for a classloader that won't match. - // (avoiding a lot of DelegatingClassLoader instances) - // This is primarily an optimization. - return TypePool.CacheProvider.NoOp.INSTANCE; - } else { - return EvictingCacheProvider.withObjectType(cleaner, 1, TimeUnit.MINUTES); + /* + * CacheInstance embodies the core of the cache. In general, we only + * expect a single CacheInstance object to ever be created. + * + * However, CacheInstance does provide an extra layer of protection + * against loaderIdSeq exhaustion. If ever the loaderIdSeq of + * CacheInstance is exhausted, then DDCachingPoolStrategy.typePool + * will detect that and discard the CacheInstance. + * + * At that time, a new CacheInstance with a fresh sequence will + * be created in its place. + */ + private static final class CacheInstance { + static final int CONCURRENCY_LEVEL = 8; + static final int LOADER_CAPACITY = 64; + static final int TYPE_CAPACITY = 64; + + static final long BOOTSTRAP_ID = Long.MIN_VALUE; + static final long START_ID = BOOTSTRAP_ID + 1; + static final long LIMIT_ID = Long.MAX_VALUE - 10; + + static final long EXHAUSTED_ID = LIMIT_ID; + + // Many things are package visible for testing purposes -- + // others to avoid creation of synthetic accessors + + /** + * Cache of recent loaderIds: guarantee is that no two loaders are given the same ID; however, a + * loader may be given more than one ID if it falls out the cache. + */ + final Cache loaderIdCache = + CacheBuilder.newBuilder() + .weakKeys() + .concurrencyLevel(CONCURRENCY_LEVEL) + .initialCapacity(LOADER_CAPACITY / 2) + .maximumSize(LOADER_CAPACITY) + .build(); + + /** + * Single shared Type.Resolution cache -- uses a composite key of loader ID & class name The + * initial capacity is set to the maximum capacity to avoid expansion overhead. + */ + final Cache sharedResolutionCache = + CacheBuilder.newBuilder() + .softValues() + .concurrencyLevel(CONCURRENCY_LEVEL) + .initialCapacity(TYPE_CAPACITY) + .maximumSize(TYPE_CAPACITY) + .build(); + + /** + * ID sequence for loaders -- BOOTSTRAP_ID is reserved -- starts higher at START_ID Sequence + * proceeds up until LIMIT_ID at which the sequence and this cacheInstance are considered to be + * exhausted + */ + final AtomicLong loaderIdSeq = new AtomicLong(START_ID); + + /** Fast path for bootstrap */ + final SharedResolutionCacheAdapter bootstrapCacheProvider = + new SharedResolutionCacheAdapter(BOOTSTRAP_ID, sharedResolutionCache); + + private final Callable provisionIdCallable = + new Callable() { + @Override + public final Long call() throws Exception { + return provisionId(); + } + }; + + final TypePool typePool( + final ClassFileLocator classFileLocator, final ClassLoader classLoader) { + if (classLoader == null) { + return createCachingTypePool(bootstrapCacheProvider, classFileLocator); + } + + Long existingId = loaderIdCache.getIfPresent(classLoader); + if (existingId != null) { + return createCachingTypePool(existingId, classFileLocator); + } + + if (exhaustedLoaderIdSeq()) { + return createNonCachingTypePool(classFileLocator); + } + + long provisionedId = 0; + try { + provisionedId = loaderIdCache.get(classLoader, this.provisionIdCallable); + } catch (ExecutionException e) { + log.error("unexpected exception", e); + + return createNonCachingTypePool(classFileLocator); + } + if (provisionedId == EXHAUSTED_ID) { + return createNonCachingTypePool(classFileLocator); + } else { + return createCachingTypePool(provisionedId, classFileLocator); + } + } + + final boolean exhaustedLoaderIdSeq() { + return (loaderIdSeq.get() >= LIMIT_ID); + } + + final long provisionId() { + do { + long curId = loaderIdSeq.get(); + if (curId >= LIMIT_ID) return EXHAUSTED_ID; + + long newId = curId + 1; + boolean acquired = loaderIdSeq.compareAndSet(curId, newId); + if (acquired) return newId; + } while (!Thread.currentThread().isInterrupted()); + + return EXHAUSTED_ID; + } + + private final TypePool createNonCachingTypePool(final ClassFileLocator classFileLocator) { + return new TypePool.Default.WithLazyResolution( + TypePool.CacheProvider.NoOp.INSTANCE, classFileLocator, TypePool.Default.ReaderMode.FAST); + } + + private final TypePool.CacheProvider createCacheProvider(final long loaderId) { + return new SharedResolutionCacheAdapter(loaderId, sharedResolutionCache); + } + + private final TypePool createCachingTypePool( + final long loaderId, final ClassFileLocator classFileLocator) { + return new TypePool.Default.WithLazyResolution( + createCacheProvider(loaderId), + classFileLocator, + TypePool.Default.ReaderMode.FAST); + } + + private final TypePool createCachingTypePool( + final TypePool.CacheProvider cacheProvider, final ClassFileLocator classFileLocator) { + return new TypePool.Default.WithLazyResolution( + cacheProvider, classFileLocator, TypePool.Default.ReaderMode.FAST); + } + + final long approximateSize() { + return sharedResolutionCache.size(); } } - private static class EvictingCacheProvider implements TypePool.CacheProvider { + /** + * TypeCacheKey is key for the sharedResolutionCache. It is a mix of a cacheId/loaderId & a type + * name. + */ + static final class TypeCacheKey { + private final long cacheId; + private final String name; - /** A map containing all cached resolutions by their names. */ - private final Cache cache; + private final int hashCode; - /** Creates a new simple cache. */ - private EvictingCacheProvider( - final Cleaner cleaner, final long expireDuration, final TimeUnit unit) { - cache = - CacheBuilder.newBuilder() - .initialCapacity(100) // Per classloader, so we want a small default. - .maximumSize(5000) - .softValues() - .expireAfterAccess(expireDuration, unit) - .build(); + TypeCacheKey(final long cacheId, final String name) { + this.cacheId = cacheId; + this.name = name; - /* - * The cache only does cleanup on occasional reads and writes. - * We want to ensure this happens more regularly, so we schedule a thread to do run cleanup manually. - */ - cleaner.scheduleCleaning(cache, CacheCleaner.CLEANER, expireDuration, unit); + hashCode = (int) (31 * cacheId) ^ name.hashCode(); } - private static EvictingCacheProvider withObjectType( - final Cleaner cleaner, final long expireDuration, final TimeUnit unit) { - final EvictingCacheProvider cacheProvider = - new EvictingCacheProvider(cleaner, expireDuration, unit); - cacheProvider.register( - Object.class.getName(), new TypePool.Resolution.Simple(TypeDescription.OBJECT)); - return cacheProvider; + @Override + public final int hashCode() { + return hashCode; + } + + @Override + public boolean equals(final Object obj) { + if (!(obj instanceof TypeCacheKey)) return false; + + TypeCacheKey that = (TypeCacheKey) obj; + return (cacheId == that.cacheId) && name.equals(that.name); + } + } + + static final class SharedResolutionCacheAdapter implements TypePool.CacheProvider { + private static final String OBJECT_NAME = "java.lang.Object"; + private static final TypePool.Resolution OBJECT_RESOLUTION = + new TypePool.Resolution.Simple(TypeDescription.OBJECT); + + private final long cacheId; + private final Cache sharedResolutionCache; + + SharedResolutionCacheAdapter( + final long cacheId, final Cache sharedResolutionCache) { + this.cacheId = cacheId; + this.sharedResolutionCache = sharedResolutionCache; } @Override public TypePool.Resolution find(final String name) { - return cache.getIfPresent(name); + TypePool.Resolution existingResolution = sharedResolutionCache.getIfPresent(new TypeCacheKey(cacheId, name)); + if ( existingResolution != null ) return existingResolution; + + if ( OBJECT_NAME.equals(name) ) { + return OBJECT_RESOLUTION; + } + + return null; } @Override public TypePool.Resolution register(final String name, final TypePool.Resolution resolution) { - try { - return cache.get(name, new ResolutionProvider(resolution)); - } catch (final ExecutionException e) { + if ( OBJECT_NAME.equals(name) ) { return resolution; } + + sharedResolutionCache.put(new TypeCacheKey(cacheId, name), resolution); + return resolution; } @Override public void clear() { - cache.invalidateAll(); - } - - public long size() { - return cache.size(); - } - - private static class CacheCleaner implements Cleaner.Adapter { - private static final CacheCleaner CLEANER = new CacheCleaner(); - - @Override - public void clean(final Cache target) { - target.cleanUp(); - } - } - - private static class ResolutionProvider implements Callable { - private final TypePool.Resolution value; - - private ResolutionProvider(final TypePool.Resolution value) { - this.value = value; - } - - @Override - public TypePool.Resolution call() { - return value; - } + // Allowing the high-level eviction policy make the clearing decisions } } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy new file mode 100644 index 0000000000..dcc26f8b32 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy @@ -0,0 +1,223 @@ +package datadog.trace.agent.tooling + +import datadog.trace.util.gc.GCUtils +import datadog.trace.util.test.DDSpecification +import net.bytebuddy.description.type.TypeDescription +import net.bytebuddy.dynamic.ClassFileLocator +import net.bytebuddy.pool.TypePool +import spock.lang.Timeout + +import java.lang.ref.WeakReference +import java.security.SecureClassLoader +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference + +import static datadog.trace.agent.tooling.AgentTooling.CLEANER + +@Timeout(5) +class CacheProviderTest extends DDSpecification { + def "key equivalence"() { + setup: + def key1 = new DDCachingPoolStrategy.TypeCacheKey(1, "foo") + def key2 = new DDCachingPoolStrategy.TypeCacheKey(1, "foo") + + expect: + key1.hashCode() == key2.hashCode() + key1.equals(key2) + } + + def "different loader - same name"() { + setup: + def key1 = new DDCachingPoolStrategy.TypeCacheKey(1, "foo") + def key2 = new DDCachingPoolStrategy.TypeCacheKey(2, "foo") + + expect: + // not strictly guaranteed, but important for performance + key1.hashCode() != key2.hashCode() + + !key1.equals(key2) + } + + def "same loader - different name"() { + setup: + def key1 = new DDCachingPoolStrategy.TypeCacheKey(1, "foo") + def key2 = new DDCachingPoolStrategy.TypeCacheKey(1, "foobar") + + expect: + // not strictly guaranteed, but important for performance + key1.hashCode() != key2.hashCode() + + !key1.equals(key2) + } + + def "test basic caching"() { + setup: + def cacheInstance = new DDCachingPoolStrategy.CacheInstance(); + + def cacheProvider = cacheInstance.createCacheProvider(1); + + when: + cacheProvider.register("foo", new TypePool.Resolution.Simple(TypeDescription.VOID)) + + then: + // not strictly guaranteed, but fine for this test + cacheProvider.find("foo") != null + cacheInstance.approximateSize() == 1 + } + + def "test ID equivalence"() { + setup: + def cacheInstance = new DDCachingPoolStrategy.CacheInstance(); + + def cacheProvider1A = cacheInstance.createCacheProvider(1); + def cacheProvider1B = cacheInstance.createCacheProvider(1); + + when: + cacheProvider1A.register("foo", newVoid()) + + then: + // not strictly guaranteed, but fine for this test + cacheProvider1A.find("foo") != null + cacheProvider1B.find("foo") != null + + cacheProvider1A.find("foo").is(cacheProvider1B.find("foo")) + cacheInstance.approximateSize() == 1 + } + + def "test ID separation"() { + setup: + def cacheInstance = new DDCachingPoolStrategy.CacheInstance(); + + def cacheProvider1 = cacheInstance.createCacheProvider(1); + def cacheProvider2 = cacheInstance.createCacheProvider(2); + + when: + cacheProvider1.register("foo", newVoid()) + cacheProvider2.register("foo", newVoid()) + + then: + // not strictly guaranteed, but fine for this test + cacheProvider1.find("foo") != null + cacheProvider2.find("foo") != null + + !cacheProvider1.find("foo").is(cacheProvider2.find("foo")) + cacheInstance.approximateSize() == 2 + } + + def "test loader ID assignment"() { + setup: + def cacheInstance = new DDCachingPoolStrategy.CacheInstance() + + def locator1 = newLocator() + def loader1 = newClassLoader() + + def locator2 = newLocator() + def loader2 = newClassLoader() + + when: + cacheInstance.typePool(locator1, loader1) + cacheInstance.typePool(locator2, loader2) + + then: + def loaderId1 = cacheInstance.loaderIdCache.getIfPresent(loader1) + def loaderId2 = cacheInstance.loaderIdCache.getIfPresent(loader2) + + // both were assigned an ID -- technically these can fall out of the ID cache + loaderId1 != null + loaderId2 != null + + // both IDs are not the BOOTSTRAP_ID + loaderId1 != DDCachingPoolStrategy.CacheInstance.BOOTSTRAP_ID + loaderId2 != DDCachingPoolStrategy.CacheInstance.BOOTSTRAP_ID + + // class loaders don't share an ID + cacheInstance.loaderIdCache.getIfPresent(loader1) != cacheInstance.loaderIdCache.getIfPresent(loader2) + } + + def "test loader ID exhaustion"() { + setup: + def cacheInstance = new DDCachingPoolStrategy.CacheInstance() + + when: + cacheInstance.loaderIdSeq.set(DDCachingPoolStrategy.CacheInstance.LIMIT_ID - 2) + + then: + cacheInstance.provisionId() != DDCachingPoolStrategy.CacheInstance.EXHAUSTED_ID + + then: + // once exhausted provisioning -- stays exhausted + cacheInstance.provisionId() == DDCachingPoolStrategy.CacheInstance.EXHAUSTED_ID + cacheInstance.exhaustedLoaderIdSeq() + cacheInstance.provisionId() == DDCachingPoolStrategy.CacheInstance.EXHAUSTED_ID + cacheInstance.exhaustedLoaderIdSeq() + cacheInstance.provisionId() == DDCachingPoolStrategy.CacheInstance.EXHAUSTED_ID + cacheInstance.exhaustedLoaderIdSeq() + } + + def "test exhaustion cacheInstance switch"() { + setup: + def cachingStrat = new DDCachingPoolStrategy() + def origCacheInstance = cachingStrat.cacheInstance + + cachingStrat.cacheInstance.loaderIdSeq.set(DDCachingPoolStrategy.CacheInstance.LIMIT_ID) + + when: + cachingStrat.typePool(newLocator(), newClassLoader()) + + then: + cachingStrat.cacheInstance != origCacheInstance + } + + def "test cacheInstance capacity"() { + setup: + def cacheInstance = new DDCachingPoolStrategy.CacheInstance() + def capacity = DDCachingPoolStrategy.CacheInstance.TYPE_CAPACITY + + def cacheProvider1 = cacheInstance.createCacheProvider(1); + def cacheProvider2 = cacheInstance.createCacheProvider(2); + + def id = 0 + + when: + (capacity / 2).times { + id += 1 + cacheProvider1.register("foo${id}", newVoid()) + cacheProvider2.register("foo${id}", newVoid()) + } + + then: + // cache will start to proactively free slots & size calc is approximate + cacheInstance.approximateSize() > capacity - 4 + + when: + 10.times { + id += 1 + cacheProvider1.register("foo${id}", newVoid()) + cacheProvider2.register("foo${id}", newVoid()) + } + + then: + // cache will start to proactively free slots & size calc is approximate + cacheInstance.approximateSize() > capacity - 4 + } + + static def newVoid() { + return new TypePool.Resolution.Simple(TypeDescription.VOID) + } + + static def newClassLoader() { + return new SecureClassLoader(null) {}; + } + + static def newLocator() { + return new ClassFileLocator() { + @Override + ClassFileLocator.Resolution locate(String name) throws IOException { + return null + } + + @Override + void close() throws IOException {} + } + } +} diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/EvictingCacheProviderTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/EvictingCacheProviderTest.groovy deleted file mode 100644 index 3de7cc3140..0000000000 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/EvictingCacheProviderTest.groovy +++ /dev/null @@ -1,102 +0,0 @@ -package datadog.trace.agent.tooling - -import datadog.trace.util.gc.GCUtils -import datadog.trace.util.test.DDSpecification -import net.bytebuddy.description.type.TypeDescription -import net.bytebuddy.pool.TypePool -import spock.lang.Timeout - -import java.lang.ref.WeakReference -import java.util.concurrent.TimeUnit -import java.util.concurrent.atomic.AtomicReference - -import static datadog.trace.agent.tooling.AgentTooling.CLEANER - -@Timeout(5) -class EvictingCacheProviderTest extends DDSpecification { - - def "test provider"() { - setup: - def provider = new DDCachingPoolStrategy.EvictingCacheProvider(CLEANER, 2, TimeUnit.MINUTES) - - expect: - provider.size() == 0 - provider.find(className) == null - - when: - provider.register(className, new TypePool.Resolution.Simple(TypeDescription.VOID)) - - then: - provider.size() == 1 - provider.find(className) == new TypePool.Resolution.Simple(TypeDescription.VOID) - - when: - provider.clear() - - then: - provider.size() == 0 - provider.find(className) == null - - where: - className = "SomeClass" - } - - def "test timeout eviction"() { - setup: - def provider = new DDCachingPoolStrategy.EvictingCacheProvider(CLEANER, timeout, TimeUnit.MILLISECONDS) - def resolutionRef = new AtomicReference(new TypePool.Resolution.Simple(TypeDescription.VOID)) - def weakRef = new WeakReference(resolutionRef.get()) - - when: - def lastAccess = System.nanoTime() - provider.register(className, resolutionRef.get()) - - then: - // Ensure continued access prevents expiration. - for (int i = 0; i < timeout + 10; i++) { - assert TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - lastAccess) < timeout: "test took too long on " + i - assert provider.find(className) != null - assert provider.size() == 1 - lastAccess = System.nanoTime() - Thread.sleep(1) - } - - when: - Thread.sleep(timeout) - - then: - provider.find(className) == null - - when: - provider.register(className, resolutionRef.get()) - resolutionRef.set(null) - GCUtils.awaitGC(weakRef) - - then: - // Verify properly GC'd - provider.find(className) == null - weakRef.get() == null - - where: - className = "SomeClass" - timeout = 500 // Takes about 50 ms locally, adding an order of magnitude for CI. - } - - def "test size limit"() { - setup: - def provider = new DDCachingPoolStrategy.EvictingCacheProvider(CLEANER, 2, TimeUnit.MINUTES) - def typeDef = new TypePool.Resolution.Simple(TypeDescription.VOID) - for (int i = 0; i < 10000; i++) { - provider.register("ClassName$i", typeDef) - } - - expect: - provider.size() == 5000 - - when: - provider.clear() - - then: - provider.size() == 0 - } -} From 984d77e44ce0de96df6b7fe00cc7210deb360c94 Mon Sep 17 00:00:00 2001 From: dougqh Date: Thu, 23 Jan 2020 11:15:39 -0500 Subject: [PATCH 03/11] googleJavaFormat & codeNarc --- .../agent/tooling/DDCachingPoolStrategy.java | 25 +++++++------- .../agent/tooling/CacheProviderTest.groovy | 34 ++++++++----------- 2 files changed, 26 insertions(+), 33 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index a1ad033970..657141e97c 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -29,17 +29,17 @@ import net.bytebuddy.pool.TypePool; * that combines loader ID & name * * - *

This design was chosen to create a single limited size cache that can be adjusted - * for the entire application -- without having to create a large number of WeakReference objects. + *

This design was chosen to create a single limited size cache that can be adjusted for the + * entire application -- without having to create a large number of WeakReference objects. * *

The ID assignment mostly assigns a single ID to each ClassLoader, but the maximumSize * restriction means that an evicted ClassLoader could be assigned another ID later on. * - *

For the validity of the cache, the important part is that ID assignment guarantees that - * no two ClassLoaders share the same ID. + *

For the validity of the cache, the important part is that ID assignment guarantees that no two + * ClassLoaders share the same ID. * - *

NOTE: As an additional safe-guard, a new CacheInstance can be created if the original loader ID - * sequence is exhausted. + *

NOTE: As an additional safe-guard, a new CacheInstance can be created if the original loader + * ID sequence is exhausted. */ @Slf4j public class DDCachingPoolStrategy implements PoolStrategy { @@ -192,9 +192,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { private final TypePool createCachingTypePool( final long loaderId, final ClassFileLocator classFileLocator) { return new TypePool.Default.WithLazyResolution( - createCacheProvider(loaderId), - classFileLocator, - TypePool.Default.ReaderMode.FAST); + createCacheProvider(loaderId), classFileLocator, TypePool.Default.ReaderMode.FAST); } private final TypePool createCachingTypePool( @@ -255,10 +253,11 @@ public class DDCachingPoolStrategy implements PoolStrategy { @Override public TypePool.Resolution find(final String name) { - TypePool.Resolution existingResolution = sharedResolutionCache.getIfPresent(new TypeCacheKey(cacheId, name)); - if ( existingResolution != null ) return existingResolution; + TypePool.Resolution existingResolution = + sharedResolutionCache.getIfPresent(new TypeCacheKey(cacheId, name)); + if (existingResolution != null) return existingResolution; - if ( OBJECT_NAME.equals(name) ) { + if (OBJECT_NAME.equals(name)) { return OBJECT_RESOLUTION; } @@ -267,7 +266,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { @Override public TypePool.Resolution register(final String name, final TypePool.Resolution resolution) { - if ( OBJECT_NAME.equals(name) ) { + if (OBJECT_NAME.equals(name)) { return resolution; } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy index dcc26f8b32..959313c545 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy @@ -1,18 +1,12 @@ package datadog.trace.agent.tooling -import datadog.trace.util.gc.GCUtils import datadog.trace.util.test.DDSpecification import net.bytebuddy.description.type.TypeDescription import net.bytebuddy.dynamic.ClassFileLocator import net.bytebuddy.pool.TypePool import spock.lang.Timeout -import java.lang.ref.WeakReference import java.security.SecureClassLoader -import java.util.concurrent.TimeUnit -import java.util.concurrent.atomic.AtomicReference - -import static datadog.trace.agent.tooling.AgentTooling.CLEANER @Timeout(5) class CacheProviderTest extends DDSpecification { @@ -52,9 +46,9 @@ class CacheProviderTest extends DDSpecification { def "test basic caching"() { setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance(); + def cacheInstance = new DDCachingPoolStrategy.CacheInstance() - def cacheProvider = cacheInstance.createCacheProvider(1); + def cacheProvider = cacheInstance.createCacheProvider(1) when: cacheProvider.register("foo", new TypePool.Resolution.Simple(TypeDescription.VOID)) @@ -67,10 +61,10 @@ class CacheProviderTest extends DDSpecification { def "test ID equivalence"() { setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance(); + def cacheInstance = new DDCachingPoolStrategy.CacheInstance() - def cacheProvider1A = cacheInstance.createCacheProvider(1); - def cacheProvider1B = cacheInstance.createCacheProvider(1); + def cacheProvider1A = cacheInstance.createCacheProvider(1) + def cacheProvider1B = cacheInstance.createCacheProvider(1) when: cacheProvider1A.register("foo", newVoid()) @@ -86,10 +80,10 @@ class CacheProviderTest extends DDSpecification { def "test ID separation"() { setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance(); + def cacheInstance = new DDCachingPoolStrategy.CacheInstance() - def cacheProvider1 = cacheInstance.createCacheProvider(1); - def cacheProvider2 = cacheInstance.createCacheProvider(2); + def cacheProvider1 = cacheInstance.createCacheProvider(1) + def cacheProvider2 = cacheInstance.createCacheProvider(2) when: cacheProvider1.register("foo", newVoid()) @@ -173,8 +167,8 @@ class CacheProviderTest extends DDSpecification { def cacheInstance = new DDCachingPoolStrategy.CacheInstance() def capacity = DDCachingPoolStrategy.CacheInstance.TYPE_CAPACITY - def cacheProvider1 = cacheInstance.createCacheProvider(1); - def cacheProvider2 = cacheInstance.createCacheProvider(2); + def cacheProvider1 = cacheInstance.createCacheProvider(1) + def cacheProvider2 = cacheInstance.createCacheProvider(2) def id = 0 @@ -201,15 +195,15 @@ class CacheProviderTest extends DDSpecification { cacheInstance.approximateSize() > capacity - 4 } - static def newVoid() { + static newVoid() { return new TypePool.Resolution.Simple(TypeDescription.VOID) } - static def newClassLoader() { - return new SecureClassLoader(null) {}; + static newClassLoader() { + return new SecureClassLoader(null) {} } - static def newLocator() { + static newLocator() { return new ClassFileLocator() { @Override ClassFileLocator.Resolution locate(String name) throws IOException { From cf877f67e5fa67656857c59395c3c1fbde0ef0a6 Mon Sep 17 00:00:00 2001 From: dougqh Date: Mon, 27 Jan 2020 09:47:36 -0500 Subject: [PATCH 04/11] Working around muzzle quirk Muzzle doesn't like creation of SecureClassLoader-s, so switching to a URLClassLoader for my placeholder loader in tests --- .../groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy index 959313c545..0d7e687850 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy @@ -200,7 +200,7 @@ class CacheProviderTest extends DDSpecification { } static newClassLoader() { - return new SecureClassLoader(null) {} + return new URLClassLoader([] as URL[], (ClassLoader)null); } static newLocator() { From fb871611b5b827d88a8400c96b6f31a071c2abbb Mon Sep 17 00:00:00 2001 From: dougqh Date: Mon, 27 Jan 2020 12:03:44 -0500 Subject: [PATCH 05/11] Replacing ID generation with WeakReference reuse First pass at replacing ID generation with WeakReference reuse In this first version, the Cache was replaced with Cache>. The core cache is still of Cache and TypeCacheKey logically remains a composite key of ClassLoader, class name. The removal of ID assignment means ID exhaustion is no longer na issue, so there's never a need to rebuild the cache. For that reason, CacheInstance has removed and the core caching logic has been moved into DDCachingPoolStrategy. While TypeCacheKey remains conceptually the same, the internals have changed somewhat. The TypeCacheKey now has 3 core fields... - loaderHash - loadeRef - class name Since loader refs are recycled, the fast path for key equivalence can use reference equivalence of the reference objects. This change ripples through the CacheProvider-s which also have to store loaderHash and loaderRef. It may be worth going a step further and switching to a Cache as well. That still avoid the creation of many WeakReference-s, since the underlying CacheProvider will hold a canonical WeakReference per ClassLoader. --- .../agent/tooling/DDCachingPoolStrategy.java | 291 ++++++++---------- .../agent/tooling/CacheProviderTest.groovy | 199 ++++++------ 2 files changed, 228 insertions(+), 262 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index 657141e97c..1e47032a7a 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -4,9 +4,9 @@ import static net.bytebuddy.agent.builder.AgentBuilder.PoolStrategy; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; -import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.atomic.AtomicLong; + +import java.lang.ref.WeakReference; + import lombok.extern.slf4j.Slf4j; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.ClassFileLocator; @@ -43,167 +43,94 @@ import net.bytebuddy.pool.TypePool; */ @Slf4j public class DDCachingPoolStrategy implements PoolStrategy { + // Many things are package visible for testing purposes -- + // others to avoid creation of synthetic accessors + + static final int CONCURRENCY_LEVEL = 8; + static final int LOADER_CAPACITY = 64; + static final int TYPE_CAPACITY = 64; + + static final int BOOTSTRAP_HASH = 0; + /** - * Most of the logic exists in CacheInstance This volatile + exhaustion checking is defense - * against loader ID exhaustion + * Cache of recent ClassLoader WeakReferences; used to... + *

*/ - volatile CacheInstance cacheInstance = new CacheInstance(); + final Cache> loaderRefCache = + CacheBuilder.newBuilder() + .weakKeys() + .concurrencyLevel(CONCURRENCY_LEVEL) + .initialCapacity(LOADER_CAPACITY / 2) + .maximumSize(LOADER_CAPACITY) + .build(); - @Override - public TypePool typePool(final ClassFileLocator classFileLocator, final ClassLoader classLoader) { - CacheInstance cacheInstance = this.cacheInstance; - - TypePool typePool = cacheInstance.typePool(classFileLocator, classLoader); - if (cacheInstance.exhaustedLoaderIdSeq()) { - // If the loader ID sequence is exhausted, drop the prior cache & start over - // The ID space is so large that this shouldn't occur - log.error("cacheInstance exhausted - rebuilding cache"); - - this.cacheInstance = new CacheInstance(); - } - return typePool; - } - - /* - * CacheInstance embodies the core of the cache. In general, we only - * expect a single CacheInstance object to ever be created. - * - * However, CacheInstance does provide an extra layer of protection - * against loaderIdSeq exhaustion. If ever the loaderIdSeq of - * CacheInstance is exhausted, then DDCachingPoolStrategy.typePool - * will detect that and discard the CacheInstance. - * - * At that time, a new CacheInstance with a fresh sequence will - * be created in its place. + /** + * Single shared Type.Resolution cache -- uses a composite key -- + * conceptually of loader & name */ - private static final class CacheInstance { - static final int CONCURRENCY_LEVEL = 8; - static final int LOADER_CAPACITY = 64; - static final int TYPE_CAPACITY = 64; - - static final long BOOTSTRAP_ID = Long.MIN_VALUE; - static final long START_ID = BOOTSTRAP_ID + 1; - static final long LIMIT_ID = Long.MAX_VALUE - 10; - - static final long EXHAUSTED_ID = LIMIT_ID; - - // Many things are package visible for testing purposes -- - // others to avoid creation of synthetic accessors - - /** - * Cache of recent loaderIds: guarantee is that no two loaders are given the same ID; however, a - * loader may be given more than one ID if it falls out the cache. - */ - final Cache loaderIdCache = - CacheBuilder.newBuilder() - .weakKeys() - .concurrencyLevel(CONCURRENCY_LEVEL) - .initialCapacity(LOADER_CAPACITY / 2) - .maximumSize(LOADER_CAPACITY) - .build(); - - /** - * Single shared Type.Resolution cache -- uses a composite key of loader ID & class name The - * initial capacity is set to the maximum capacity to avoid expansion overhead. - */ - final Cache sharedResolutionCache = - CacheBuilder.newBuilder() - .softValues() - .concurrencyLevel(CONCURRENCY_LEVEL) - .initialCapacity(TYPE_CAPACITY) - .maximumSize(TYPE_CAPACITY) - .build(); - - /** - * ID sequence for loaders -- BOOTSTRAP_ID is reserved -- starts higher at START_ID Sequence - * proceeds up until LIMIT_ID at which the sequence and this cacheInstance are considered to be - * exhausted - */ - final AtomicLong loaderIdSeq = new AtomicLong(START_ID); + final Cache sharedResolutionCache = + CacheBuilder.newBuilder() + .softValues() + .concurrencyLevel(CONCURRENCY_LEVEL) + .initialCapacity(TYPE_CAPACITY) + .maximumSize(TYPE_CAPACITY) + .build(); /** Fast path for bootstrap */ final SharedResolutionCacheAdapter bootstrapCacheProvider = - new SharedResolutionCacheAdapter(BOOTSTRAP_ID, sharedResolutionCache); + new SharedResolutionCacheAdapter(BOOTSTRAP_HASH, null, sharedResolutionCache); - private final Callable provisionIdCallable = - new Callable() { - @Override - public final Long call() throws Exception { - return provisionId(); - } - }; - - final TypePool typePool( - final ClassFileLocator classFileLocator, final ClassLoader classLoader) { - if (classLoader == null) { - return createCachingTypePool(bootstrapCacheProvider, classFileLocator); - } - - Long existingId = loaderIdCache.getIfPresent(classLoader); - if (existingId != null) { - return createCachingTypePool(existingId, classFileLocator); - } - - if (exhaustedLoaderIdSeq()) { - return createNonCachingTypePool(classFileLocator); - } - - long provisionedId = 0; - try { - provisionedId = loaderIdCache.get(classLoader, this.provisionIdCallable); - } catch (ExecutionException e) { - log.error("unexpected exception", e); - - return createNonCachingTypePool(classFileLocator); - } - if (provisionedId == EXHAUSTED_ID) { - return createNonCachingTypePool(classFileLocator); - } else { - return createCachingTypePool(provisionedId, classFileLocator); - } + @Override + public final TypePool typePool( + final ClassFileLocator classFileLocator, final ClassLoader classLoader) { + if (classLoader == null) { + return createCachingTypePool(bootstrapCacheProvider, classFileLocator); } - final boolean exhaustedLoaderIdSeq() { - return (loaderIdSeq.get() >= LIMIT_ID); + WeakReference loaderRef = loaderRefCache.getIfPresent(classLoader); + + if ( loaderRef == null ) { + loaderRef = new WeakReference<>(classLoader); + loaderRefCache.put(classLoader, loaderRef); } - final long provisionId() { - do { - long curId = loaderIdSeq.get(); - if (curId >= LIMIT_ID) return EXHAUSTED_ID; + int loaderHash = classLoader.hashCode(); + return createCachingTypePool(loaderHash, loaderRef, classFileLocator); + } - long newId = curId + 1; - boolean acquired = loaderIdSeq.compareAndSet(curId, newId); - if (acquired) return newId; - } while (!Thread.currentThread().isInterrupted()); + private final TypePool createNonCachingTypePool(final ClassFileLocator classFileLocator) { + return new TypePool.Default.WithLazyResolution( + TypePool.CacheProvider.NoOp.INSTANCE, classFileLocator, TypePool.Default.ReaderMode.FAST); + } - return EXHAUSTED_ID; - } + private final TypePool.CacheProvider createCacheProvider( + final int loaderHash, + final WeakReference loaderRef) + { + return new SharedResolutionCacheAdapter(loaderHash, loaderRef, sharedResolutionCache); + } - private final TypePool createNonCachingTypePool(final ClassFileLocator classFileLocator) { - return new TypePool.Default.WithLazyResolution( - TypePool.CacheProvider.NoOp.INSTANCE, classFileLocator, TypePool.Default.ReaderMode.FAST); - } + private final TypePool createCachingTypePool( + final int loaderHash, + final WeakReference loaderRef, + final ClassFileLocator classFileLocator) { + return new TypePool.Default.WithLazyResolution( + createCacheProvider(loaderHash, loaderRef), + classFileLocator, + TypePool.Default.ReaderMode.FAST); + } - private final TypePool.CacheProvider createCacheProvider(final long loaderId) { - return new SharedResolutionCacheAdapter(loaderId, sharedResolutionCache); - } + private final TypePool createCachingTypePool( + final TypePool.CacheProvider cacheProvider, final ClassFileLocator classFileLocator) { + return new TypePool.Default.WithLazyResolution( + cacheProvider, classFileLocator, TypePool.Default.ReaderMode.FAST); + } - private final TypePool createCachingTypePool( - final long loaderId, final ClassFileLocator classFileLocator) { - return new TypePool.Default.WithLazyResolution( - createCacheProvider(loaderId), classFileLocator, TypePool.Default.ReaderMode.FAST); - } - - private final TypePool createCachingTypePool( - final TypePool.CacheProvider cacheProvider, final ClassFileLocator classFileLocator) { - return new TypePool.Default.WithLazyResolution( - cacheProvider, classFileLocator, TypePool.Default.ReaderMode.FAST); - } - - final long approximateSize() { - return sharedResolutionCache.size(); - } + final long approximateSize() { + return sharedResolutionCache.size(); } /** @@ -211,16 +138,22 @@ public class DDCachingPoolStrategy implements PoolStrategy { * name. */ static final class TypeCacheKey { - private final long cacheId; - private final String name; + private final int loaderHash; + private final WeakReference loaderRef; + private final String className; private final int hashCode; - TypeCacheKey(final long cacheId, final String name) { - this.cacheId = cacheId; - this.name = name; + TypeCacheKey( + final int loaderHash, + final WeakReference loaderRef, + final String className) + { + this.loaderHash = loaderHash; + this.loaderRef = loaderRef; + this.className = className; - hashCode = (int) (31 * cacheId) ^ name.hashCode(); + hashCode = (int) (31 * this.loaderHash) ^ className.hashCode(); } @Override @@ -230,10 +163,34 @@ public class DDCachingPoolStrategy implements PoolStrategy { @Override public boolean equals(final Object obj) { - if (!(obj instanceof TypeCacheKey)) return false; + if ( !(obj instanceof TypeCacheKey) ) return false; - TypeCacheKey that = (TypeCacheKey) obj; - return (cacheId == that.cacheId) && name.equals(that.name); + TypeCacheKey that = (TypeCacheKey)obj; + + if ( loaderHash != that.loaderHash ) return false; + + // Fastpath loaderRef equivalence -- works because of WeakReference cache used + // Also covers the bootstrap null loaderRef case + if ( loaderRef == that.loaderRef ) { + // still need to check name + return className.equals(that.className); + } else if ( className.equals(that.className) ) { + // need to perform a deeper loader check -- requires calling Reference.get + // which can strengthened the Reference, so deliberately done last + + // If either reference has gone null, they aren't considered equivalent + // Technically, this is a bit of violation of equals semantics, since + // two equivalent references can be not equivalent. + ClassLoader thisLoader = loaderRef.get(); + if ( thisLoader == null ) return false; + + ClassLoader thatLoader = that.loaderRef.get(); + if ( thatLoader == null ) return false; + + return (thisLoader == thatLoader); + } else { + return false; + } } } @@ -242,22 +199,26 @@ public class DDCachingPoolStrategy implements PoolStrategy { private static final TypePool.Resolution OBJECT_RESOLUTION = new TypePool.Resolution.Simple(TypeDescription.OBJECT); - private final long cacheId; + private final int loaderHash; + private final WeakReference loaderRef; private final Cache sharedResolutionCache; SharedResolutionCacheAdapter( - final long cacheId, final Cache sharedResolutionCache) { - this.cacheId = cacheId; + final int loaderHash, + final WeakReference loaderRef, + final Cache sharedResolutionCache) { + this.loaderHash = loaderHash; + this.loaderRef = loaderRef; this.sharedResolutionCache = sharedResolutionCache; } @Override - public TypePool.Resolution find(final String name) { + public TypePool.Resolution find(final String className) { TypePool.Resolution existingResolution = - sharedResolutionCache.getIfPresent(new TypeCacheKey(cacheId, name)); + sharedResolutionCache.getIfPresent(new TypeCacheKey(loaderHash, loaderRef, className)); if (existingResolution != null) return existingResolution; - if (OBJECT_NAME.equals(name)) { + if (OBJECT_NAME.equals(className)) { return OBJECT_RESOLUTION; } @@ -265,12 +226,12 @@ public class DDCachingPoolStrategy implements PoolStrategy { } @Override - public TypePool.Resolution register(final String name, final TypePool.Resolution resolution) { - if (OBJECT_NAME.equals(name)) { + public TypePool.Resolution register(final String className, final TypePool.Resolution resolution) { + if (OBJECT_NAME.equals(className)) { return resolution; } - sharedResolutionCache.put(new TypeCacheKey(cacheId, name), resolution); + sharedResolutionCache.put(new TypeCacheKey(loaderHash, loaderRef, className), resolution); return resolution; } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy index 0d7e687850..4ce1bc5ba5 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy @@ -6,49 +6,97 @@ import net.bytebuddy.dynamic.ClassFileLocator import net.bytebuddy.pool.TypePool import spock.lang.Timeout +import java.lang.ref.WeakReference import java.security.SecureClassLoader @Timeout(5) class CacheProviderTest extends DDSpecification { - def "key equivalence"() { - setup: - def key1 = new DDCachingPoolStrategy.TypeCacheKey(1, "foo") - def key2 = new DDCachingPoolStrategy.TypeCacheKey(1, "foo") + def "key bootstrap equivalence"() { + def loader = null + def loaderHash = DDCachingPoolStrategy.BOOTSTRAP_HASH + def loaderRef = null + + def key1 = new DDCachingPoolStrategy.TypeCacheKey(loaderHash, loaderRef, "foo") + def key2 = new DDCachingPoolStrategy.TypeCacheKey(loaderHash, loaderRef, "foo") expect: key1.hashCode() == key2.hashCode() key1.equals(key2) } - def "different loader - same name"() { + def "key same ref equivalence"() { setup: - def key1 = new DDCachingPoolStrategy.TypeCacheKey(1, "foo") - def key2 = new DDCachingPoolStrategy.TypeCacheKey(2, "foo") + def loader = newClassLoader() + def loaderHash = loader.hashCode() + def loaderRef = new WeakReference(loader) + + def key1 = new DDCachingPoolStrategy.TypeCacheKey(loaderHash, loaderRef, "foo") + def key2 = new DDCachingPoolStrategy.TypeCacheKey(loaderHash, loaderRef, "foo") expect: - // not strictly guaranteed, but important for performance - key1.hashCode() != key2.hashCode() - - !key1.equals(key2) + key1.hashCode() == key2.hashCode() + key1.equals(key2) } - def "same loader - different name"() { + def "key different ref equivalence"() { setup: - def key1 = new DDCachingPoolStrategy.TypeCacheKey(1, "foo") - def key2 = new DDCachingPoolStrategy.TypeCacheKey(1, "foobar") + def loader = newClassLoader() + def loaderHash = loader.hashCode() + def loaderRef1 = new WeakReference(loader) + def loaderRef2 = new WeakReference(loader) + + def key1 = new DDCachingPoolStrategy.TypeCacheKey(loaderHash, loaderRef1, "foo") + def key2 = new DDCachingPoolStrategy.TypeCacheKey(loaderHash, loaderRef2, "foo") expect: - // not strictly guaranteed, but important for performance - key1.hashCode() != key2.hashCode() + loaderRef1 != loaderRef2 - !key1.equals(key2) + key1.hashCode() == key2.hashCode() + key1.equals(key2) + } + + def "key mismatch -- same loader - diff name"() { + setup: + def loader = newClassLoader() + def loaderHash = loader.hashCode() + def loaderRef = new WeakReference(loader) + def fooKey = new DDCachingPoolStrategy.TypeCacheKey(loaderHash, loaderRef, "foo") + def barKey = new DDCachingPoolStrategy.TypeCacheKey(loaderHash, loaderRef, "bar") + + expect: + // not strictly guaranteed -- but important for performance + fooKey.hashCode() != barKey.hashCode() + !fooKey.equals(barKey) + } + + def "key mismatch -- same name - diff loader"() { + setup: + def loader1 = newClassLoader() + def loader1Hash = loader1.hashCode() + def loaderRef1 = new WeakReference(loader1) + + def loader2 = newClassLoader() + def loader2Hash = loader2.hashCode() + def loaderRef2 = new WeakReference(loader2) + + def fooKey1 = new DDCachingPoolStrategy.TypeCacheKey(loader1Hash, loaderRef1, "foo") + def fooKey2 = new DDCachingPoolStrategy.TypeCacheKey(loader2Hash, loaderRef2, "foo") + + expect: + // not strictly guaranteed -- but important for performance + fooKey1.hashCode() != fooKey2.hashCode() + !fooKey1.equals(fooKey2) } def "test basic caching"() { setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance() + def poolStrat = new DDCachingPoolStrategy() - def cacheProvider = cacheInstance.createCacheProvider(1) + def loader = newClassLoader() + def loaderHash = loader.hashCode() + def loaderRef = new WeakReference(loader) + + def cacheProvider = poolStrat.createCacheProvider(loaderHash, loaderRef) when: cacheProvider.register("foo", new TypePool.Resolution.Simple(TypeDescription.VOID)) @@ -56,15 +104,20 @@ class CacheProviderTest extends DDSpecification { then: // not strictly guaranteed, but fine for this test cacheProvider.find("foo") != null - cacheInstance.approximateSize() == 1 + poolStrat.approximateSize() == 1 } - def "test ID equivalence"() { + def "test loader equivalence"() { setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance() + def poolStrat = new DDCachingPoolStrategy() - def cacheProvider1A = cacheInstance.createCacheProvider(1) - def cacheProvider1B = cacheInstance.createCacheProvider(1) + def loader1 = newClassLoader() + def loaderHash1 = loader1.hashCode() + def loaderRef1A = new WeakReference(loader1) + def loaderRef1B = new WeakReference(loader1) + + def cacheProvider1A = poolStrat.createCacheProvider(loaderHash1, loaderRef1A) + def cacheProvider1B = poolStrat.createCacheProvider(loaderHash1, loaderRef1B) when: cacheProvider1A.register("foo", newVoid()) @@ -75,15 +128,23 @@ class CacheProviderTest extends DDSpecification { cacheProvider1B.find("foo") != null cacheProvider1A.find("foo").is(cacheProvider1B.find("foo")) - cacheInstance.approximateSize() == 1 + poolStrat.approximateSize() == 1 } - def "test ID separation"() { + def "test loader separation"() { setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance() + def poolStrat = new DDCachingPoolStrategy() - def cacheProvider1 = cacheInstance.createCacheProvider(1) - def cacheProvider2 = cacheInstance.createCacheProvider(2) + def loader1 = newClassLoader() + def loaderHash1 = loader1.hashCode() + def loaderRef1 = new WeakReference(loader1) + + def loader2 = newClassLoader() + def loaderHash2 = loader2.hashCode() + def loaderRef2 = new WeakReference(loader2) + + def cacheProvider1 = poolStrat.createCacheProvider(loaderHash1, loaderRef1) + def cacheProvider2 = poolStrat.createCacheProvider(loaderHash2, loaderRef2) when: cacheProvider1.register("foo", newVoid()) @@ -95,80 +156,24 @@ class CacheProviderTest extends DDSpecification { cacheProvider2.find("foo") != null !cacheProvider1.find("foo").is(cacheProvider2.find("foo")) - cacheInstance.approximateSize() == 2 + poolStrat.approximateSize() == 2 } - def "test loader ID assignment"() { + def "test capacity"() { setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance() + def poolStrat = new DDCachingPoolStrategy() + def capacity = DDCachingPoolStrategy.TYPE_CAPACITY - def locator1 = newLocator() def loader1 = newClassLoader() + def loaderHash1 = loader1.hashCode() + def loaderRef1 = new WeakReference(loader1) - def locator2 = newLocator() def loader2 = newClassLoader() + def loaderHash2 = loader2.hashCode() + def loaderRef2 = new WeakReference(loader2) - when: - cacheInstance.typePool(locator1, loader1) - cacheInstance.typePool(locator2, loader2) - - then: - def loaderId1 = cacheInstance.loaderIdCache.getIfPresent(loader1) - def loaderId2 = cacheInstance.loaderIdCache.getIfPresent(loader2) - - // both were assigned an ID -- technically these can fall out of the ID cache - loaderId1 != null - loaderId2 != null - - // both IDs are not the BOOTSTRAP_ID - loaderId1 != DDCachingPoolStrategy.CacheInstance.BOOTSTRAP_ID - loaderId2 != DDCachingPoolStrategy.CacheInstance.BOOTSTRAP_ID - - // class loaders don't share an ID - cacheInstance.loaderIdCache.getIfPresent(loader1) != cacheInstance.loaderIdCache.getIfPresent(loader2) - } - - def "test loader ID exhaustion"() { - setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance() - - when: - cacheInstance.loaderIdSeq.set(DDCachingPoolStrategy.CacheInstance.LIMIT_ID - 2) - - then: - cacheInstance.provisionId() != DDCachingPoolStrategy.CacheInstance.EXHAUSTED_ID - - then: - // once exhausted provisioning -- stays exhausted - cacheInstance.provisionId() == DDCachingPoolStrategy.CacheInstance.EXHAUSTED_ID - cacheInstance.exhaustedLoaderIdSeq() - cacheInstance.provisionId() == DDCachingPoolStrategy.CacheInstance.EXHAUSTED_ID - cacheInstance.exhaustedLoaderIdSeq() - cacheInstance.provisionId() == DDCachingPoolStrategy.CacheInstance.EXHAUSTED_ID - cacheInstance.exhaustedLoaderIdSeq() - } - - def "test exhaustion cacheInstance switch"() { - setup: - def cachingStrat = new DDCachingPoolStrategy() - def origCacheInstance = cachingStrat.cacheInstance - - cachingStrat.cacheInstance.loaderIdSeq.set(DDCachingPoolStrategy.CacheInstance.LIMIT_ID) - - when: - cachingStrat.typePool(newLocator(), newClassLoader()) - - then: - cachingStrat.cacheInstance != origCacheInstance - } - - def "test cacheInstance capacity"() { - setup: - def cacheInstance = new DDCachingPoolStrategy.CacheInstance() - def capacity = DDCachingPoolStrategy.CacheInstance.TYPE_CAPACITY - - def cacheProvider1 = cacheInstance.createCacheProvider(1) - def cacheProvider2 = cacheInstance.createCacheProvider(2) + def cacheProvider1 = poolStrat.createCacheProvider(loaderHash1, loaderRef1) + def cacheProvider2 = poolStrat.createCacheProvider(loaderHash2, loaderRef2) def id = 0 @@ -181,7 +186,7 @@ class CacheProviderTest extends DDSpecification { then: // cache will start to proactively free slots & size calc is approximate - cacheInstance.approximateSize() > capacity - 4 + poolStrat.approximateSize() > capacity - 4 when: 10.times { @@ -192,7 +197,7 @@ class CacheProviderTest extends DDSpecification { then: // cache will start to proactively free slots & size calc is approximate - cacheInstance.approximateSize() > capacity - 4 + poolStrat.approximateSize() > capacity - 4 } static newVoid() { From d50f901f3993cbfddb4e8b1abf8cf01880ae6adc Mon Sep 17 00:00:00 2001 From: dougqh Date: Mon, 27 Jan 2020 12:34:39 -0500 Subject: [PATCH 06/11] googleJavaFormat, codeNarcTest, and test reliability --- .../agent/tooling/DDCachingPoolStrategy.java | 56 +++++++++---------- .../agent/tooling/CacheProviderTest.groovy | 7 +-- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index 1e47032a7a..7f3d3f7e83 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -4,9 +4,7 @@ import static net.bytebuddy.agent.builder.AgentBuilder.PoolStrategy; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; - import java.lang.ref.WeakReference; - import lombok.extern.slf4j.Slf4j; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.ClassFileLocator; @@ -54,9 +52,10 @@ public class DDCachingPoolStrategy implements PoolStrategy { /** * Cache of recent ClassLoader WeakReferences; used to... + * *
    - *
  • Reduced number of WeakReferences created
    • - *
  • Allow for quick fast path equivalence check of composite keys
    • + *
  • Reduced number of WeakReferences created + *
  • Allow for quick fast path equivalence check of composite keys *
*/ final Cache> loaderRefCache = @@ -68,8 +67,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { .build(); /** - * Single shared Type.Resolution cache -- uses a composite key -- - * conceptually of loader & name + * Single shared Type.Resolution cache -- uses a composite key -- conceptually of loader & name */ final Cache sharedResolutionCache = CacheBuilder.newBuilder() @@ -79,9 +77,9 @@ public class DDCachingPoolStrategy implements PoolStrategy { .maximumSize(TYPE_CAPACITY) .build(); - /** Fast path for bootstrap */ - final SharedResolutionCacheAdapter bootstrapCacheProvider = - new SharedResolutionCacheAdapter(BOOTSTRAP_HASH, null, sharedResolutionCache); + /** Fast path for bootstrap */ + final SharedResolutionCacheAdapter bootstrapCacheProvider = + new SharedResolutionCacheAdapter(BOOTSTRAP_HASH, null, sharedResolutionCache); @Override public final TypePool typePool( @@ -92,7 +90,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { WeakReference loaderRef = loaderRefCache.getIfPresent(classLoader); - if ( loaderRef == null ) { + if (loaderRef == null) { loaderRef = new WeakReference<>(classLoader); loaderRefCache.put(classLoader, loaderRef); } @@ -107,20 +105,18 @@ public class DDCachingPoolStrategy implements PoolStrategy { } private final TypePool.CacheProvider createCacheProvider( - final int loaderHash, - final WeakReference loaderRef) - { + final int loaderHash, final WeakReference loaderRef) { return new SharedResolutionCacheAdapter(loaderHash, loaderRef, sharedResolutionCache); } private final TypePool createCachingTypePool( - final int loaderHash, - final WeakReference loaderRef, - final ClassFileLocator classFileLocator) { + final int loaderHash, + final WeakReference loaderRef, + final ClassFileLocator classFileLocator) { return new TypePool.Default.WithLazyResolution( - createCacheProvider(loaderHash, loaderRef), - classFileLocator, - TypePool.Default.ReaderMode.FAST); + createCacheProvider(loaderHash, loaderRef), + classFileLocator, + TypePool.Default.ReaderMode.FAST); } private final TypePool createCachingTypePool( @@ -145,10 +141,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { private final int hashCode; TypeCacheKey( - final int loaderHash, - final WeakReference loaderRef, - final String className) - { + final int loaderHash, final WeakReference loaderRef, final String className) { this.loaderHash = loaderHash; this.loaderRef = loaderRef; this.className = className; @@ -163,18 +156,18 @@ public class DDCachingPoolStrategy implements PoolStrategy { @Override public boolean equals(final Object obj) { - if ( !(obj instanceof TypeCacheKey) ) return false; + if (!(obj instanceof TypeCacheKey)) return false; - TypeCacheKey that = (TypeCacheKey)obj; + TypeCacheKey that = (TypeCacheKey) obj; - if ( loaderHash != that.loaderHash ) return false; + if (loaderHash != that.loaderHash) return false; // Fastpath loaderRef equivalence -- works because of WeakReference cache used // Also covers the bootstrap null loaderRef case - if ( loaderRef == that.loaderRef ) { + if (loaderRef == that.loaderRef) { // still need to check name return className.equals(that.className); - } else if ( className.equals(that.className) ) { + } else if (className.equals(that.className)) { // need to perform a deeper loader check -- requires calling Reference.get // which can strengthened the Reference, so deliberately done last @@ -182,10 +175,10 @@ public class DDCachingPoolStrategy implements PoolStrategy { // Technically, this is a bit of violation of equals semantics, since // two equivalent references can be not equivalent. ClassLoader thisLoader = loaderRef.get(); - if ( thisLoader == null ) return false; + if (thisLoader == null) return false; ClassLoader thatLoader = that.loaderRef.get(); - if ( thatLoader == null ) return false; + if (thatLoader == null) return false; return (thisLoader == thatLoader); } else { @@ -226,7 +219,8 @@ public class DDCachingPoolStrategy implements PoolStrategy { } @Override - public TypePool.Resolution register(final String className, final TypePool.Resolution resolution) { + public TypePool.Resolution register( + final String className, final TypePool.Resolution resolution) { if (OBJECT_NAME.equals(className)) { return resolution; } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy index 4ce1bc5ba5..354069acea 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy @@ -7,12 +7,11 @@ import net.bytebuddy.pool.TypePool import spock.lang.Timeout import java.lang.ref.WeakReference -import java.security.SecureClassLoader @Timeout(5) class CacheProviderTest extends DDSpecification { def "key bootstrap equivalence"() { - def loader = null + // def loader = null def loaderHash = DDCachingPoolStrategy.BOOTSTRAP_HASH def loaderRef = null @@ -197,7 +196,7 @@ class CacheProviderTest extends DDSpecification { then: // cache will start to proactively free slots & size calc is approximate - poolStrat.approximateSize() > capacity - 4 + poolStrat.approximateSize() > 0.9 * capacity } static newVoid() { @@ -205,7 +204,7 @@ class CacheProviderTest extends DDSpecification { } static newClassLoader() { - return new URLClassLoader([] as URL[], (ClassLoader)null); + return new URLClassLoader([] as URL[], (ClassLoader)null) } static newLocator() { From 4c7a0ba7a7ca60918b927ca59a315ec1044eb7f4 Mon Sep 17 00:00:00 2001 From: dougqh Date: Mon, 27 Jan 2020 17:14:17 -0500 Subject: [PATCH 07/11] Fixing muzzle? MuzzlePlugin groovy checks that no threads are spawned because this holds the ClassLoader live. This was breaking with the caching change because the cache no longer uses the Cleaner service. This caused a problem because the Thread behind the cleaner is created lazily when the first task is created, but without the cache the creation was delayed. To solve this, I addressed the original cause of the leak. The newly created Thread automatically inherits the contextClassLoader of its parent, but that's unnecessary for a cleaner thread. So I changed the ThreadFactory for cleaner to explicitly null out the contextClassLoader. We should probably null out contextClassLoader in other thread factories and also reduce our use of contextClassLoaders in general, but that will left to another PR. --- buildSrc/src/main/groovy/MuzzlePlugin.groovy | 5 ++++- .../src/main/java/datadog/trace/agent/tooling/Cleaner.java | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy index 99ad0b90dd..2f852d9fd3 100644 --- a/buildSrc/src/main/groovy/MuzzlePlugin.groovy +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -291,7 +291,10 @@ class MuzzlePlugin implements Plugin { doLast { final ClassLoader instrumentationCL = createInstrumentationClassloader(instrumentationProject, toolingProject) def ccl = Thread.currentThread().contextClassLoader - def bogusLoader = new SecureClassLoader() + def bogusLoader = new SecureClassLoader() { + @Override + String toString() { return "bogus" } + } Thread.currentThread().contextClassLoader = bogusLoader final ClassLoader userCL = createClassLoaderForTask(instrumentationProject, bootstrapProject, taskName) try { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java index 8115fb0f48..61529aff63 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java @@ -20,6 +20,7 @@ class Cleaner { final Thread thread = new Thread(r, "dd-cleaner"); thread.setDaemon(true); thread.setPriority(Thread.MIN_PRIORITY); + thread.setContextClassLoader(null); return thread; } }; From 176f826a440c9557ec933d7d62c90c1aa416de09 Mon Sep 17 00:00:00 2001 From: dougqh Date: Mon, 27 Jan 2020 17:44:39 -0500 Subject: [PATCH 08/11] Adjusting approximateSize check to be more reliable --- .../groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy index 354069acea..111763d098 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy @@ -185,7 +185,7 @@ class CacheProviderTest extends DDSpecification { then: // cache will start to proactively free slots & size calc is approximate - poolStrat.approximateSize() > capacity - 4 + poolStrat.approximateSize() > 0.9 * capacity when: 10.times { From 0f095f0adbafb52b745f439de6fc1f9ce1864e0e Mon Sep 17 00:00:00 2001 From: dougqh Date: Thu, 30 Jan 2020 17:44:24 -0500 Subject: [PATCH 09/11] Final clean-up - Removed unused method from earlier version - Corrected previously overlooked comments that were remnant of prior version --- .../agent/tooling/DDCachingPoolStrategy.java | 37 +++++++++---------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index 7f3d3f7e83..cc5165dfa8 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -22,22 +22,15 @@ import net.bytebuddy.pool.TypePool; * *
    * There two core parts to the cache... - *
  • a cache of ID assignments for ClassLoaders - *
  • a single cache of TypeResolutions for all ClassLoaders - keyed by a custom composite key - * that combines loader ID & name + *
  • a cache of ClassLoader to WeakReference<ClassLoader> + *
  • a single cache of TypeResolutions for all ClassLoaders - keyed by a custom composite key of ClassLoader & class name *
* *

This design was chosen to create a single limited size cache that can be adjusted for the * entire application -- without having to create a large number of WeakReference objects. * - *

The ID assignment mostly assigns a single ID to each ClassLoader, but the maximumSize - * restriction means that an evicted ClassLoader could be assigned another ID later on. - * - *

For the validity of the cache, the important part is that ID assignment guarantees that no two - * ClassLoaders share the same ID. - * - *

NOTE: As an additional safe-guard, a new CacheInstance can be created if the original loader - * ID sequence is exhausted. + *

Eviction is handled almost entirely through a size restriction; however, + * softValues are still used as a further safeguard. */ @Slf4j public class DDCachingPoolStrategy implements PoolStrategy { @@ -99,11 +92,6 @@ public class DDCachingPoolStrategy implements PoolStrategy { return createCachingTypePool(loaderHash, loaderRef, classFileLocator); } - private final TypePool createNonCachingTypePool(final ClassFileLocator classFileLocator) { - return new TypePool.Default.WithLazyResolution( - TypePool.CacheProvider.NoOp.INSTANCE, classFileLocator, TypePool.Default.ReaderMode.FAST); - } - private final TypePool.CacheProvider createCacheProvider( final int loaderHash, final WeakReference loaderRef) { return new SharedResolutionCacheAdapter(loaderHash, loaderRef, sharedResolutionCache); @@ -130,8 +118,13 @@ public class DDCachingPoolStrategy implements PoolStrategy { } /** - * TypeCacheKey is key for the sharedResolutionCache. It is a mix of a cacheId/loaderId & a type - * name. + * TypeCacheKey is key for the sharedResolutionCache. + * Conceptually, it is a mix of ClassLoader & class name. + * + * For efficiency & GC purposes, it is actually composed of + * loaderHash & WeakReference<ClassLoader> + * + * The loaderHash exists to avoid calling get & strengthening the Reference. */ static final class TypeCacheKey { private final int loaderHash; @@ -169,11 +162,15 @@ public class DDCachingPoolStrategy implements PoolStrategy { return className.equals(that.className); } else if (className.equals(that.className)) { // need to perform a deeper loader check -- requires calling Reference.get - // which can strengthened the Reference, so deliberately done last + // which can strengthen the Reference, so deliberately done last // If either reference has gone null, they aren't considered equivalent // Technically, this is a bit of violation of equals semantics, since - // two equivalent references can be not equivalent. + // two equivalent references can become not equivalent. + + // In this case, it is fine because that means the ClassLoader is no + // longer live, so the entries will never match anyway and will fall + // out of the cache. ClassLoader thisLoader = loaderRef.get(); if (thisLoader == null) return false; From 235a6470fb0afd7bd317179644e66294cfd825fc Mon Sep 17 00:00:00 2001 From: dougqh Date: Thu, 30 Jan 2020 17:45:40 -0500 Subject: [PATCH 10/11] googleJavaFormat --- .../agent/tooling/DDCachingPoolStrategy.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index cc5165dfa8..1d4cccdb35 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -23,14 +23,15 @@ import net.bytebuddy.pool.TypePool; *

    * There two core parts to the cache... *
  • a cache of ClassLoader to WeakReference<ClassLoader> - *
  • a single cache of TypeResolutions for all ClassLoaders - keyed by a custom composite key of ClassLoader & class name + *
  • a single cache of TypeResolutions for all ClassLoaders - keyed by a custom composite key of + * ClassLoader & class name *
* *

This design was chosen to create a single limited size cache that can be adjusted for the * entire application -- without having to create a large number of WeakReference objects. * - *

Eviction is handled almost entirely through a size restriction; however, - * softValues are still used as a further safeguard. + *

Eviction is handled almost entirely through a size restriction; however, softValues are still + * used as a further safeguard. */ @Slf4j public class DDCachingPoolStrategy implements PoolStrategy { @@ -118,13 +119,13 @@ public class DDCachingPoolStrategy implements PoolStrategy { } /** - * TypeCacheKey is key for the sharedResolutionCache. - * Conceptually, it is a mix of ClassLoader & class name. + * TypeCacheKey is key for the sharedResolutionCache. Conceptually, it is a mix of ClassLoader & + * class name. * - * For efficiency & GC purposes, it is actually composed of - * loaderHash & WeakReference<ClassLoader> + *

For efficiency & GC purposes, it is actually composed of loaderHash & + * WeakReference<ClassLoader> * - * The loaderHash exists to avoid calling get & strengthening the Reference. + *

The loaderHash exists to avoid calling get & strengthening the Reference. */ static final class TypeCacheKey { private final int loaderHash; From faeb0694240c564e272abe1a6fb7021cf1887b7f Mon Sep 17 00:00:00 2001 From: dougqh Date: Thu, 30 Jan 2020 18:08:49 -0500 Subject: [PATCH 11/11] Adjusting capacity check again --- .../datadog/trace/agent/tooling/CacheProviderTest.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy index 111763d098..73d0a1fe19 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CacheProviderTest.groovy @@ -185,7 +185,7 @@ class CacheProviderTest extends DDSpecification { then: // cache will start to proactively free slots & size calc is approximate - poolStrat.approximateSize() > 0.9 * capacity + poolStrat.approximateSize() > 0.8 * capacity when: 10.times { @@ -196,7 +196,7 @@ class CacheProviderTest extends DDSpecification { then: // cache will start to proactively free slots & size calc is approximate - poolStrat.approximateSize() > 0.9 * capacity + poolStrat.approximateSize() > 0.8 * capacity } static newVoid() {