Skip to content

Commit

Permalink
Add a cache around ASTHelpers.isInherited
Browse files Browse the repository at this point in the history
This is called quite often, and with a very small number of distinct
arguments. I'd like to speed up the actual implementation, or avoid
calling it so often, but for starters introducing a cache speeds up
the method by around 80%, shaving 2% off of the entire javac action.

This adds a new dependency on Caffeine; I considered using a Guava
Cache since we already depend on Guava, but Caffeine's version is 3
times as fast for this use case.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=259064453
  • Loading branch information
amalloy authored and nick-someone committed Jul 29, 2019
1 parent 40a4ed4 commit bfb1789
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 13 deletions.
10 changes: 9 additions & 1 deletion check_api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
limitations under the License.
-->

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<!-- Added this because the JSR 305 dependency wouldn't download from the
Expand Down Expand Up @@ -139,6 +141,12 @@
<artifactId>software-and-algorithms</artifactId>
<version>1.0</version>
</dependency>
<dependency>
<!-- Apache 2.0 -->
<groupId>com.github.ben-manes.caffeine</groupId>
<artifactId>caffeine</artifactId>
<version>2.2.6</version>
</dependency>
</dependencies>

</project>
34 changes: 22 additions & 12 deletions check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import static com.sun.tools.javac.code.Scope.LookupKind.NON_RECURSIVE;
import static java.util.Objects.requireNonNull;

import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
Expand Down Expand Up @@ -677,19 +679,27 @@ public static boolean hasAnnotation(Symbol sym, String annotationClass, VisitorS
return false;
}

private static final Cache<String, Boolean> inheritedAnnotationCache =
Caffeine.newBuilder().maximumSize(1000).build();

@SuppressWarnings("ConstantConditions") // IntelliJ worries unboxing our Boolean may throw NPE.
private static boolean isInherited(VisitorState state, String annotationName) {
Symbol annotationSym = state.getSymbolFromString(annotationName);
if (annotationSym == null) {
return false;
}
try {
annotationSym.complete();
} catch (CompletionFailure e) {
// @Inherited won't work if the annotation isn't on the classpath, but we can still check
// if it's present directly
}
Symbol inheritedSym = state.getSymtab().inheritedType.tsym;
return annotationSym.attribute(inheritedSym) != null;
return inheritedAnnotationCache.get(
annotationName,
name -> {
Symbol annotationSym = state.getSymbolFromString(name);
if (annotationSym == null) {
return false;
}
try {
annotationSym.complete();
} catch (CompletionFailure e) {
/* @Inherited won't work if the annotation isn't on the classpath, but we can still
check if it's present directly */
}
Symbol inheritedSym = state.getSymtab().inheritedType.tsym;
return annotationSym.attribute(inheritedSym) != null;
});
}

private static boolean hasAttribute(Symbol sym, Name annotationName) {
Expand Down

0 comments on commit bfb1789

Please sign in to comment.