From 9cd5242ee9361d5434ccabccab6e67e9c3c3c0b1 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Wed, 26 Apr 2023 11:32:12 +0100 Subject: [PATCH 1/8] Rename ClassLoaderMatchers.reset to more descriptive resetState --- .../agent/tooling/bytebuddy/matcher/ClassLoaderMatchers.java | 2 +- .../trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/ClassLoaderMatchers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/ClassLoaderMatchers.java index 1f11ba07ce5..1cea548c181 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/ClassLoaderMatchers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/ClassLoaderMatchers.java @@ -89,7 +89,7 @@ public static ElementMatcher.Junction hasClassNamedOneOf( return new ElementMatcher.Junction.Disjunction<>(matchers); } - public static void reset() { + public static void resetState() { hasClassCache.clear(); } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java index 9ad60bee9c5..72562cd902e 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java @@ -44,7 +44,7 @@ public static void assertInstrumentationMuzzled( final List mismatches = muzzle.getMismatchedReferenceSources(testApplicationLoader); - ClassLoaderMatchers.reset(); + ClassLoaderMatchers.resetState(); final boolean classLoaderMatch = instrumenter.classLoaderMatcher().matches(testApplicationLoader); From 7d3491b5af9b8b99735cbf5b20cd86b21cc5b6a8 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Wed, 22 Mar 2023 22:16:38 +0000 Subject: [PATCH 2/8] Support memoization of hierarchical matches to avoid having to repeatedly walk the type hierarchy --- .../trace/agent/tooling/AgentInstaller.java | 7 +- .../bytebuddy/memoize/HasContextField.java | 63 +++++ .../bytebuddy/memoize/HasSuperMethod.java | 83 +++++++ .../bytebuddy/memoize/MemoizedMatchers.java | 130 ++++++++++ .../tooling/bytebuddy/memoize/Memoizer.java | 234 ++++++++++++++++++ .../bytebuddy/outline/TypePoolFacade.java | 5 + .../context/FieldBackedContextInjector.java | 14 +- internal-api/build.gradle | 1 + .../datadog/trace/api/InstrumenterConfig.java | 10 +- .../trace/api/ResolverCacheConfig.java | 52 +++- 10 files changed, 588 insertions(+), 11 deletions(-) create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasContextField.java create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasSuperMethod.java create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/MemoizedMatchers.java create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java diff --git a/dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index 57f9f875aec..9d5b176949c 100644 --- a/dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-builder/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -7,6 +7,7 @@ import datadog.trace.agent.tooling.bytebuddy.DDOutlinePoolStrategy; import datadog.trace.agent.tooling.bytebuddy.SharedTypePools; import datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers; +import datadog.trace.agent.tooling.bytebuddy.memoize.MemoizedMatchers; import datadog.trace.agent.tooling.usm.UsmExtractorImpl; import datadog.trace.agent.tooling.usm.UsmMessageFactoryImpl; import datadog.trace.api.InstrumenterConfig; @@ -101,7 +102,11 @@ public static ClassFileTransformer installBytebuddyAgent( DDCachingPoolStrategy.registerAsSupplier(); } - DDElementMatchers.registerAsSupplier(); + if (InstrumenterConfig.get().isResolverMemoizingEnabled()) { + MemoizedMatchers.registerAsSupplier(); + } else { + DDElementMatchers.registerAsSupplier(); + } if (enabledSystems.contains(Instrumenter.TargetSystem.USM)) { UsmMessageFactoryImpl.registerAsSupplier(); diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasContextField.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasContextField.java new file mode 100644 index 00000000000..1c25981b63e --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasContextField.java @@ -0,0 +1,63 @@ +package datadog.trace.agent.tooling.bytebuddy.memoize; + +import datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter; +import java.util.BitSet; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +/** Matches types that should have context-store fields injected. */ +final class HasContextField extends ElementMatcher.Junction.ForNonNullValues { + private final ElementMatcher storeMatcher; + private final ElementMatcher skipMatcher; + private final BitSet skippableStoreIds = new BitSet(); + + HasContextField(ElementMatcher storeMatcher) { + this(storeMatcher, null); + } + + HasContextField( + ElementMatcher storeMatcher, ElementMatcher skipMatcher) { + this.storeMatcher = storeMatcher; + this.skipMatcher = skipMatcher; + } + + @Override + protected boolean doMatch(TypeDescription target) { + // match first type in the hierarchy which injects this store, that isn't skipped + return storeMatcher.matches(target) + && (null == skipMatcher || !skipMatcher.matches(target)) + && (!storeMatcher.matches(target.getSuperClass().asErasure())); + } + + void maybeSkip(int contextStoreId) { + skippableStoreIds.set(contextStoreId); + } + + boolean hasSuperStore(TypeDescription target, BitSet weakStoreIds) { + TypeDescription superTarget = target.getSuperClass().asErasure(); + boolean hasSuperStore = storeMatcher.matches(superTarget); + if (hasSuperStore) { + // report if super-class was skipped from field-injection + if (null != skipMatcher && skipMatcher.matches(superTarget)) { + synchronized (weakStoreIds) { + weakStoreIds.or(skippableStoreIds); + } + } + } + return hasSuperStore; + } + + /** Matches types that would have had fields injected, but were explicitly excluded. */ + static final class Skip extends ElementMatcher.Junction.ForNonNullValues { + private final ExcludeFilter.ExcludeType excludeType; + + Skip(ExcludeFilter.ExcludeType excludeType) { + this.excludeType = excludeType; + } + + @Override + protected boolean doMatch(TypeDescription target) { + return ExcludeFilter.exclude(excludeType, target.getName()); + } + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasSuperMethod.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasSuperMethod.java new file mode 100644 index 00000000000..5d408072b9c --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasSuperMethod.java @@ -0,0 +1,83 @@ +package datadog.trace.agent.tooling.bytebuddy.memoize; + +import static net.bytebuddy.matcher.ElementMatchers.hasSignature; + +import java.util.HashSet; +import java.util.Set; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDefinition; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.description.type.TypeList; +import net.bytebuddy.matcher.ElementMatcher; + +/** + * Matches methods that are either a direct match or have a super-class method with the same + * signature that matches. Uses a memoized type matcher to reduce the potential search space. + */ +final class HasSuperMethod extends ElementMatcher.Junction.ForNonNullValues { + private final ElementMatcher typeMatcher; + private final ElementMatcher methodMatcher; + + HasSuperMethod( + ElementMatcher typeMatcher, + ElementMatcher methodMatcher) { + this.typeMatcher = typeMatcher; + this.methodMatcher = methodMatcher; + } + + @Override + protected boolean doMatch(MethodDescription target) { + if (target.isConstructor()) { + return false; + } + + TypeDefinition type = target.getDeclaringType(); + if (!typeMatcher.matches(type.asErasure())) { + return false; // no further matches recorded in hierarchy + } else if (methodMatcher.matches(target)) { + return true; // direct match, no need to check hierarchy + } + + // need to search hierarchy; use expected signature to filter candidate methods + ElementMatcher signatureMatcher = hasSignature(target.asSignatureToken()); + Set visited = new HashSet<>(); + + if (interfaceMatches(type.getInterfaces(), signatureMatcher, visited)) { + return true; + } + + type = type.getSuperClass(); + while (null != type && typeMatcher.matches(type.asErasure())) { + for (MethodDescription method : type.getDeclaredMethods()) { + if (signatureMatcher.matches(method) && methodMatcher.matches(method)) { + return true; + } + } + if (interfaceMatches(type.getInterfaces(), signatureMatcher, visited)) { + return true; + } + type = type.getSuperClass(); + } + return false; + } + + private boolean interfaceMatches( + TypeList.Generic interfaces, + ElementMatcher signatureMatcher, + Set visited) { + for (TypeDefinition type : interfaces) { + if (!visited.add(type.getTypeName()) || !typeMatcher.matches(type.asErasure())) { + continue; // skip if already visited or that part of the hierarchy doesn't match + } + for (MethodDescription method : type.getDeclaredMethods()) { + if (signatureMatcher.matches(method) && methodMatcher.matches(method)) { + return true; + } + } + if (interfaceMatches(type.getInterfaces(), signatureMatcher, visited)) { + return true; + } + } + return false; + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/MemoizedMatchers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/MemoizedMatchers.java new file mode 100644 index 00000000000..a2d1f284073 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/MemoizedMatchers.java @@ -0,0 +1,130 @@ +package datadog.trace.agent.tooling.bytebuddy.memoize; + +import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static datadog.trace.agent.tooling.bytebuddy.memoize.Memoizer.MatcherKind.ANNOTATION; +import static datadog.trace.agent.tooling.bytebuddy.memoize.Memoizer.MatcherKind.CLASS; +import static datadog.trace.agent.tooling.bytebuddy.memoize.Memoizer.MatcherKind.FIELD; +import static datadog.trace.agent.tooling.bytebuddy.memoize.Memoizer.MatcherKind.INTERFACE; +import static datadog.trace.agent.tooling.bytebuddy.memoize.Memoizer.MatcherKind.METHOD; +import static datadog.trace.agent.tooling.bytebuddy.memoize.Memoizer.MatcherKind.TYPE; +import static datadog.trace.bootstrap.FieldBackedContextStores.getContextStoreId; + +import datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers; +import datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter; +import java.util.BitSet; +import java.util.HashMap; +import java.util.Map; +import net.bytebuddy.description.NamedElement; +import net.bytebuddy.description.field.FieldDescription; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +/** Supplies memoized matchers. */ +public final class MemoizedMatchers implements HierarchyMatchers.Supplier { + public static void registerAsSupplier() { + HierarchyMatchers.registerIfAbsent(new MemoizedMatchers()); + Memoizer.resetState(); + } + + @Override + public ElementMatcher.Junction declaresAnnotation( + ElementMatcher.Junction matcher) { + return Memoizer.prepare(ANNOTATION, matcher, false); + } + + @Override + public ElementMatcher.Junction declaresField( + ElementMatcher.Junction matcher) { + return Memoizer.prepare(FIELD, matcher, false); + } + + @Override + public ElementMatcher.Junction declaresMethod( + ElementMatcher.Junction matcher) { + return Memoizer.prepare(METHOD, matcher, false); + } + + @Override + public ElementMatcher.Junction concreteClass() { + return Memoizer.isConcrete; + } + + @Override + public ElementMatcher.Junction extendsClass( + ElementMatcher.Junction matcher) { + return Memoizer.prepare(CLASS, matcher, true); + } + + @Override + public ElementMatcher.Junction implementsInterface( + ElementMatcher.Junction matcher) { + return Memoizer.isClass.and(Memoizer.prepare(INTERFACE, matcher, true)); + } + + @Override + public ElementMatcher.Junction hasInterface( + ElementMatcher.Junction matcher) { + return Memoizer.prepare(INTERFACE, matcher, true); + } + + @Override + public ElementMatcher.Junction hasSuperType( + ElementMatcher.Junction matcher) { + return Memoizer.isClass.and(Memoizer.prepare(TYPE, matcher, true)); + } + + @Override + public ElementMatcher.Junction hasSuperMethod( + ElementMatcher.Junction matcher) { + return new HasSuperMethod(Memoizer.prepare(METHOD, matcher, true), matcher); + } + + /** Keeps track of which context-field matchers we've supplied so far. */ + private static final Map contextFields = new HashMap<>(); + + @Override + public ElementMatcher.Junction declaresContextField( + String keyType, String contextType) { + + // is there a chance a type might match, but be excluded (skipped) from field-injection? + ExcludeFilter.ExcludeType excludeType = ExcludeFilter.ExcludeType.fromFieldType(keyType); + + HasContextField contextField = contextFields.get(keyType); + if (null == contextField) { + ElementMatcher storeMatcher = hasSuperType(named(keyType)); + if (null != excludeType) { + ElementMatcher skipMatcher = + Memoizer.prepare(CLASS, new HasContextField.Skip(excludeType), true); + contextField = new HasContextField(storeMatcher, skipMatcher); + } else { + contextField = new HasContextField(storeMatcher); + } + contextFields.put(keyType, contextField); + } + + if (null != excludeType) { + // record that this store may be skipped from field-injection + contextField.maybeSkip(getContextStoreId(keyType, contextType)); + } + + return contextField; + } + + /** + * Returns {@code true} if ancestors of the given type have any context-stores. + * + *

Stores which were skipped from field-injection in the super-class hierarchy are recorded in + * {@code weakStoreIds} so the appropriate weak-map delegation can be applied when injecting code + * to handle context-store requests. + */ + public static boolean hasSuperStores(TypeDescription target, BitSet weakStoreIds) { + boolean hasSuperStores = false; + for (HasContextField contextField : contextFields.values()) { + if (contextField.hasSuperStore(target, weakStoreIds)) { + hasSuperStores = true; + } + } + return hasSuperStores; + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java new file mode 100644 index 00000000000..1520db0052c --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java @@ -0,0 +1,234 @@ +package datadog.trace.agent.tooling.bytebuddy.memoize; + +import static net.bytebuddy.matcher.ElementMatchers.not; + +import datadog.trace.agent.tooling.bytebuddy.TypeInfoCache; +import datadog.trace.agent.tooling.bytebuddy.TypeInfoCache.SharedTypeInfo; +import datadog.trace.agent.tooling.bytebuddy.outline.TypePoolFacade; +import datadog.trace.api.InstrumenterConfig; +import datadog.trace.api.cache.DDCache; +import datadog.trace.api.cache.DDCaches; +import de.thetaphi.forbiddenapis.SuppressForbidden; +import java.util.ArrayList; +import java.util.BitSet; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import net.bytebuddy.description.annotation.AnnotationDescription; +import net.bytebuddy.description.field.FieldDescription; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import net.bytebuddy.matcher.ElementMatchers; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@SuppressForbidden +@SuppressWarnings("rawtypes") +public final class Memoizer { + private static final Logger log = LoggerFactory.getLogger(Memoizer.class); + + enum MatcherKind { + ANNOTATION, + FIELD, + METHOD, + CLASS, + INTERFACE, + TYPE // i.e. class or interface + } + + private static final BitSet NO_MATCH = new BitSet(0); + + private static final int SIZE_HINT = 320; // estimated number of matchers + + // records the kind of matcher and whether matches should be inherited + private static final BitSet annotationMatcherIds = new BitSet(SIZE_HINT); + private static final BitSet fieldMatcherIds = new BitSet(SIZE_HINT); + private static final BitSet methodMatcherIds = new BitSet(SIZE_HINT); + private static final BitSet classMatcherIds = new BitSet(SIZE_HINT); + private static final BitSet interfaceMatcherIds = new BitSet(SIZE_HINT); + private static final BitSet inheritedMatcherIds = new BitSet(SIZE_HINT); + + // matchers that are ready for memoization + static final List matchers = new ArrayList<>(); + + // small cache to de-duplicate memoization requests + private static final DDCache memoizingMatcherCache = + DDCaches.newFixedSizeIdentityCache(8); + + // caches positive memoized matches + private static final TypeInfoCache memos = + new TypeInfoCache<>(InstrumenterConfig.get().getResolverMemoPoolSize(), true); + + // local memoized results, used to detect circular references + static final ThreadLocal> localMemosHolder = + ThreadLocal.withInitial(HashMap::new); + + private static final int INTERNAL_MATCHERS = 2; // isClass and isConcrete + + // memoize whether the type is a class + static final ElementMatcher.Junction isClass = + prepare(MatcherKind.CLASS, ElementMatchers.any(), true); + + // memoize whether the type is a concrete class, i.e. no abstract methods + static final ElementMatcher.Junction isConcrete = + prepare(MatcherKind.CLASS, not(ElementMatchers.isAbstract()), false); + + public static void resetState() { + // no need to reset the state if we haven't added any external matchers + if (matchers.size() > INTERNAL_MATCHERS) { + Memoizer.clear(); + } + } + + public static void clear() { + memos.clear(); + } + + static MemoizingMatcher withMatcherId(ElementMatcher matcher) { + return new MemoizingMatcher(matchers.size()); + } + + /** Prepares a matcher for memoization. */ + static ElementMatcher.Junction prepare( + MatcherKind kind, ElementMatcher.Junction matcher, boolean inherited) { + + MemoizingMatcher memoizingMatcher = + memoizingMatcherCache.computeIfAbsent(matcher, Memoizer::withMatcherId); + + int matcherId = memoizingMatcher.matcherId; + if (matcherId < matchers.size()) { + return memoizingMatcher; // de-duplicated matcher + } else { + matchers.add(matcher); + } + + switch (kind) { + case ANNOTATION: + annotationMatcherIds.set(matcherId); + break; + case FIELD: + fieldMatcherIds.set(matcherId); + break; + case METHOD: + methodMatcherIds.set(matcherId); + break; + case CLASS: + classMatcherIds.set(matcherId); + break; + case INTERFACE: + interfaceMatcherIds.set(matcherId); + break; + case TYPE: + classMatcherIds.set(matcherId); + interfaceMatcherIds.set(matcherId); + break; + } + + if (inherited) { + inheritedMatcherIds.set(matcherId); + } + + return memoizingMatcher; + } + + /** Matcher wrapper that triggers memoization on request. */ + static final class MemoizingMatcher + extends ElementMatcher.Junction.ForNonNullValues { + final int matcherId; + + MemoizingMatcher(int matcherId) { + this.matcherId = matcherId; + } + + @Override + protected boolean doMatch(TypeDescription target) { + if ("java.lang.Object".equals(target.getName()) || target.isPrimitive()) { + return false; + } else { + return doMemoize(target, localMemosHolder.get()).get(matcherId); + } + } + } + + static BitSet doMemoize(TypeDescription type, Map localMemos) { + + String name = type.getName(); + BitSet memo = localMemos.get(name); + if (null != memo) { + return memo; // short-circuit circular references + } + + SharedTypeInfo sharedMemo = memos.find(name); + if (null != sharedMemo) { + return sharedMemo.get(); + } + + localMemos.put(name, memo = new BitSet(matchers.size())); + boolean wasFullParsing = TypePoolFacade.disableFullDescriptions(); // only need outlines here + try { + TypeDescription.Generic superType = type.getSuperClass(); + if (null != superType && !"java.lang.Object".equals(superType.getTypeName())) { + inherit(doMemoize(superType.asErasure(), localMemos), memo); + } + for (TypeDescription.Generic intf : type.getInterfaces()) { + inherit(doMemoize(intf.asErasure(), localMemos), memo); + } + for (AnnotationDescription ann : type.getDeclaredAnnotations()) { + record(annotationMatcherIds, ann.getAnnotationType(), memo); + } + for (FieldDescription field : type.getDeclaredFields()) { + record(fieldMatcherIds, field, memo); + } + for (MethodDescription method : type.getDeclaredMethods()) { + record(methodMatcherIds, method, memo); + } + record(type.isInterface() ? interfaceMatcherIds : classMatcherIds, type, memo); + } catch (Throwable e) { + if (log.isDebugEnabled()) { + log.debug( + "{} recording matches for type {}: {}", + e.getClass().getSimpleName(), + name, + e.getMessage()); + } + } finally { + localMemos.remove(name); + if (wasFullParsing) { + TypePoolFacade.enableFullDescriptions(); + } + } + + if (memo.nextSetBit(INTERNAL_MATCHERS) < 0) { + memo = NO_MATCH; // we're only interested if there's at least one external match + } + + memos.share(name, null, null, memo); + + return memo; + } + + /** Inherit positive matches from a super-class or interface. */ + private static void inherit(BitSet superMemo, BitSet memo) { + int matcherId = superMemo.nextSetBit(0); + while (matcherId >= 0) { + if (inheritedMatcherIds.get(matcherId)) { + memo.set(matcherId); + } + matcherId = superMemo.nextSetBit(matcherId + 1); + } + } + + /** Run a series of memoized matchers and record positive matches. */ + @SuppressWarnings("unchecked") + private static void record(BitSet matcherIds, Object type, BitSet memo) { + int matcherId = matcherIds.nextSetBit(0); + while (matcherId >= 0) { + // can skip match if we've already inherited a positive result + if (!memo.get(matcherId) && matchers.get(matcherId).matches(type)) { + memo.set(matcherId); + } + matcherId = matcherIds.nextSetBit(matcherId + 1); + } + } +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/outline/TypePoolFacade.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/outline/TypePoolFacade.java index 5e31f676322..e4882db4b83 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/outline/TypePoolFacade.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/outline/TypePoolFacade.java @@ -5,6 +5,8 @@ import static datadog.trace.agent.tooling.bytebuddy.outline.TypeFactory.typeFactory; import datadog.trace.agent.tooling.bytebuddy.SharedTypePools; +import datadog.trace.agent.tooling.bytebuddy.memoize.Memoizer; +import datadog.trace.api.InstrumenterConfig; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.pool.TypePool; @@ -63,6 +65,9 @@ public void endTransform() { @Override public void clear() { + if (InstrumenterConfig.get().isResolverMemoizingEnabled()) { + Memoizer.clear(); + } TypeFactory.clear(); } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedContextInjector.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedContextInjector.java index 77f332ec8ff..fad271966e4 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedContextInjector.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedContextInjector.java @@ -1,9 +1,9 @@ package datadog.trace.agent.tooling.context; -import static datadog.trace.agent.tooling.context.ShouldInjectFieldsState.hasInjectedField; import static datadog.trace.bootstrap.FieldBackedContextStores.getContextStoreId; import static datadog.trace.util.Strings.getInternalName; +import datadog.trace.agent.tooling.bytebuddy.memoize.MemoizedMatchers; import datadog.trace.api.InstrumenterConfig; import datadog.trace.bootstrap.ContextStore; import datadog.trace.bootstrap.FieldBackedContextAccessor; @@ -70,6 +70,8 @@ public final class FieldBackedContextInjector implements AsmVisitorWrapper { final boolean serialVersionUIDFieldInjection = InstrumenterConfig.get().isSerialVersionUIDFieldInjection(); + final boolean isMemoizingEnabled = InstrumenterConfig.get().isResolverMemoizingEnabled(); + final String keyClassName; final String contextClassName; @@ -203,8 +205,14 @@ public void visitEnd() { BitSet weakStoreIds = new BitSet(); // check hierarchy to see if we might need to delegate to the superclass - boolean hasSuperStores = - hasInjectedField(instrumentedType.getSuperClass(), weakStoreIds); + boolean hasSuperStores; + if (isMemoizingEnabled) { + hasSuperStores = MemoizedMatchers.hasSuperStores(instrumentedType, weakStoreIds); + } else { + hasSuperStores = + ShouldInjectFieldsState.hasInjectedField( + instrumentedType.getSuperClass(), weakStoreIds); + } if (!foundGetter) { addStoreGetter(injectedStoreIds, hasSuperStores, weakStoreIds); diff --git a/internal-api/build.gradle b/internal-api/build.gradle index 9826ec6f916..a0c0d8637c7 100644 --- a/internal-api/build.gradle +++ b/internal-api/build.gradle @@ -102,6 +102,7 @@ excludedClassesCoverage += [ "datadog.trace.api.DynamicConfig.Builder", "datadog.trace.api.DynamicConfig.State", "datadog.trace.api.InstrumenterConfig", + "datadog.trace.api.ResolverCacheConfig.*", // can't reliably force same identity hash for different instance to cover branch "datadog.trace.api.cache.FixedSizeCache.IdentityHash", "datadog.trace.api.cache.FixedSizeWeakKeyCache", diff --git a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java index 4a531e65cd8..754c3be5ff3 100644 --- a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java +++ b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java @@ -167,7 +167,7 @@ private InstrumenterConfig() { resolverCacheConfig = configProvider.getEnum( - RESOLVER_CACHE_CONFIG, ResolverCacheConfig.class, ResolverCacheConfig.DEFAULT); + RESOLVER_CACHE_CONFIG, ResolverCacheConfig.class, ResolverCacheConfig.MEMOS); resolverUseLoadClass = configProvider.getBoolean(RESOLVER_USE_LOADCLASS, true); resolverResetInterval = Platform.isNativeImageBuilder() @@ -288,6 +288,14 @@ public List getExcludedCodeSources() { return excludedCodeSources; } + public boolean isResolverMemoizingEnabled() { + return resolverCacheConfig.memoPoolSize() > 0; + } + + public int getResolverMemoPoolSize() { + return resolverCacheConfig.memoPoolSize(); + } + public boolean isResolverOutliningEnabled() { return resolverCacheConfig.outlinePoolSize() > 0; } diff --git a/internal-api/src/main/java/datadog/trace/api/ResolverCacheConfig.java b/internal-api/src/main/java/datadog/trace/api/ResolverCacheConfig.java index 8c42d35249a..f821d570a5e 100644 --- a/internal-api/src/main/java/datadog/trace/api/ResolverCacheConfig.java +++ b/internal-api/src/main/java/datadog/trace/api/ResolverCacheConfig.java @@ -3,21 +3,49 @@ /** Named presets to help configure various caches inside the type resolver/matcher. */ public enum ResolverCacheConfig { - /** Pool sizes to fit large enterprise apps. */ + /** Memoizing and outlining for large enterprise apps. */ LARGE { + @Override + public int memoPoolSize() { + return 32768; + } + @Override public int outlinePoolSize() { - return 4096; + return 512; } @Override public int typePoolSize() { - return 256; + return 128; + } + }, + + /** Memoizing and outlining for the average sized app. */ + MEMOS { + @Override + public int memoPoolSize() { + return 8192; + } + + @Override + public int outlinePoolSize() { + return 128; + } + + @Override + public int typePoolSize() { + return 32; } }, - /** Pool sizes to fit the average sized app. */ - DEFAULT { + /** Outlining only for the average sized app, no memoizing. */ + NO_MEMOS { + @Override + public int memoPoolSize() { + return 0; + } + @Override public int outlinePoolSize() { return 256; @@ -29,8 +57,13 @@ public int typePoolSize() { } }, - /** Pool sizes to fit small microservice apps. */ + /** Outlining only for small microservice apps. */ SMALL { + @Override + public int memoPoolSize() { + return 0; + } + @Override public int outlinePoolSize() { return 32; @@ -44,6 +77,11 @@ public int typePoolSize() { /** The old {@code DDCachingPoolStrategy} behaviour. */ LEGACY { + @Override + public int memoPoolSize() { + return 0; + } + @Override public int outlinePoolSize() { return 0; @@ -55,6 +93,8 @@ public int typePoolSize() { } }; + public abstract int memoPoolSize(); + public abstract int outlinePoolSize(); public abstract int typePoolSize(); From c303777f3245aa59df4100785d3bc956bdc7645e Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Sun, 2 Apr 2023 00:28:41 +0100 Subject: [PATCH 3/8] Add compact no-match filter to avoid re-memoizing uninteresting types, while keeping memory usage low --- .../tooling/bytebuddy/memoize/Memoizer.java | 22 ++++- .../bytebuddy/memoize/NoMatchFilter.java | 86 +++++++++++++++++++ .../datadog/trace/api/InstrumenterConfig.java | 4 + .../trace/api/ResolverCacheConfig.java | 35 +++++++- 4 files changed, 139 insertions(+), 8 deletions(-) create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/NoMatchFilter.java diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java index 1520db0052c..7fbb933a660 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java @@ -77,6 +77,7 @@ enum MatcherKind { public static void resetState() { // no need to reset the state if we haven't added any external matchers if (matchers.size() > INTERNAL_MATCHERS) { + NoMatchFilter.clear(); Memoizer.clear(); } } @@ -143,7 +144,10 @@ static final class MemoizingMatcher @Override protected boolean doMatch(TypeDescription target) { - if ("java.lang.Object".equals(target.getName()) || target.isPrimitive()) { + String targetName = target.getName(); + if (NoMatchFilter.contains(targetName) + || "java.lang.Object".equals(targetName) + || target.isPrimitive()) { return false; } else { return doMemoize(target, localMemosHolder.get()).get(matcherId); @@ -151,6 +155,14 @@ protected boolean doMatch(TypeDescription target) { } } + static BitSet memoizeHierarchy(TypeDescription type, Map localMemos) { + if (NoMatchFilter.contains(type.getName())) { + return NO_MATCH; + } else { + return doMemoize(type, localMemos); + } + } + static BitSet doMemoize(TypeDescription type, Map localMemos) { String name = type.getName(); @@ -169,10 +181,10 @@ static BitSet doMemoize(TypeDescription type, Map localMemos) { try { TypeDescription.Generic superType = type.getSuperClass(); if (null != superType && !"java.lang.Object".equals(superType.getTypeName())) { - inherit(doMemoize(superType.asErasure(), localMemos), memo); + inherit(memoizeHierarchy(superType.asErasure(), localMemos), memo); } for (TypeDescription.Generic intf : type.getInterfaces()) { - inherit(doMemoize(intf.asErasure(), localMemos), memo); + inherit(memoizeHierarchy(intf.asErasure(), localMemos), memo); } for (AnnotationDescription ann : type.getDeclaredAnnotations()) { record(annotationMatcherIds, ann.getAnnotationType(), memo); @@ -199,8 +211,10 @@ static BitSet doMemoize(TypeDescription type, Map localMemos) { } } + // we're only interested in the type if there's at least one external match if (memo.nextSetBit(INTERNAL_MATCHERS) < 0) { - memo = NO_MATCH; // we're only interested if there's at least one external match + NoMatchFilter.add(name); + return NO_MATCH; } memos.share(name, null, null, memo); diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/NoMatchFilter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/NoMatchFilter.java new file mode 100644 index 00000000000..fcc2b7aae8d --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/NoMatchFilter.java @@ -0,0 +1,86 @@ +package datadog.trace.agent.tooling.bytebuddy.memoize; + +import datadog.trace.api.InstrumenterConfig; +import java.util.Arrays; + +/** + * Compact filter that records the hash and short 'class-code' for uninteresting types. + * + *

The 'class-code' includes the length of the package prefix and simple name, as well as the + * first and last characters of the simple name. These elements coupled with the hash of the full + * class name should minimize the probability of collisions without needing to store full names, + * which would otherwise make the no-match filter overly large. + */ +final class NoMatchFilter { + private static final int MAX_CAPACITY = 1 << 16; + private static final int MIN_CAPACITY = 1 << 8; + private static final int MAX_HASH_ATTEMPTS = 3; + + private static final long[] slots; + private static final int slotMask; + + static { + int capacity = InstrumenterConfig.get().getResolverNoMatchesSize(); + if (capacity < MIN_CAPACITY) { + capacity = MIN_CAPACITY; + } else if (capacity > MAX_CAPACITY) { + capacity = MAX_CAPACITY; + } + + // choose enough slot bits to cover the chosen capacity + slotMask = 0xFFFFFFFF >>> Integer.numberOfLeadingZeros(capacity - 1); + slots = new long[slotMask + 1]; + } + + public static boolean contains(String name) { + int hash = name.hashCode(); + for (int i = 1, h = hash; true; i++) { + long value = slots[slotMask & h]; + if (value == 0) { + return false; + } else if ((int) value == hash) { + return (int) (value >>> 32) == classCode(name); + } else if (i == MAX_HASH_ATTEMPTS) { + return false; + } + h = rehash(h); + } + } + + public static void add(String name) { + int index; + int hash = name.hashCode(); + for (int i = 1, h = hash; true; i++) { + index = slotMask & h; + if (slots[index] == 0) { + break; + } else if (i == MAX_HASH_ATTEMPTS) { + index = slotMask & hash; // overwrite original slot + break; + } + h = rehash(h); + } + slots[index] = (long) classCode(name) << 32 | 0xFFFFFFFFL & hash; + } + + public static void clear() { + Arrays.fill(slots, 0); + } + + /** + * Computes a 32-bit 'class-code' that includes the length of the package prefix and simple name, + * plus the first and last characters of the simple name (each truncated to fit into 8-bits.) + */ + private static int classCode(String name) { + int start = name.lastIndexOf('.') + 1; + int end = name.length() - 1; + int code = 0xFF & start; + code = (code << 8) | (0xFF & name.charAt(start)); + code = (code << 8) | (0xFF & name.charAt(end)); + return (code << 8) | (0xFF & (end - start)); + } + + private static int rehash(int oldHash) { + return Integer.reverseBytes(oldHash * 0x9e3775cd) * 0x9e3775cd; + } +} diff --git a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java index 754c3be5ff3..a0f971a5b43 100644 --- a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java +++ b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java @@ -288,6 +288,10 @@ public List getExcludedCodeSources() { return excludedCodeSources; } + public int getResolverNoMatchesSize() { + return resolverCacheConfig.noMatchesSize(); + } + public boolean isResolverMemoizingEnabled() { return resolverCacheConfig.memoPoolSize() > 0; } diff --git a/internal-api/src/main/java/datadog/trace/api/ResolverCacheConfig.java b/internal-api/src/main/java/datadog/trace/api/ResolverCacheConfig.java index f821d570a5e..7b9840a4479 100644 --- a/internal-api/src/main/java/datadog/trace/api/ResolverCacheConfig.java +++ b/internal-api/src/main/java/datadog/trace/api/ResolverCacheConfig.java @@ -5,27 +5,37 @@ public enum ResolverCacheConfig { /** Memoizing and outlining for large enterprise apps. */ LARGE { + @Override + public int noMatchesSize() { + return 65536; + } + @Override public int memoPoolSize() { - return 32768; + return 4096; } @Override public int outlinePoolSize() { - return 512; + return 256; } @Override public int typePoolSize() { - return 128; + return 64; } }, /** Memoizing and outlining for the average sized app. */ MEMOS { + @Override + public int noMatchesSize() { + return 16384; + } + @Override public int memoPoolSize() { - return 8192; + return 2048; } @Override @@ -41,6 +51,11 @@ public int typePoolSize() { /** Outlining only for the average sized app, no memoizing. */ NO_MEMOS { + @Override + public int noMatchesSize() { + return 0; + } + @Override public int memoPoolSize() { return 0; @@ -59,6 +74,11 @@ public int typePoolSize() { /** Outlining only for small microservice apps. */ SMALL { + @Override + public int noMatchesSize() { + return 0; + } + @Override public int memoPoolSize() { return 0; @@ -77,6 +97,11 @@ public int typePoolSize() { /** The old {@code DDCachingPoolStrategy} behaviour. */ LEGACY { + @Override + public int noMatchesSize() { + return 0; + } + @Override public int memoPoolSize() { return 0; @@ -93,6 +118,8 @@ public int typePoolSize() { } }; + public abstract int noMatchesSize(); + public abstract int memoPoolSize(); public abstract int outlinePoolSize(); From 9c63e5c245ddae45a009894b9bd4af72a39d8daf Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Sat, 15 Apr 2023 09:57:03 +0100 Subject: [PATCH 4/8] Provide option to persist NoMatchFilter when application exits and re-seed it when the application restarts --- .../bytebuddy/memoize/NoMatchFilter.java | 118 ++++++++++++++++++ .../config/TraceInstrumentationConfig.java | 1 + .../datadog/trace/api/InstrumenterConfig.java | 9 ++ 3 files changed, 128 insertions(+) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/NoMatchFilter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/NoMatchFilter.java index fcc2b7aae8d..95652b4aa97 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/NoMatchFilter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/NoMatchFilter.java @@ -1,7 +1,23 @@ package datadog.trace.agent.tooling.bytebuddy.memoize; +import static datadog.trace.util.AgentThreadFactory.AGENT_THREAD_GROUP; + +import datadog.trace.api.Config; +import datadog.trace.api.DDTraceApiInfo; import datadog.trace.api.InstrumenterConfig; +import java.io.BufferedInputStream; +import java.io.BufferedOutputStream; +import java.io.DataInputStream; +import java.io.DataOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Arrays; +import java.util.UUID; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Compact filter that records the hash and short 'class-code' for uninteresting types. @@ -12,6 +28,8 @@ * which would otherwise make the no-match filter overly large. */ final class NoMatchFilter { + private static final Logger log = LoggerFactory.getLogger(NoMatchFilter.class); + private static final int MAX_CAPACITY = 1 << 16; private static final int MIN_CAPACITY = 1 << 8; private static final int MAX_HASH_ATTEMPTS = 3; @@ -30,6 +48,12 @@ final class NoMatchFilter { // choose enough slot bits to cover the chosen capacity slotMask = 0xFFFFFFFF >>> Integer.numberOfLeadingZeros(capacity - 1); slots = new long[slotMask + 1]; + + // seed filter from previously collected results? + Path noMatchFile = discoverNoMatchFile(); + if (null != noMatchFile) { + seedNoMatchFilter(noMatchFile); + } } public static boolean contains(String name) { @@ -83,4 +107,98 @@ private static int classCode(String name) { private static int rehash(int oldHash) { return Integer.reverseBytes(oldHash * 0x9e3775cd) * 0x9e3775cd; } + + static Path discoverNoMatchFile() { + String cacheDir = InstrumenterConfig.get().getResolverCacheDir(); + if (null == cacheDir) { + return null; + } + + // use different file for each tracer + service combination + String filterKey = + DDTraceApiInfo.VERSION + + "/" + + Config.get().getServiceName() + + "/" + + Config.get().getVersion(); + + String noMatchFilterName = + UUID.nameUUIDFromBytes(filterKey.getBytes(StandardCharsets.UTF_8)) + "-nomatch.filter"; + + return Paths.get(cacheDir, noMatchFilterName); + } + + static void seedNoMatchFilter(Path noMatchFile) { + if (!Files.exists(noMatchFile)) { + Runtime.getRuntime().addShutdownHook(new ShutdownHook(noMatchFile)); + } else { + log.debug("Seeding NoMatchFilter from {}", noMatchFile); + try (DataInputStream in = + new DataInputStream(new BufferedInputStream(Files.newInputStream(noMatchFile)))) { + while (true) { + switch (in.readUTF()) { + case "dd-java-agent": + expectVersion(in, DDTraceApiInfo.VERSION); + break; + case "NoMatchFilter": + if (in.readInt() != slots.length) { + throw new IOException("filter size mismatch"); + } + for (int i = 0; i < slots.length; i++) { + slots[i] = in.readLong(); + } + return; + default: + throw new IOException("unexpected content"); + } + } + } catch (IOException e) { + if (log.isDebugEnabled()) { + log.info("Unable to seed NoMatchFilter from {}", noMatchFile, e); + } else { + log.info("Unable to seed NoMatchFilter from {}: {}", noMatchFile, e.getMessage()); + } + } + } + } + + static void persistNoMatchFilter(Path noMatchFile) { + log.debug("Persisting NoMatchFilter to {}", noMatchFile); + try (DataOutputStream out = + new DataOutputStream(new BufferedOutputStream(Files.newOutputStream(noMatchFile)))) { + out.writeUTF("dd-java-agent"); + out.writeUTF(DDTraceApiInfo.VERSION); + out.writeUTF("NoMatchFilter"); + out.writeInt(slots.length); + for (long slot : slots) { + out.writeLong(slot); + } + } catch (IOException e) { + if (log.isDebugEnabled()) { + log.info("Unable to persist NoMatchFilter to {}", noMatchFile, e); + } else { + log.info("Unable to persist NoMatchFilter to {}: {}", noMatchFile, e.getMessage()); + } + } + } + + static void expectVersion(DataInputStream in, String version) throws IOException { + if (!version.equals(in.readUTF())) { + throw new IOException("version mismatch"); + } + } + + static class ShutdownHook extends Thread { + private final Path noMatchFile; + + ShutdownHook(Path noMatchFile) { + super(AGENT_THREAD_GROUP, "dd-NoMatchFilter-persist-hook"); + this.noMatchFile = noMatchFile; + } + + @Override + public void run() { + persistNoMatchFilter(noMatchFile); + } + } } diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java index 0cf015b5ddc..695415034c4 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java @@ -105,6 +105,7 @@ public final class TraceInstrumentationConfig { "spring-data.repository.interface.resource-name"; public static final String RESOLVER_CACHE_CONFIG = "resolver.cache.config"; + public static final String RESOLVER_CACHE_DIR = "resolver.cache.dir"; public static final String RESOLVER_USE_LOADCLASS = "resolver.use.loadclass"; public static final String RESOLVER_RESET_INTERVAL = "resolver.reset.interval"; diff --git a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java index a0f971a5b43..900a3e87273 100644 --- a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java +++ b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java @@ -34,6 +34,7 @@ import static datadog.trace.api.config.TraceInstrumentationConfig.LOGS_MDC_TAGS_INJECTION_ENABLED; import static datadog.trace.api.config.TraceInstrumentationConfig.MEASURE_METHODS; import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_CACHE_CONFIG; +import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_CACHE_DIR; import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_RESET_INTERVAL; import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_USE_LOADCLASS; import static datadog.trace.api.config.TraceInstrumentationConfig.RUNTIME_CONTEXT_FIELD_INJECTION; @@ -92,6 +93,7 @@ public class InstrumenterConfig { private final List excludedCodeSources; private final ResolverCacheConfig resolverCacheConfig; + private final String resolverCacheDir; private final boolean resolverUseLoadClass; private final int resolverResetInterval; @@ -168,6 +170,7 @@ private InstrumenterConfig() { resolverCacheConfig = configProvider.getEnum( RESOLVER_CACHE_CONFIG, ResolverCacheConfig.class, ResolverCacheConfig.MEMOS); + resolverCacheDir = configProvider.getString(RESOLVER_CACHE_DIR); resolverUseLoadClass = configProvider.getBoolean(RESOLVER_USE_LOADCLASS, true); resolverResetInterval = Platform.isNativeImageBuilder() @@ -312,6 +315,10 @@ public int getResolverTypePoolSize() { return resolverCacheConfig.typePoolSize(); } + public String getResolverCacheDir() { + return resolverCacheDir; + } + public boolean isResolverUseLoadClass() { return resolverUseLoadClass; } @@ -413,6 +420,8 @@ public String toString() { + excludedCodeSources + ", resolverCacheConfig=" + resolverCacheConfig + + ", resolverCacheDir=" + + resolverCacheDir + ", resolverUseLoadClass=" + resolverUseLoadClass + ", resolverResetInterval=" From 67715b369ff827f10bddd45ddf85a21963c2c611 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Wed, 22 Mar 2023 22:13:20 +0000 Subject: [PATCH 5/8] Add 'dd.resolver.names.are.unique' property to allow simplification of memoization when we know type-names are unique. --- .../tooling/bytebuddy/memoize/Memoizer.java | 22 ++++++++++++++++--- .../config/TraceInstrumentationConfig.java | 1 + .../datadog/trace/api/InstrumenterConfig.java | 9 ++++++++ 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java index 7fbb933a660..29f83457a59 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java @@ -5,6 +5,7 @@ import datadog.trace.agent.tooling.bytebuddy.TypeInfoCache; import datadog.trace.agent.tooling.bytebuddy.TypeInfoCache.SharedTypeInfo; import datadog.trace.agent.tooling.bytebuddy.outline.TypePoolFacade; +import datadog.trace.agent.tooling.bytebuddy.outline.WithLocation; import datadog.trace.api.InstrumenterConfig; import datadog.trace.api.cache.DDCache; import datadog.trace.api.cache.DDCaches; @@ -56,9 +57,11 @@ enum MatcherKind { private static final DDCache memoizingMatcherCache = DDCaches.newFixedSizeIdentityCache(8); + private static final boolean namesAreUnique = InstrumenterConfig.get().isResolverNamesAreUnique(); + // caches positive memoized matches private static final TypeInfoCache memos = - new TypeInfoCache<>(InstrumenterConfig.get().getResolverMemoPoolSize(), true); + new TypeInfoCache<>(InstrumenterConfig.get().getResolverMemoPoolSize(), namesAreUnique); // local memoized results, used to detect circular references static final ThreadLocal> localMemosHolder = @@ -173,7 +176,9 @@ static BitSet doMemoize(TypeDescription type, Map localMemos) { SharedTypeInfo sharedMemo = memos.find(name); if (null != sharedMemo) { - return sharedMemo.get(); + if (namesAreUnique || name.startsWith("java.") || sameOrigin(type, sharedMemo)) { + return sharedMemo.get(); + } } localMemos.put(name, memo = new BitSet(matchers.size())); @@ -217,11 +222,22 @@ static BitSet doMemoize(TypeDescription type, Map localMemos) { return NO_MATCH; } - memos.share(name, null, null, memo); + if (namesAreUnique || name.startsWith("java.") || !(type instanceof WithLocation)) { + memos.share(name, null, null, memo); + } else { + WithLocation origin = (WithLocation) type; + memos.share(name, origin.getClassLoader(), origin.getClassFile(), memo); + } return memo; } + private static boolean sameOrigin(TypeDescription type, SharedTypeInfo sharedMemo) { + return !(type instanceof WithLocation) + || sharedMemo.sameClassLoader(((WithLocation) type).getClassLoader()) + || sharedMemo.sameClassFile(((WithLocation) type).getClassFile()); + } + /** Inherit positive matches from a super-class or interface. */ private static void inherit(BitSet superMemo, BitSet memo) { int matcherId = superMemo.nextSetBit(0); diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java index 695415034c4..1cac4b39254 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java @@ -108,6 +108,7 @@ public final class TraceInstrumentationConfig { public static final String RESOLVER_CACHE_DIR = "resolver.cache.dir"; public static final String RESOLVER_USE_LOADCLASS = "resolver.use.loadclass"; public static final String RESOLVER_RESET_INTERVAL = "resolver.reset.interval"; + public static final String RESOLVER_NAMES_ARE_UNIQUE = "resolver.names.are.unique"; private TraceInstrumentationConfig() {} } diff --git a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java index 900a3e87273..5741e221d44 100644 --- a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java +++ b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java @@ -35,6 +35,7 @@ import static datadog.trace.api.config.TraceInstrumentationConfig.MEASURE_METHODS; import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_CACHE_CONFIG; import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_CACHE_DIR; +import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_NAMES_ARE_UNIQUE; import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_RESET_INTERVAL; import static datadog.trace.api.config.TraceInstrumentationConfig.RESOLVER_USE_LOADCLASS; import static datadog.trace.api.config.TraceInstrumentationConfig.RUNTIME_CONTEXT_FIELD_INJECTION; @@ -94,6 +95,7 @@ public class InstrumenterConfig { private final ResolverCacheConfig resolverCacheConfig; private final String resolverCacheDir; + private final boolean resolverNamesAreUnique; private final boolean resolverUseLoadClass; private final int resolverResetInterval; @@ -171,6 +173,7 @@ private InstrumenterConfig() { configProvider.getEnum( RESOLVER_CACHE_CONFIG, ResolverCacheConfig.class, ResolverCacheConfig.MEMOS); resolverCacheDir = configProvider.getString(RESOLVER_CACHE_DIR); + resolverNamesAreUnique = configProvider.getBoolean(RESOLVER_NAMES_ARE_UNIQUE, false); resolverUseLoadClass = configProvider.getBoolean(RESOLVER_USE_LOADCLASS, true); resolverResetInterval = Platform.isNativeImageBuilder() @@ -319,6 +322,10 @@ public String getResolverCacheDir() { return resolverCacheDir; } + public boolean isResolverNamesAreUnique() { + return resolverNamesAreUnique; + } + public boolean isResolverUseLoadClass() { return resolverUseLoadClass; } @@ -422,6 +429,8 @@ public String toString() { + resolverCacheConfig + ", resolverCacheDir=" + resolverCacheDir + + ", resolverNamesAreUnique=" + + resolverNamesAreUnique + ", resolverUseLoadClass=" + resolverUseLoadClass + ", resolverResetInterval=" From 88a4f4af003a10c11dcadb05ca3d2dbc8efa1db9 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Tue, 9 May 2023 12:23:49 +0100 Subject: [PATCH 6/8] weakStoreIds is local to the caller and doesn't require synchronization --- .../agent/tooling/bytebuddy/memoize/HasContextField.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasContextField.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasContextField.java index 1c25981b63e..33b5d66c039 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasContextField.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/HasContextField.java @@ -39,9 +39,7 @@ boolean hasSuperStore(TypeDescription target, BitSet weakStoreIds) { if (hasSuperStore) { // report if super-class was skipped from field-injection if (null != skipMatcher && skipMatcher.matches(superTarget)) { - synchronized (weakStoreIds) { - weakStoreIds.or(skippableStoreIds); - } + weakStoreIds.or(skippableStoreIds); } } return hasSuperStore; From 64c752c9e4d05b7081dc157609e27bde124bc6e1 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Thu, 11 May 2023 09:35:51 +0100 Subject: [PATCH 7/8] Avoid caching memoization results based on partial type information --- .../tooling/bytebuddy/memoize/Memoizer.java | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java index 29f83457a59..83589d67dc9 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java @@ -67,16 +67,18 @@ enum MatcherKind { static final ThreadLocal> localMemosHolder = ThreadLocal.withInitial(HashMap::new); - private static final int INTERNAL_MATCHERS = 2; // isClass and isConcrete + private static final int INTERNAL_MATCHERS = 3; // isClass, isConcrete, isPartial // memoize whether the type is a class - static final ElementMatcher.Junction isClass = - prepare(MatcherKind.CLASS, ElementMatchers.any(), true); + static final MemoizingMatcher isClass = prepare(MatcherKind.CLASS, ElementMatchers.any(), true); // memoize whether the type is a concrete class, i.e. no abstract methods - static final ElementMatcher.Junction isConcrete = + static final MemoizingMatcher isConcrete = prepare(MatcherKind.CLASS, not(ElementMatchers.isAbstract()), false); + // memoize whether this is based on partial information due to missing types + static final MemoizingMatcher isPartial = prepare(MatcherKind.TYPE, ElementMatchers.none(), true); + public static void resetState() { // no need to reset the state if we haven't added any external matchers if (matchers.size() > INTERNAL_MATCHERS) { @@ -94,7 +96,7 @@ static MemoizingMatcher withMatcherId(ElementMatcher matcher) { } /** Prepares a matcher for memoization. */ - static ElementMatcher.Junction prepare( + static MemoizingMatcher prepare( MatcherKind kind, ElementMatcher.Junction matcher, boolean inherited) { MemoizingMatcher memoizingMatcher = @@ -202,6 +204,8 @@ static BitSet doMemoize(TypeDescription type, Map localMemos) { } record(type.isInterface() ? interfaceMatcherIds : classMatcherIds, type, memo); } catch (Throwable e) { + // we're missing some type information so record the result as partial + memo.set(isPartial.matcherId); if (log.isDebugEnabled()) { log.debug( "{} recording matches for type {}: {}", @@ -216,6 +220,11 @@ static BitSet doMemoize(TypeDescription type, Map localMemos) { } } + // don't cache the result if it's based on partial/incomplete type information + if (memo.get(isPartial.matcherId)) { + return memo; + } + // we're only interested in the type if there's at least one external match if (memo.nextSetBit(INTERNAL_MATCHERS) < 0) { NoMatchFilter.add(name); From a2e7761be682b2bf5cc9996fe54e4b0556293305 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Thu, 11 May 2023 10:21:55 +0100 Subject: [PATCH 8/8] Optimization: we can still share partial results in the location-based cache, we just need to avoid adding them to the simple 'no-match' filter --- .../agent/tooling/bytebuddy/memoize/Memoizer.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java index 83589d67dc9..09f612fd32b 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/memoize/Memoizer.java @@ -220,17 +220,13 @@ static BitSet doMemoize(TypeDescription type, Map localMemos) { } } - // don't cache the result if it's based on partial/incomplete type information - if (memo.get(isPartial.matcherId)) { - return memo; - } - - // we're only interested in the type if there's at least one external match - if (memo.nextSetBit(INTERNAL_MATCHERS) < 0) { + // update no-match filter if there's no interesting matches and result is complete + if (memo.nextSetBit(INTERNAL_MATCHERS) < 0 && !memo.get(isPartial.matcherId)) { NoMatchFilter.add(name); return NO_MATCH; } + // otherwise share result for this location (other locations may have different results) if (namesAreUnique || name.startsWith("java.") || !(type instanceof WithLocation)) { memos.share(name, null, null, memo); } else {