Merge pull request #6152 from robolectric/piper_351251828

Avoid ClassNode construction for classes that do not get instrumented
robolectric · Jan 16, 2021 · 98d1eca · 98d1eca
2 parents ba1331d + c2ebe87
commit 98d1eca
Show file tree

Hide file tree

Showing 11 changed files with 108 additions and 58 deletions.
diff --git a/buildSrc/build.gradle b/buildSrc/build.gradle
@@ -10,6 +10,6 @@ dependencies {
     implementation gradleApi()
     implementation localGroovy()
 
-    implementation "org.ow2.asm:asm-tree:7.3.1"
+    implementation "org.ow2.asm:asm-tree:9.0"
     implementation 'com.android.tools.build:gradle:3.5.3'
 }
diff --git a/processor/build.gradle b/processor/build.gradle
@@ -32,8 +32,8 @@ dependencies {
     api project(":shadowapi")
 
     compileOnly "com.google.code.findbugs:jsr305:3.0.2"
-    api "org.ow2.asm:asm:7.3.1"
-    api "org.ow2.asm:asm-commons:7.3.1"
+    api "org.ow2.asm:asm:9.0"
+    api "org.ow2.asm:asm-commons:9.0"
     api "com.google.guava:guava:27.0.1-jre"
     api "com.google.code.gson:gson:2.8.2"
 

diff --git a/...ric/src/test/java/org/robolectric/internal/bytecode/InstrumentationConfigurationTest.java b/...ric/src/test/java/org/robolectric/internal/bytecode/InstrumentationConfigurationTest.java
@@ -43,7 +43,8 @@ public void shouldNotInstrumentCoreJdkClasses() throws Exception {
   @Test
   public void shouldInstrumentAndroidCoreClasses() throws Exception {
     assertThat(config.shouldInstrument(wrap("android.content.Intent"))).isTrue();
-    assertThat(config.shouldInstrument(wrap("android.and.now.for.something.completely.different"))).isTrue();
+    assertThat(config.shouldInstrument(wrap("android.and.now.for.something.completely.different")))
+        .isTrue();
   }
 
   @Test
@@ -79,7 +80,8 @@ public void shouldNotAcquireExcludedPackages() throws Exception {
     assertThat(config.shouldAcquire("scala.Test")).isFalse();
     assertThat(config.shouldAcquire("scala.util.Test")).isFalse();
     assertThat(config.shouldAcquire("org.specs2.whatever.foo")).isFalse();
-    assertThat(config.shouldAcquire("com.almworks.sqlite4java.whatever.Cls$anything$else")).isFalse();
+    assertThat(config.shouldAcquire("com.almworks.sqlite4java.whatever.Cls$anything$else"))
+        .isFalse();
   }
 
   @Test
@@ -178,8 +180,8 @@ public void shouldNotInstrumentClassNamesWithMultiRegex() throws Exception {
 
   }
 
-  private MutableClass wrap(final String className) {
-    MutableClass info = mock(MutableClass.class);
+  private static ClassDetails wrap(final String className) {
+    ClassDetails info = mock(ClassDetails.class);
     when(info.getName()).thenReturn(className);
     return info;
   }

diff --git a/sandbox/build.gradle b/sandbox/build.gradle
@@ -13,8 +13,8 @@ dependencies {
     api "javax.annotation:javax.annotation-api:1.3.2"
     api "javax.inject:javax.inject:1"
 
-    api "org.ow2.asm:asm:7.3.1"
-    api "org.ow2.asm:asm-commons:7.3.1"
+    api "org.ow2.asm:asm:9.0"
+    api "org.ow2.asm:asm-commons:9.0"
     api "com.google.guava:guava:27.0.1-jre"
     compileOnly "com.google.code.findbugs:jsr305:3.0.2"
 

diff --git a/sandbox/src/main/java/org/robolectric/internal/bytecode/ClassDetails.java b/sandbox/src/main/java/org/robolectric/internal/bytecode/ClassDetails.java
@@ -0,0 +1,64 @@
+package org.robolectric.internal.bytecode;
+
+import java.lang.annotation.Annotation;
+import java.util.HashSet;
+import java.util.Set;
+import org.objectweb.asm.ClassReader;
+import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.Opcodes;
+
+/**
+ * A more lightweight variant of {@link MutableClass}. This lets you check for basic metadata like
+ * class name, interfaces, and annotation info by wrapping a {@link ClassReader}, which is
+ * significantly faster than a {@link org.objectweb.asm.tree.ClassNode} object.
+ */
+public class ClassDetails {
+  private final ClassReader classReader;
+  private final String className;
+  private Set<String> annotations;
+
+  public ClassDetails(byte[] classBytes) {
+    this.classReader = new ClassReader(classBytes);
+    this.className = classReader.getClassName().replace('/', '.');
+  }
+
+  public boolean isInterface() {
+    return (classReader.getAccess() & Opcodes.ACC_INTERFACE) != 0;
+  }
+
+  public boolean isAnnotation() {
+    return (classReader.getAccess() & Opcodes.ACC_ANNOTATION) != 0;
+  }
+
+  public String getName() {
+    return className;
+  }
+
+  public boolean hasAnnotation(Class<? extends Annotation> annotationClass) {
+    if (annotations == null) {
+      this.annotations = new HashSet<>();
+      classReader.accept(
+          new AnnotationVisitor(annotations),
+          ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES);
+    }
+    String internalName = "L" + annotationClass.getName().replace('.', '/') + ";";
+    return this.annotations.contains(internalName);
+  }
+
+  private static class AnnotationVisitor extends ClassVisitor {
+    private final Set<String> annotations;
+
+    public AnnotationVisitor(Set<String> annotations) {
+      super(Opcodes.ASM9);
+      this.annotations = annotations;
+    }
+
+    @Override
+    public org.objectweb.asm.AnnotationVisitor visitAnnotation(String descriptor, boolean visible) {
+      if (visible) {
+        annotations.add(descriptor);
+      }
+      return null;
+    }
+  }
+}
diff --git a/sandbox/src/main/java/org/robolectric/internal/bytecode/ClassInstrumentor.java b/sandbox/src/main/java/org/robolectric/internal/bytecode/ClassInstrumentor.java
@@ -31,6 +31,7 @@
 import org.objectweb.asm.tree.MethodNode;
 import org.objectweb.asm.tree.TypeInsnNode;
 import org.objectweb.asm.tree.VarInsnNode;
+import org.robolectric.util.PerfStatsCollector;
 
 public abstract class ClassInstrumentor {
   private static final String ROBO_INIT_METHOD_NAME = "$$robo$init";
@@ -47,7 +48,7 @@ protected ClassInstrumentor(Decorator decorator) {
     this.dumpClassesDirectory = System.getProperty(DUMP_CLASSES_PROPERTY, "");
   }
 
-  public MutableClass analyzeClass(
+  private MutableClass analyzeClass(
       byte[] origClassBytes,
       final InstrumentationConfiguration config,
       ClassNodeProvider classNodeProvider) {
@@ -96,10 +97,13 @@ public String map(final String internalName) {
     return classBytes;
   }
 
-  public byte[] instrument(byte[] origBytes, InstrumentationConfiguration config,
-      ClassNodeProvider classNodeProvider) {
-    MutableClass mutableClass = analyzeClass(origBytes, config, classNodeProvider);
-    return instrumentToBytes(mutableClass);
+  public byte[] instrument(
+      byte[] origBytes, InstrumentationConfiguration config, ClassNodeProvider classNodeProvider) {
+    PerfStatsCollector perfStats = PerfStatsCollector.getInstance();
+    MutableClass mutableClass =
+        perfStats.measure(
+            "analyze class", () -> analyzeClass(origBytes, config, classNodeProvider));
+    return perfStats.measure("instrument class", () -> instrumentToBytes(mutableClass));
   }
 
   public void instrument(MutableClass mutableClass) {

diff --git a/sandbox/src/main/java/org/robolectric/internal/bytecode/InstrumentationConfiguration.java b/sandbox/src/main/java/org/robolectric/internal/bytecode/InstrumentationConfiguration.java
@@ -79,19 +79,19 @@ protected InstrumentationConfiguration(
   /**
    * Determine if {@link SandboxClassLoader} should instrument a given class.
    *
-   * @param   mutableClass The class to check.
-   * @return  True if the class should be instrumented.
+   * @param classDetails The class to check.
+   * @return True if the class should be instrumented.
    */
-  public boolean shouldInstrument(MutableClass mutableClass) {
-    return !(mutableClass.isInterface()
-            || mutableClass.isAnnotation()
-            || mutableClass.hasAnnotation(DoNotInstrument.class))
-        && (isInInstrumentedPackage(mutableClass.getName())
-            || instrumentedClasses.contains(mutableClass.getName())
-            || mutableClass.hasAnnotation(Instrument.class))
-        && !(classesToNotInstrument.contains(mutableClass.getName()))
-        && !(isInPackagesToNotInstrument(mutableClass.getName()))
-        && !classMatchesExclusionRegex(mutableClass.getName());
+  public boolean shouldInstrument(ClassDetails classDetails) {
+    return !classDetails.isInterface()
+        && !classDetails.isAnnotation()
+        && !classesToNotInstrument.contains(classDetails.getName())
+        && !isInPackagesToNotInstrument(classDetails.getName())
+        && !classMatchesExclusionRegex(classDetails.getName())
+        && !classDetails.hasAnnotation(DoNotInstrument.class)
+        && (isInInstrumentedPackage(classDetails.getName())
+            || instrumentedClasses.contains(classDetails.getName())
+            || classDetails.hasAnnotation(Instrument.class));
   }
 
   private boolean classMatchesExclusionRegex(String className) {
@@ -219,7 +219,9 @@ public String mappedTypeName(String internalName) {
   }
 
   boolean shouldIntercept(MethodInsnNode targetMethod) {
-    if (targetMethod.name.equals("<init>")) return false; // sorry, can't strip out calls to super() in constructor
+    if (targetMethod.name.equals("<init>")) {
+      return false; // sorry, can't strip out calls to super() in constructor
+    }
     return methodsToIntercept.contains(new MethodRef(targetMethod.owner, targetMethod.name))
         || methodsToIntercept.contains(new MethodRef(targetMethod.owner, "*"));
   }

diff --git a/sandbox/src/main/java/org/robolectric/internal/bytecode/MutableClass.java b/sandbox/src/main/java/org/robolectric/internal/bytecode/MutableClass.java
@@ -1,12 +1,10 @@
 package org.robolectric.internal.bytecode;
 
 import com.google.common.collect.ImmutableSet;
-import java.lang.annotation.Annotation;
 import java.util.ArrayList;
 import java.util.List;
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.Type;
-import org.objectweb.asm.tree.AnnotationNode;
 import org.objectweb.asm.tree.ClassNode;
 import org.objectweb.asm.tree.FieldNode;
 import org.objectweb.asm.tree.MethodNode;
@@ -45,21 +43,6 @@ public boolean isAnnotation() {
     return (classNode.access & Opcodes.ACC_ANNOTATION) != 0;
   }
 
-  public boolean hasAnnotation(Class<? extends Annotation> annotationClass) {
-    String internalName = "L" + annotationClass.getName().replace('.', '/') + ";";
-    if (classNode.visibleAnnotations == null) {
-      return false;
-    }
-
-    for (Object visibleAnnotation : classNode.visibleAnnotations) {
-      AnnotationNode annotationNode = (AnnotationNode) visibleAnnotation;
-      if (annotationNode.desc.equals(internalName)) {
-        return true;
-      }
-    }
-    return false;
-  }
-
   public String getName() {
     return className;
   }

diff --git a/sandbox/src/main/java/org/robolectric/internal/bytecode/SandboxClassLoader.java b/sandbox/src/main/java/org/robolectric/internal/bytecode/SandboxClassLoader.java
@@ -128,18 +128,13 @@ public Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundExce
   protected Class<?> maybeInstrumentClass(String className) throws ClassNotFoundException {
     final byte[] origClassBytes = getByteCode(className);
 
-    MutableClass mutableClass = PerfStatsCollector.getInstance().measure("analyze class",
-        () -> classInstrumentor.analyzeClass(origClassBytes, config, classNodeProvider)
-    );
-
     try {
       final byte[] bytes;
-      if (config.shouldInstrument(mutableClass)) {
-        bytes = PerfStatsCollector.getInstance().measure("instrument class",
-            () -> classInstrumentor.instrumentToBytes(mutableClass)
-        );
+      ClassDetails classDetails = new ClassDetails(origClassBytes);
+      if (config.shouldInstrument(classDetails)) {
+        bytes = classInstrumentor.instrument(origClassBytes, config, classNodeProvider);
       } else {
-        bytes = postProcessUninstrumentedClass(mutableClass, origClassBytes);
+        bytes = postProcessUninstrumentedClass(classDetails, origClassBytes);
       }
       ensurePackage(className);
       return defineClass(className, bytes, 0, bytes.length);
@@ -152,7 +147,7 @@ protected Class<?> maybeInstrumentClass(String className) throws ClassNotFoundEx
   }
 
   protected byte[] postProcessUninstrumentedClass(
-      MutableClass mutableClass, byte[] origClassBytes) {
+      ClassDetails classDetails, byte[] origClassBytes) {
     return origClassBytes;
   }
 

diff --git a/sandbox/src/test/java/org/robolectric/internal/bytecode/SandboxClassLoaderTest.java b/sandbox/src/test/java/org/robolectric/internal/bytecode/SandboxClassLoaderTest.java
@@ -118,7 +118,7 @@ public void shouldDelegateToHandlerForConstructors() throws Exception {
   public void shouldDelegateClassLoadForUnacquiredClasses() throws Exception {
     InstrumentationConfiguration config = mock(InstrumentationConfiguration.class);
     when(config.shouldAcquire(anyString())).thenReturn(false);
-    when(config.shouldInstrument(any(MutableClass.class))).thenReturn(false);
+    when(config.shouldInstrument(any(ClassDetails.class))).thenReturn(false);
     ClassLoader classLoader = new SandboxClassLoader(config);
     Class<?> exampleClass = classLoader.loadClass(AnExampleClass.class.getName());
     assertSame(getClass().getClassLoader(), exampleClass.getClassLoader());

diff --git a/utils/reflector/build.gradle b/utils/reflector/build.gradle
@@ -2,9 +2,9 @@ apply plugin: org.robolectric.gradle.RoboJavaModulePlugin
 apply plugin: org.robolectric.gradle.DeployedRoboJavaModulePlugin
 
 dependencies {
-    api "org.ow2.asm:asm:7.3.1"
-    api "org.ow2.asm:asm-commons:7.3.1"
-    api "org.ow2.asm:asm-util:7.3.1"
+    api "org.ow2.asm:asm:9.0"
+    api "org.ow2.asm:asm-commons:9.0"
+    api "org.ow2.asm:asm-util:9.0"
     api project(":utils")
 
     testImplementation project(":shadowapi")