ClassLoaderMatcher put to Guava Cache outside critical section

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java

@@ -1,17 +1,17 @@
 package datadog.trace.agent.tooling;
 
+import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
 import datadog.trace.bootstrap.PatchLogger;
 import io.opentracing.util.GlobalTracer;
-import java.util.concurrent.ExecutionException;
 import lombok.extern.slf4j.Slf4j;
 import net.bytebuddy.matcher.ElementMatcher;
 
 @Slf4j
-public class ClassLoaderMatcher {
+public final class ClassLoaderMatcher {
   public static final ClassLoader BOOTSTRAP_CLASSLOADER = null;
+  public static final int CACHE_CONCURRENCY =
+      Math.max(8, Runtime.getRuntime().availableProcessors());
 
   /** A private constructor that must not be invoked. */
   private ClassLoaderMatcher() {
@@ -22,37 +22,35 @@ public static ElementMatcher.Junction.AbstractBase<ClassLoader> skipClassLoader(
     return SkipClassLoaderMatcher.INSTANCE;
   }
 
-  public static ElementMatcher.Junction.AbstractBase<ClassLoader> classLoaderHasClasses(
-      final String... names) {
-    return new ClassLoaderHasClassMatcher(names);
+  public static ElementMatcher.Junction.AbstractBase<ClassLoader> classLoaderHasNoResources(
+      final String... resources) {
+    return new ClassLoaderHasNoResourceMatcher(resources);
   }
 
   private static final class SkipClassLoaderMatcher
       extends ElementMatcher.Junction.AbstractBase<ClassLoader> {
     public static final SkipClassLoaderMatcher INSTANCE = new SkipClassLoaderMatcher();
     /* Cache of classloader-instance -> (true|false). True = skip instrumentation. False = safe to instrument. */
-    private static final LoadingCache<ClassLoader, Boolean> skipCache =
-        CacheBuilder.newBuilder()
-            .weakKeys()
-            .build(
-                new CacheLoader<ClassLoader, Boolean>() {
-                  @Override
-                  public Boolean load(ClassLoader loader) {
-                    return !delegatesToBootstrap(loader);
-                  }
-                });
+    private static final Cache<ClassLoader, Boolean> skipCache =
+        CacheBuilder.newBuilder().weakKeys().concurrencyLevel(CACHE_CONCURRENCY).build();
     private static final String DATADOG_CLASSLOADER_NAME =
         "datadog.trace.bootstrap.DatadogClassLoader";
 
     private SkipClassLoaderMatcher() {}
 
     @Override
-    public boolean matches(final ClassLoader target) {
-      if (target == BOOTSTRAP_CLASSLOADER) {
+    public boolean matches(final ClassLoader cl) {
+      if (cl == BOOTSTRAP_CLASSLOADER) {
         // Don't skip bootstrap loader
         return false;
       }
-      return shouldSkipClass(target) || shouldSkipInstance(target);
+      Boolean v = skipCache.getIfPresent(cl);
+      if (v != null) {
+        return v;
+      }
+      v = shouldSkipClass(cl) || !delegatesToBootstrap(cl);
+      skipCache.put(cl, v);
+      return v;
     }
 
     private static boolean shouldSkipClass(final ClassLoader loader) {
@@ -69,15 +67,6 @@ private static boolean shouldSkipClass(final ClassLoader loader) {
       return false;
     }
 
-    private static boolean shouldSkipInstance(final ClassLoader loader) {
-      try {
-        return skipCache.get(loader);
-      } catch (ExecutionException e) {
-        log.warn("Exception while getting from cache", e);
-      }
-      return false;
-    }
-
     /**
      * TODO: this turns out to be useless with OSGi: {@code
      * org.eclipse.osgi.internal.loader.BundleLoader#isRequestFromVM} returns {@code true} when
@@ -107,41 +96,38 @@ private static boolean loadsExpectedClass(
     }
   }
 
-  public static class ClassLoaderHasClassMatcher
+  private static class ClassLoaderHasNoResourceMatcher
       extends ElementMatcher.Junction.AbstractBase<ClassLoader> {
+    private final Cache<ClassLoader, Boolean> cache =
+        CacheBuilder.newBuilder().weakKeys().concurrencyLevel(CACHE_CONCURRENCY).build();
 
-    private final LoadingCache<ClassLoader, Boolean> cache =
-        CacheBuilder.newBuilder()
-            .weakKeys()
-            .build(
-                new CacheLoader<ClassLoader, Boolean>() {
-                  @Override
-                  public Boolean load(ClassLoader cl) {
-                    for (final String name : names) {
-                      if (cl.getResource(Utils.getResourceName(name)) == null) {
-                        return false;
-                      }
-                    }
-                    return true;
-                  }
-                });
-
-    private final String[] names;
-
-    private ClassLoaderHasClassMatcher(final String... names) {
-      this.names = names;
+    private final String[] resources;
+
+    private ClassLoaderHasNoResourceMatcher(final String... resources) {
+      this.resources = resources;
     }
 
-    @Override
-    public boolean matches(final ClassLoader target) {
-      if (target != null) {
-        try {
-          return cache.get(target);
-        } catch (ExecutionException e) {
-          log.warn("Can't get from cache", e);
+    private boolean hasNoResources(ClassLoader cl) {
+      for (final String resource : resources) {
+        if (cl.getResource(resource) == null) {
+          return true;
         }
       }
       return false;
     }
+
+    @Override
+    public boolean matches(final ClassLoader cl) {
+      if (cl == null) {
+        return false;
+      }
+      Boolean v = cache.getIfPresent(cl);
+      if (v != null) {
+        return v;
+      }
+      v = hasNoResources(cl);
+      cache.put(cl, v);
+      return v;
+    }
   }
 }

dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsInstrumentation.java

@@ -1,6 +1,6 @@
 package datadog.trace.instrumentation.jaxrs1;
 
-import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClasses;
+import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasNoResources;
 import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.hasSuperMethod;
 import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.safeHasSuperType;
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan;
@@ -12,7 +12,6 @@
 import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.named;
-import static net.bytebuddy.matcher.ElementMatchers.not;
 
 import com.google.auto.service.AutoService;
 import datadog.trace.agent.tooling.Instrumenter;
@@ -37,7 +36,7 @@ public JaxRsAnnotationsInstrumentation() {
   // this is required to make sure instrumentation won't apply to jax-rs 2
   @Override
   public ElementMatcher<ClassLoader> classLoaderMatcher() {
-    return not(classLoaderHasClasses("javax.ws.rs.container.AsyncResponse"));
+    return classLoaderHasNoResources("javax/ws/rs/container/AsyncResponse.class");
   }
 
   @Override

dd-java-agent/instrumentation/jedis-1.4/src/main/java/datadog/trace/instrumentation/jedis/JedisInstrumentation.java

@@ -1,14 +1,13 @@
 package datadog.trace.instrumentation.jedis;
 
-import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClasses;
+import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasNoResources;
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan;
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan;
 import static datadog.trace.instrumentation.jedis.JedisClientDecorator.DECORATE;
 import static java.util.Collections.singletonMap;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.isPublic;
 import static net.bytebuddy.matcher.ElementMatchers.named;
-import static net.bytebuddy.matcher.ElementMatchers.not;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
 import com.google.auto.service.AutoService;
@@ -34,7 +33,7 @@ public JedisInstrumentation() {
 
   @Override
   public ElementMatcher<ClassLoader> classLoaderMatcher() {
-    return not(classLoaderHasClasses("redis.clients.jedis.commands.ProtocolCommand"));
+    return classLoaderHasNoResources("redis/clients/jedis/commands/ProtocolCommand.class");
   }
 
   @Override

dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Instrumentation.java

@@ -1,6 +1,6 @@
 package datadog.trace.instrumentation.servlet2;
 
-import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClasses;
+import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasNoResources;
 import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.safeHasSuperType;
 import static java.util.Collections.singletonMap;
 import static net.bytebuddy.matcher.ElementMatchers.isPublic;
@@ -25,7 +25,8 @@ public Servlet2Instrumentation() {
   // this is required to make sure servlet 2 instrumentation won't apply to servlet 3
   @Override
   public ElementMatcher<ClassLoader> classLoaderMatcher() {
-    return not(classLoaderHasClasses("javax.servlet.AsyncEvent", "javax.servlet.AsyncListener"));
+    return classLoaderHasNoResources(
+        "javax/servlet/AsyncEvent.class", "javax/servlet/AsyncListener.class");
   }
 
   @Override