Skip to content

Fail fast in the matcher, let the debug outputs use the cache #1251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tylerbenson merged 8 commits into from
Feb 28, 2020
Merged

Fail fast in the matcher, let the debug outputs use the cache #1251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e65177a
Fail fast in the matcher, let the debug outputs use the cache
Feb 24, 2020
75af715
Refactor an agent usage, and fix logic issue
Feb 24, 2020
af67dfa
Need to cache the boolean. The list can be taken later as debug is ex…
Feb 24, 2020
5ebc13c
Increase performance on java 8
Feb 24, 2020
3b78d45
Don't need to collect mismatches in the critical path
Feb 25, 2020
2441ca3
No more arraylist(0)
Feb 25, 2020
815bd0e
Fix comment
Feb 25, 2020
30e1d56
Address comments
Feb 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,11 @@ public boolean matches(
*/
final ReferenceMatcher muzzle = getInstrumentationMuzzle();
if (null != muzzle) {
final List<Reference.Mismatch> mismatches =
muzzle.getMismatchedReferenceSources(classLoader);
if (mismatches.size() > 0) {
final boolean isMatch = muzzle.matches(classLoader);
if (!isMatch) {
if (log.isDebugEnabled()) {
final List<Reference.Mismatch> mismatches =
muzzle.getMismatchedReferenceSources(classLoader);
log.debug(
"Instrumentation muzzled: {} -- {} on {}",
instrumentationNames,
Expand All @@ -153,7 +154,7 @@ public boolean matches(
Instrumenter.Default.this.getClass().getName(),
classLoader);
}
return mismatches.size() == 0;
return isMatch;
}
return true;
}
Expand Down
82 changes: 50 additions & 32 deletions ...gent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@

/** Matches a set of references against a classloader. */
@Slf4j
public class ReferenceMatcher
implements WeakMap.ValueSupplier<ClassLoader, List<Reference.Mismatch>> {
private final WeakMap<ClassLoader, List<Reference.Mismatch>> mismatchCache = newWeakMap();
public final class ReferenceMatcher implements WeakMap.ValueSupplier<ClassLoader, Boolean> {
private final WeakMap<ClassLoader, Boolean> mismatchCache = newWeakMap();
private final Reference[] references;
private final Set<String> helperClassNames;
Copy link
Copy Markdown
Contributor

@lpriima lpriima Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this field always empty set ? where do we add elements to it ?

Copy link
Copy Markdown
Contributor

@dougqh dougqh Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that it is final, it must be assigned in the constructor.

Copy link
Copy Markdown
Contributor Author

@devinsba devinsba Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is set to a list of the args in the constructor(s)


Expand All @@ -42,14 +41,37 @@ public Reference[] getReferences() {
}

/**
* 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();
}

return mismatchCache.computeIfAbsent(loader, this);
}

@Override
public Boolean get(final ClassLoader loader) {
for (final Reference reference : references) {
// Don't reference-check helper classes.
// They will be injected by the instrumentation's HelperInjector.
if (!helperClassNames.contains(reference.getClassName())) {
if (!checkMatch(reference, loader).isEmpty()) {
return false;
}
}
Comment thread
devinsba marked this conversation as resolved.
}

return true;
}

/**
* Loads 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.
*/
Expand All @@ -58,12 +80,7 @@ public List<Reference.Mismatch> getMismatchedReferenceSources(ClassLoader loader
loader = Utils.getBootstrapProxy();
}

return mismatchCache.computeIfAbsent(loader, this);
}

@Override
public List<Mismatch> get(final ClassLoader loader) {
final List<Mismatch> mismatches = new ArrayList<>(0);
final List<Mismatch> mismatches = new ArrayList<>();

for (final Reference reference : references) {
// Don't reference-check helper classes.
Expand All @@ -82,11 +99,12 @@ public List<Mismatch> get(final ClassLoader loader) {
* @param loader
* @return A list of mismatched sources. A list of size 0 means the reference matches the class.
*/
public static List<Reference.Mismatch> checkMatch(Reference reference, ClassLoader loader) {
private static List<Reference.Mismatch> checkMatch(
final Reference reference, final ClassLoader loader) {
final TypePool typePool =
AgentTooling.poolStrategy()
.typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader);
final List<Mismatch> mismatches = new ArrayList<>(0);
final List<Mismatch> mismatches = new ArrayList<>();
try {
final TypePool.Resolution resolution =
typePool.describe(Utils.getClassName(reference.getClassName()));
Expand All @@ -96,7 +114,7 @@ public static List<Reference.Mismatch> checkMatch(Reference reference, ClassLoad
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
Expand All @@ -112,10 +130,10 @@ public static List<Reference.Mismatch> checkMatch(Reference reference, ClassLoad
}

public static List<Reference.Mismatch> checkMatch(
Reference reference, TypeDescription typeOnClasspath) {
final List<Mismatch> mismatches = new ArrayList<>(0);
final Reference reference, final TypeDescription typeOnClasspath) {
final List<Mismatch> mismatches = new ArrayList<>();

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(
Expand All @@ -127,8 +145,8 @@ public static List<Reference.Mismatch> checkMatch(
}
}

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(
Expand All @@ -137,7 +155,7 @@ public static List<Reference.Mismatch> checkMatch(
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()
Expand All @@ -155,7 +173,7 @@ public static List<Reference.Mismatch> checkMatch(
}
}

for (Reference.Method methodRef : reference.getMethods()) {
for (final Reference.Method methodRef : reference.getMethods()) {
final MethodDescription.InDefinedShape methodDescription =
findMethod(methodRef, typeOnClasspath);
if (methodDescription == null) {
Expand All @@ -165,7 +183,7 @@ public static List<Reference.Mismatch> checkMatch(
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();
Expand All @@ -184,8 +202,8 @@ public static List<Reference.Mismatch> checkMatch(
}

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()
Expand All @@ -196,14 +214,14 @@ private static FieldDescription.InDefinedShape findField(
}
}
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;
Expand All @@ -213,23 +231,23 @@ private static FieldDescription.InDefinedShape findField(
}

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())) {
return methodDescription;
}
}
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;
Expand Down