From e65177a143bc75e1015cdf6f241465a6679fe4b4 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 24 Feb 2020 10:43:55 -0500 Subject: [PATCH 1/8] Fail fast in the matcher, let the debug outputs use the cache --- .../tooling/muzzle/ReferenceMatcher.java | 65 +++++++++++++------ 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index c669646024..bfc9bed9d6 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -42,14 +42,36 @@ public class ReferenceMatcher } /** + * Matcher used by ByteBuddy. Fails fast and only caches empty results, or complete results + * * @param loader Classloader to validate against (or null for bootstrap) * @return true if all references match the classpath of loader */ - public boolean matches(final ClassLoader loader) { - return getMismatchedReferenceSources(loader).isEmpty(); + public boolean matches(ClassLoader loader) { + if (loader == BOOTSTRAP_LOADER) { + loader = Utils.getBootstrapProxy(); + } + + final List mismatches = new ArrayList<>(0); + + for (final Reference reference : references) { + if (mismatches.size() > 0) { + return false; + } + // Don't reference-check helper classes. + // They will be injected by the instrumentation's HelperInjector. + if (!helperClassNames.contains(reference.getClassName())) { + mismatches.addAll(checkMatch(reference, loader)); + } + } + + mismatchCache.put(loader, mismatches); + return true; } /** + * Loads and caches the full list of mismatches. Used in debug contexts only + * * @param loader Classloader to validate against (or null for bootstrap) * @return A list of all mismatches between this ReferenceMatcher and loader's classpath. */ @@ -82,7 +104,8 @@ public class ReferenceMatcher * @param loader * @return A list of mismatched sources. A list of size 0 means the reference matches the class. */ - public static List checkMatch(Reference reference, ClassLoader loader) { + public static List checkMatch( + final Reference reference, final ClassLoader loader) { final TypePool typePool = AgentTooling.poolStrategy() .typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader); @@ -96,7 +119,7 @@ public class ReferenceMatcher reference.getSources().toArray(new Source[0]), reference.getClassName())); } return checkMatch(reference, resolution.resolve()); - } catch (Exception e) { + } catch (final Exception e) { if (e.getMessage().startsWith("Cannot resolve type description for ")) { // bytebuddy throws an illegal state exception with this message if it cannot resolve types // TODO: handle missing type resolutions without catching bytebuddy's exceptions @@ -112,10 +135,10 @@ public class ReferenceMatcher } public static List checkMatch( - Reference reference, TypeDescription typeOnClasspath) { + final Reference reference, final TypeDescription typeOnClasspath) { final List mismatches = new ArrayList<>(0); - for (Reference.Flag flag : reference.getFlags()) { + for (final Reference.Flag flag : reference.getFlags()) { if (!flag.matches(typeOnClasspath.getModifiers())) { final String desc = reference.getClassName(); mismatches.add( @@ -127,8 +150,8 @@ public class ReferenceMatcher } } - for (Reference.Field fieldRef : reference.getFields()) { - FieldDescription.InDefinedShape fieldDescription = findField(fieldRef, typeOnClasspath); + for (final Reference.Field fieldRef : reference.getFields()) { + final FieldDescription.InDefinedShape fieldDescription = findField(fieldRef, typeOnClasspath); if (fieldDescription == null) { mismatches.add( new Reference.Mismatch.MissingField( @@ -137,7 +160,7 @@ public class ReferenceMatcher fieldRef.getName(), fieldRef.getType().getInternalName())); } else { - for (Reference.Flag flag : fieldRef.getFlags()) { + for (final Reference.Flag flag : fieldRef.getFlags()) { if (!flag.matches(fieldDescription.getModifiers())) { final String desc = reference.getClassName() @@ -155,7 +178,7 @@ public class ReferenceMatcher } } - for (Reference.Method methodRef : reference.getMethods()) { + for (final Reference.Method methodRef : reference.getMethods()) { final MethodDescription.InDefinedShape methodDescription = findMethod(methodRef, typeOnClasspath); if (methodDescription == null) { @@ -165,7 +188,7 @@ public class ReferenceMatcher methodRef.getName(), methodRef.getDescriptor())); } else { - for (Reference.Flag flag : methodRef.getFlags()) { + for (final Reference.Flag flag : methodRef.getFlags()) { if (!flag.matches(methodDescription.getModifiers())) { final String desc = reference.getClassName() + "#" + methodRef.getName() + methodRef.getDescriptor(); @@ -184,8 +207,8 @@ public class ReferenceMatcher } private static FieldDescription.InDefinedShape findField( - Reference.Field fieldRef, TypeDescription typeOnClasspath) { - for (FieldDescription.InDefinedShape fieldType : typeOnClasspath.getDeclaredFields()) { + final Reference.Field fieldRef, final TypeDescription typeOnClasspath) { + for (final FieldDescription.InDefinedShape fieldType : typeOnClasspath.getDeclaredFields()) { if (fieldType.getName().equals(fieldRef.getName()) && fieldType .getType() @@ -196,14 +219,14 @@ public class ReferenceMatcher } } if (typeOnClasspath.getSuperClass() != null) { - FieldDescription.InDefinedShape fieldOnSupertype = + final FieldDescription.InDefinedShape fieldOnSupertype = findField(fieldRef, typeOnClasspath.getSuperClass().asErasure()); if (fieldOnSupertype != null) { return fieldOnSupertype; } } - for (TypeDescription.Generic interfaceType : typeOnClasspath.getInterfaces()) { - FieldDescription.InDefinedShape fieldOnSupertype = + for (final TypeDescription.Generic interfaceType : typeOnClasspath.getInterfaces()) { + final FieldDescription.InDefinedShape fieldOnSupertype = findField(fieldRef, interfaceType.asErasure()); if (fieldOnSupertype != null) { return fieldOnSupertype; @@ -213,8 +236,8 @@ public class ReferenceMatcher } private static MethodDescription.InDefinedShape findMethod( - Reference.Method methodRef, TypeDescription typeOnClasspath) { - for (MethodDescription.InDefinedShape methodDescription : + final Reference.Method methodRef, final TypeDescription typeOnClasspath) { + for (final MethodDescription.InDefinedShape methodDescription : typeOnClasspath.getDeclaredMethods()) { if (methodDescription.getInternalName().equals(methodRef.getName()) && methodDescription.getDescriptor().equals(methodRef.getDescriptor())) { @@ -222,14 +245,14 @@ public class ReferenceMatcher } } if (typeOnClasspath.getSuperClass() != null) { - MethodDescription.InDefinedShape methodOnSupertype = + final MethodDescription.InDefinedShape methodOnSupertype = findMethod(methodRef, typeOnClasspath.getSuperClass().asErasure()); if (methodOnSupertype != null) { return methodOnSupertype; } } - for (TypeDescription.Generic interfaceType : typeOnClasspath.getInterfaces()) { - MethodDescription.InDefinedShape methodOnSupertype = + for (final TypeDescription.Generic interfaceType : typeOnClasspath.getInterfaces()) { + final MethodDescription.InDefinedShape methodOnSupertype = findMethod(methodRef, interfaceType.asErasure()); if (methodOnSupertype != null) { return methodOnSupertype; From 75af71584c5abf2ddca9139d52083f862bc2b1d9 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 24 Feb 2020 13:11:10 -0500 Subject: [PATCH 2/8] Refactor an agent usage, and fix logic issue --- .../java/datadog/trace/agent/tooling/Instrumenter.java | 9 +++++---- .../trace/agent/tooling/muzzle/ReferenceMatcher.java | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index f532086ae4..294724a1e3 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -133,9 +133,10 @@ public interface Instrumenter { */ final ReferenceMatcher muzzle = getInstrumentationMuzzle(); if (null != muzzle) { - final List mismatches = - muzzle.getMismatchedReferenceSources(classLoader); - if (mismatches.size() > 0) { + final boolean isMatch = muzzle.matches(classLoader); + if (!isMatch) { + final List mismatches = + muzzle.getMismatchedReferenceSources(classLoader); if (log.isDebugEnabled()) { log.debug( "Instrumentation muzzled: {} -- {} on {}", @@ -153,7 +154,7 @@ public interface Instrumenter { Instrumenter.Default.this.getClass().getName(), classLoader); } - return mismatches.size() == 0; + return isMatch; } return true; } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index bfc9bed9d6..4997dc98d4 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -66,7 +66,7 @@ public class ReferenceMatcher } mismatchCache.put(loader, mismatches); - return true; + return mismatches.size() == 0; } /** From af67dfa72034ed4b4f4daa605380be5654e534e2 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 24 Feb 2020 14:38:10 -0500 Subject: [PATCH 3/8] Need to cache the boolean. The list can be taken later as debug is expected to be slower --- .../trace/agent/tooling/Instrumenter.java | 4 ++-- .../agent/tooling/muzzle/ReferenceMatcher.java | 16 +++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 294724a1e3..57a7d6a666 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -135,9 +135,9 @@ public interface Instrumenter { if (null != muzzle) { final boolean isMatch = muzzle.matches(classLoader); if (!isMatch) { - final List mismatches = - muzzle.getMismatchedReferenceSources(classLoader); if (log.isDebugEnabled()) { + final List mismatches = + muzzle.getMismatchedReferenceSources(classLoader); log.debug( "Instrumentation muzzled: {} -- {} on {}", instrumentationNames, diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index 4997dc98d4..b86acfbc3e 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -22,9 +22,8 @@ import net.bytebuddy.pool.TypePool; /** Matches a set of references against a classloader. */ @Slf4j -public class ReferenceMatcher - implements WeakMap.ValueSupplier> { - private final WeakMap> mismatchCache = newWeakMap(); +public class ReferenceMatcher implements WeakMap.ValueSupplier { + private final WeakMap mismatchCache = newWeakMap(); private final Reference[] references; private final Set helperClassNames; @@ -52,6 +51,11 @@ public class ReferenceMatcher loader = Utils.getBootstrapProxy(); } + return mismatchCache.computeIfAbsent(loader, this); + } + + @Override + public Boolean get(final ClassLoader loader) { final List mismatches = new ArrayList<>(0); for (final Reference reference : references) { @@ -65,7 +69,6 @@ public class ReferenceMatcher } } - mismatchCache.put(loader, mismatches); return mismatches.size() == 0; } @@ -80,11 +83,6 @@ public class ReferenceMatcher loader = Utils.getBootstrapProxy(); } - return mismatchCache.computeIfAbsent(loader, this); - } - - @Override - public List get(final ClassLoader loader) { final List mismatches = new ArrayList<>(0); for (final Reference reference : references) { From 5ebc13cafa89038fb3d7bf388124d98ceff133ca Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 24 Feb 2020 17:10:41 -0500 Subject: [PATCH 4/8] Increase performance on java 8 --- .../datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index b86acfbc3e..bad1e3c928 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -56,7 +56,7 @@ public class ReferenceMatcher implements WeakMap.ValueSupplier mismatches = new ArrayList<>(0); + final List mismatches = new ArrayList<>(); for (final Reference reference : references) { if (mismatches.size() > 0) { From 3b78d4593fbf559b50369a360f078bd0cebfa8fa Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Tue, 25 Feb 2020 09:36:00 -0500 Subject: [PATCH 5/8] Don't need to collect mismatches in the critical path --- .../trace/agent/tooling/muzzle/ReferenceMatcher.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index bad1e3c928..828fb46ef5 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -56,20 +56,17 @@ public class ReferenceMatcher implements WeakMap.ValueSupplier mismatches = new ArrayList<>(); - for (final Reference reference : references) { - if (mismatches.size() > 0) { - return false; - } // Don't reference-check helper classes. // They will be injected by the instrumentation's HelperInjector. if (!helperClassNames.contains(reference.getClassName())) { - mismatches.addAll(checkMatch(reference, loader)); + if (!checkMatch(reference, loader).isEmpty()) { + return false; + } } } - return mismatches.size() == 0; + return true; } /** From 2441ca3332cf856d69599642c1decb85811bb49b Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Tue, 25 Feb 2020 10:02:53 -0500 Subject: [PATCH 6/8] No more arraylist(0) --- .../trace/agent/tooling/muzzle/ReferenceMatcher.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index 828fb46ef5..893c53fa27 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -80,7 +80,7 @@ public class ReferenceMatcher implements WeakMap.ValueSupplier mismatches = new ArrayList<>(0); + final List mismatches = new ArrayList<>(); for (final Reference reference : references) { // Don't reference-check helper classes. @@ -104,7 +104,7 @@ public class ReferenceMatcher implements WeakMap.ValueSupplier mismatches = new ArrayList<>(0); + final List mismatches = new ArrayList<>(); try { final TypePool.Resolution resolution = typePool.describe(Utils.getClassName(reference.getClassName())); @@ -131,7 +131,7 @@ public class ReferenceMatcher implements WeakMap.ValueSupplier checkMatch( final Reference reference, final TypeDescription typeOnClasspath) { - final List mismatches = new ArrayList<>(0); + final List mismatches = new ArrayList<>(); for (final Reference.Flag flag : reference.getFlags()) { if (!flag.matches(typeOnClasspath.getModifiers())) { From 815bd0e9f65e5ddc7a3c4acfb2f9f11f85bc159c Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Tue, 25 Feb 2020 10:31:52 -0500 Subject: [PATCH 7/8] Fix comment --- .../datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index 893c53fa27..5f9636e6fb 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -70,7 +70,7 @@ public class ReferenceMatcher implements WeakMap.ValueSupplier Date: Wed, 26 Feb 2020 09:14:49 -0500 Subject: [PATCH 8/8] Address comments --- .../datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index 5f9636e6fb..d16556c9f6 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -22,7 +22,7 @@ import net.bytebuddy.pool.TypePool; /** Matches a set of references against a classloader. */ @Slf4j -public class ReferenceMatcher implements WeakMap.ValueSupplier { +public final class ReferenceMatcher implements WeakMap.ValueSupplier { private final WeakMap mismatchCache = newWeakMap(); private final Reference[] references; private final Set helperClassNames; @@ -99,7 +99,7 @@ public class ReferenceMatcher implements WeakMap.ValueSupplier checkMatch( + private static List checkMatch( final Reference reference, final ClassLoader loader) { final TypePool typePool = AgentTooling.poolStrategy()