Skip to content

Commit

Permalink
Remove static RequestKinds cache and improve build performance.
Browse files Browse the repository at this point in the history 
See #1645

The static RequestKinds cache causes a memory leak in gradle due to the persistent GradleDaemon. This CL removes that cache entirely, and instead fixes the root causes of the performance issues, by short circuiting InjectionSiteVisitor to abort early if the method is not injectable. This avoids a lot of unnecessary calls to RequestKinds.getRequestKind() for methods that are not injectable (e.g. aren't annotated with @Inject).

The aggregated pprof results from 10 runs before and after changes in this CL are below.

                                            Before (s)    After (s)     Diff
-----------------------------------------------------------------------------
ComponentProcessingStep.process               51.80         43.25      -16.5%
RequestKinds.getRequestKind                    8.48          0.33      -95.9%
-----------------------------------------------------------------------------

RELNOTES=Fix memory leak with RequestKinds cache.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=286934462
  • Loading branch information
@bcorso @cpovirk
bcorso authored and cpovirk committed Dec 26, 2019
1 parent 545f851 commit 2902519
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 86 deletions.
24 changes: 1 addition & 23 deletions java/dagger/internal/codegen/base/RequestKinds.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@
import static dagger.model.RequestKind.PROVIDER_OF_LAZY;
import static javax.lang.model.type.TypeKind.DECLARED;

import com.google.auto.common.MoreTypes;
import com.google.common.base.Equivalence;
import com.google.common.collect.ImmutableMap;
import com.google.common.util.concurrent.ListenableFuture;
import com.squareup.javapoet.TypeName;
Expand All @@ -43,16 +41,11 @@
import dagger.model.RequestKind;
import dagger.producers.Produced;
import dagger.producers.Producer;
import java.util.HashMap;
import java.util.Map;
import javax.inject.Provider;
import javax.lang.model.type.TypeMirror;

/** Utility methods for {@link RequestKind}s. */
public final class RequestKinds {
private static final Equivalence<TypeMirror> EQUIVALENCE = MoreTypes.equivalence();
private static final Map<Equivalence.Wrapper<TypeMirror>, RequestKind> requestKindMap =
new HashMap<>();

/** Returns the type of a request of this kind for a key with a given type. */
public static TypeMirror requestType(
Expand Down Expand Up @@ -110,11 +103,6 @@ public static TypeName requestTypeName(RequestKind requestKind, TypeName keyType

/** Returns the {@link RequestKind} that matches the wrapping types (if any) of {@code type}. */
public static RequestKind getRequestKind(TypeMirror type) {
return requestKindMap.computeIfAbsent(
EQUIVALENCE.wrap(type), unused -> getRequestKindUncached(type));
}

public static RequestKind getRequestKindUncached(TypeMirror type) {
checkTypePresent(type);
if (!type.getKind().equals(DECLARED) || asDeclared(type).getTypeArguments().isEmpty()) {
// If the type is not a declared type (i.e. class or interface) with type arguments, then we
Expand All @@ -123,7 +111,7 @@ public static RequestKind getRequestKindUncached(TypeMirror type) {
}
for (RequestKind kind : FRAMEWORK_CLASSES.keySet()) {
if (isTypeOf(frameworkClass(kind), type)) {
if (kind.equals(PROVIDER) && matchesKind(LAZY, extractKeyType(kind, type))) {
if (kind.equals(PROVIDER) && getRequestKind(DaggerTypes.unwrapType(type)).equals(LAZY)) {
return PROVIDER_OF_LAZY;
}
return kind;
Expand All @@ -132,16 +120,6 @@ public static RequestKind getRequestKindUncached(TypeMirror type) {
return RequestKind.INSTANCE;
}

/**
* Returns {@code true} if {@code type} is a parameterized type of {@code kind}'s {@link
* #frameworkClass(RequestKind) framework class}.
*/
private static boolean matchesKind(RequestKind kind, TypeMirror type) {
return isType(type)
&& isTypeOf(frameworkClass(kind), type)
&& !asDeclared(type).getTypeArguments().isEmpty();
}

/**
* Unwraps the framework class(es) of {@code requestKind} from {@code type}. If {@code
* requestKind} is {@link RequestKind#INSTANCE}, this acts as an identity function.
Expand Down
111 changes: 48 additions & 63 deletions java/dagger/internal/codegen/binding/InjectionSiteFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package dagger.internal.codegen.binding;

import static com.google.auto.common.MoreElements.isAnnotationPresent;
import static dagger.internal.codegen.binding.MembersInjectionBinding.InjectionSite.Kind.METHOD;
import static dagger.internal.codegen.langmodel.DaggerElements.DECLARATION_ORDER;
import static javax.lang.model.element.Modifier.PRIVATE;
import static javax.lang.model.element.Modifier.STATIC;
Expand All @@ -38,7 +37,6 @@
import java.util.Set;
import javax.inject.Inject;
import javax.lang.model.element.Element;
import javax.lang.model.element.ElementVisitor;
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.TypeElement;
import javax.lang.model.element.VariableElement;
Expand All @@ -49,33 +47,6 @@

/** A factory for {@link Binding} objects. */
final class InjectionSiteFactory {
private final ElementVisitor<Optional<InjectionSite>, DeclaredType> injectionSiteVisitor =
new ElementKindVisitor8<Optional<InjectionSite>, DeclaredType>(Optional.empty()) {
@Override
public Optional<InjectionSite> visitExecutableAsMethod(
ExecutableElement method, DeclaredType type) {
ExecutableType resolved = MoreTypes.asExecutable(types.asMemberOf(type, method));
return Optional.of(
InjectionSite.method(
method,
dependencyRequestFactory.forRequiredResolvedVariables(
method.getParameters(), resolved.getParameterTypes())));
}

@Override
public Optional<InjectionSite> visitVariableAsField(
VariableElement field, DeclaredType type) {
if (!isAnnotationPresent(field, Inject.class)
|| field.getModifiers().contains(PRIVATE)
|| field.getModifiers().contains(STATIC)) {
return Optional.empty();
}
TypeMirror resolved = types.asMemberOf(type, field);
return Optional.of(
InjectionSite.field(
field, dependencyRequestFactory.forRequiredResolvedVariable(field, resolved)));
}
};

private final DaggerTypes types;
private final DaggerElements elements;
Expand All @@ -95,27 +66,14 @@ public Optional<InjectionSite> visitVariableAsField(
ImmutableSortedSet<InjectionSite> getInjectionSites(DeclaredType declaredType) {
Set<InjectionSite> injectionSites = new HashSet<>();
List<TypeElement> ancestors = new ArrayList<>();
SetMultimap<String, ExecutableElement> overriddenMethodMap = LinkedHashMultimap.create();
InjectionSiteVisitor injectionSiteVisitor = new InjectionSiteVisitor();
for (Optional<DeclaredType> currentType = Optional.of(declaredType);
currentType.isPresent();
currentType = types.nonObjectSuperclass(currentType.get())) {
DeclaredType type = currentType.get();
ancestors.add(MoreElements.asType(type.asElement()));
for (Element enclosedElement : type.asElement().getEnclosedElements()) {
Optional<InjectionSite> maybeInjectionSite =
injectionSiteVisitor.visit(enclosedElement, type);
if (maybeInjectionSite.isPresent()) {
InjectionSite injectionSite = maybeInjectionSite.get();
if (shouldBeInjected(injectionSite.element(), overriddenMethodMap)) {
injectionSites.add(injectionSite);
}
if (injectionSite.kind().equals(METHOD)) {
ExecutableElement injectionSiteMethod =
MoreElements.asExecutable(injectionSite.element());
overriddenMethodMap.put(
injectionSiteMethod.getSimpleName().toString(), injectionSiteMethod);
}
}
injectionSiteVisitor.visit(enclosedElement, type).ifPresent(injectionSites::add);
}
}
return ImmutableSortedSet.copyOf(
Expand All @@ -132,30 +90,57 @@ ImmutableSortedSet<InjectionSite> getInjectionSites(DeclaredType declaredType) {
injectionSites);
}

private boolean shouldBeInjected(
Element injectionSite, SetMultimap<String, ExecutableElement> overriddenMethodMap) {
if (!isAnnotationPresent(injectionSite, Inject.class)
|| injectionSite.getModifiers().contains(PRIVATE)
|| injectionSite.getModifiers().contains(STATIC)) {
return false;
private final class InjectionSiteVisitor
extends ElementKindVisitor8<Optional<InjectionSite>, DeclaredType> {
private final SetMultimap<String, ExecutableElement> subclassMethodMap =
LinkedHashMultimap.create();

InjectionSiteVisitor() {
super(Optional.empty());
}

if (injectionSite.getKind().isField()) { // Inject all fields (self and ancestors)
return true;
@Override
public Optional<InjectionSite> visitExecutableAsMethod(
ExecutableElement method, DeclaredType type) {
subclassMethodMap.put(method.getSimpleName().toString(), method);
if (!shouldBeInjected(method)) {
return Optional.empty();
}
// This visitor assumes that subclass methods are visited before superclass methods, so we can
// skip any overridden method that has already been visited. To decrease the number of methods
// that are checked, we store the already injected methods in a SetMultimap and only check the
// methods with the same name.
String methodName = method.getSimpleName().toString();
TypeElement enclosingType = MoreElements.asType(method.getEnclosingElement());
for (ExecutableElement subclassMethod : subclassMethodMap.get(methodName)) {
if (method != subclassMethod && elements.overrides(subclassMethod, method, enclosingType)) {
return Optional.empty();
}
}
ExecutableType resolved = MoreTypes.asExecutable(types.asMemberOf(type, method));
return Optional.of(
InjectionSite.method(
method,
dependencyRequestFactory.forRequiredResolvedVariables(
method.getParameters(), resolved.getParameterTypes())));
}

// For each method with the same name belonging to any descendant class, return false if any
// method has already overridden the injectionSite method. To decrease the number of methods
// that are checked, we store the already injected methods in a SetMultimap and only
// check the methods with the same name.
ExecutableElement injectionSiteMethod = MoreElements.asExecutable(injectionSite);
TypeElement injectionSiteType = MoreElements.asType(injectionSite.getEnclosingElement());
for (ExecutableElement method :
overriddenMethodMap.get(injectionSiteMethod.getSimpleName().toString())) {
if (elements.overrides(method, injectionSiteMethod, injectionSiteType)) {
return false;
@Override
public Optional<InjectionSite> visitVariableAsField(
VariableElement field, DeclaredType type) {
if (!shouldBeInjected(field)) {
return Optional.empty();
}
TypeMirror resolved = types.asMemberOf(type, field);
return Optional.of(
InjectionSite.field(
field, dependencyRequestFactory.forRequiredResolvedVariable(field, resolved)));
}

private boolean shouldBeInjected(Element injectionSite) {
return isAnnotationPresent(injectionSite, Inject.class)
&& !injectionSite.getModifiers().contains(PRIVATE)
&& !injectionSite.getModifiers().contains(STATIC);
}
return true;
}
}

0 comments on commit 2902519

Please sign in to comment.