Skip to content

Commit

Permalink
Use HashMap with computeIfAbsent instead of a LoadingCache for Visito…
Browse files Browse the repository at this point in the history
…rState.getTypeFromString(). LoadingCache has concurrency baked in, but we were setting concurrencyLevel = 1 because javac is not meant to be used in a concurrency environment.

RELNOTES: Performance improvements

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=248210051
  • Loading branch information
ronshapiro committed May 27, 2019
1 parent 5871842 commit 30137a0
Showing 1 changed file with 10 additions and 15 deletions.
25 changes: 10 additions & 15 deletions check_api/src/main/java/com/google/errorprone/VisitorState.java
Expand Up @@ -18,9 +18,6 @@

import com.google.common.base.Optional;
import com.google.common.base.Splitter;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultiset;
Expand Down Expand Up @@ -54,9 +51,9 @@
import com.sun.tools.javac.util.Names;
import com.sun.tools.javac.util.Options;
import java.io.IOException;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ExecutionException;
import javax.annotation.Nullable;

/** @author alexeagle@google.com (Alex Eagle) */
Expand All @@ -66,7 +63,7 @@ public class VisitorState {
private final StatisticsCollector statisticsCollector;
private final Map<String, SeverityLevel> severityMap;
private final ErrorProneOptions errorProneOptions;
private final LoadingCache<String, Optional<Type>> typeCache;
private final Map<String, Optional<Type>> typeCache;
private final ErrorProneTimings timings;
public final Context context;

Expand Down Expand Up @@ -198,7 +195,7 @@ private VisitorState(
Map<String, SeverityLevel> severityMap,
ErrorProneOptions errorProneOptions,
StatisticsCollector statisticsCollector,
LoadingCache<String, Optional<Type>> typeCache,
Map<String, Optional<Type>> typeCache,
TreePath path,
SuppressedState suppressedState) {
this.context = context;
Expand All @@ -213,10 +210,10 @@ private VisitorState(
this.typeCache =
typeCache != null
? typeCache
: CacheBuilder.newBuilder()
.concurrencyLevel(1) // resolving symbols in javac is not thread-safe
.build(
CacheLoader.from(key -> Optional.fromNullable(getTypeFromStringInternal(key))));
// TODO(ronshapiro): should we presize this with a reasonable size? We can check for the
// smallest build and see how many types are loaded and use that. Or perhaps a heuristic
// based on number of files?
: new HashMap<>();
}

public VisitorState withPath(TreePath path) {
Expand Down Expand Up @@ -335,11 +332,9 @@ public Name getName(String nameStr) {
*/
@Nullable
public Type getTypeFromString(String typeStr) {
try {
return typeCache.get(typeStr).orNull();
} catch (ExecutionException e) {
return null;
}
return typeCache
.computeIfAbsent(typeStr, key -> Optional.fromNullable(getTypeFromStringInternal(key)))
.orNull();
}

@Nullable
Expand Down

0 comments on commit 30137a0

Please sign in to comment.