Skip to content

Commit

Permalink
Fix O(N^2) performance issue when determining declaration order.
Browse files Browse the repository at this point in the history 
Note: This CL fixes the issue for `InjectionSiteFactory`, but I ended up just inlining the last remaining (unfixed) usage into `DiagnosticMessageGenerator`.

Looks like there's a number of similar comparators within `DiagnosticMessageGenerator`, which we may want to eventually look into fixing them all at some point. On the other hand, the `DiagnosticMessageGenerator` only runs when there's errors so it's likely not as big of an issue.

RELNOTES=N/A
PiperOrigin-RevId: 444957049
  • Loading branch information
@bcorso
bcorso authored and Dagger Team committed Apr 27, 2022
1 parent 75a79c0 commit 1a01575
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 39 deletions.
22 changes: 11 additions & 11 deletions java/dagger/internal/codegen/binding/InjectionSiteFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@
import static androidx.room.compiler.processing.XElementKt.isField;
import static androidx.room.compiler.processing.XElementKt.isMethod;
import static com.google.common.base.Preconditions.checkArgument;
import static dagger.internal.codegen.binding.SourceFiles.DECLARATION_ORDER;
import static dagger.internal.codegen.xprocessing.XElements.asField;
import static dagger.internal.codegen.xprocessing.XElements.asMethod;
import static dagger.internal.codegen.xprocessing.XElements.closestEnclosingTypeElement;
import static dagger.internal.codegen.xprocessing.XElements.getSimpleName;
import static dagger.internal.codegen.xprocessing.XProcessingEnvs.javacOverrides;
import static dagger.internal.codegen.xprocessing.XTypes.isDeclared;
import static dagger.internal.codegen.xprocessing.XTypes.nonObjectSuperclass;
import static java.util.Comparator.comparing;

import androidx.room.compiler.processing.XElement;
import androidx.room.compiler.processing.XFieldElement;
Expand All @@ -40,10 +40,9 @@
import com.google.common.collect.SetMultimap;
import dagger.internal.codegen.binding.MembersInjectionBinding.InjectionSite;
import dagger.internal.codegen.xprocessing.XElements;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import javax.inject.Inject;
Expand All @@ -65,30 +64,31 @@ final class InjectionSiteFactory {
ImmutableSortedSet<InjectionSite> getInjectionSites(XType type) {
checkArgument(isDeclared(type));
Set<InjectionSite> injectionSites = new HashSet<>();
List<XTypeElement> ancestors = new ArrayList<>();
InjectionSiteVisitor injectionSiteVisitor = new InjectionSiteVisitor();
Map<XTypeElement, Integer> enclosingTypeElementOrder = new HashMap<>();
Map<XElement, Integer> enclosedElementOrder = new HashMap<>();
for (Optional<XType> currentType = Optional.of(type);
currentType.isPresent();
currentType = nonObjectSuperclass(currentType.get())) {
XTypeElement typeElement = currentType.get().getTypeElement();
ancestors.add(typeElement);
enclosingTypeElementOrder.put(typeElement, enclosingTypeElementOrder.size());
for (XElement enclosedElement : typeElement.getEnclosedElements()) {
enclosedElementOrder.put(enclosedElement, enclosedElementOrder.size());
injectionSiteVisitor
.visit(enclosedElement, currentType.get())
.ifPresent(injectionSites::add);
}
}
return ImmutableSortedSet.copyOf(
// supertypes before subtypes
Comparator.comparing(
(InjectionSite injectionSite) ->
ancestors.indexOf(injectionSite.enclosingTypeElement()))
.reversed()
comparing(
InjectionSite::enclosingTypeElement,
comparing(enclosingTypeElementOrder::get).reversed())
// fields before methods
.thenComparing(InjectionSite::kind)
// then sort by whichever element comes first in the parent
// this isn't necessary, but makes the processor nice and predictable
.thenComparing(InjectionSite::element, DECLARATION_ORDER),
.thenComparing(InjectionSite::element, comparing(enclosedElementOrder::get)),
injectionSites);
}

Expand Down
26 changes: 0 additions & 26 deletions java/dagger/internal/codegen/binding/SourceFiles.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 androidx.room.compiler.processing.XElementKt.isConstructor;
import static androidx.room.compiler.processing.XElementKt.isTypeElement;
import static com.google.common.base.CaseFormat.LOWER_CAMEL;
import static com.google.common.base.CaseFormat.UPPER_CAMEL;
import static com.google.common.base.Preconditions.checkArgument;
Expand All @@ -38,16 +37,13 @@
import static dagger.internal.codegen.xprocessing.XElements.asExecutable;
import static dagger.internal.codegen.xprocessing.XElements.asTypeElement;
import static dagger.internal.codegen.xprocessing.XElements.getSimpleName;
import static dagger.internal.codegen.xprocessing.XElements.isExecutable;
import static dagger.internal.codegen.xprocessing.XTypeElements.typeVariableNames;
import static dagger.spi.model.BindingKind.ASSISTED_INJECTION;
import static dagger.spi.model.BindingKind.INJECTION;
import static dagger.spi.model.BindingKind.MULTIBOUND_MAP;
import static dagger.spi.model.BindingKind.MULTIBOUND_SET;
import static java.util.Comparator.comparing;
import static javax.lang.model.SourceVersion.isName;

import androidx.room.compiler.processing.XElement;
import androidx.room.compiler.processing.XExecutableElement;
import androidx.room.compiler.processing.XFieldElement;
import androidx.room.compiler.processing.XTypeElement;
Expand All @@ -68,35 +64,13 @@
import dagger.internal.codegen.javapoet.TypeNames;
import dagger.spi.model.DependencyRequest;
import dagger.spi.model.RequestKind;
import java.util.Comparator;
import java.util.List;
import javax.lang.model.SourceVersion;

/** Utilities for generating files. */
public class SourceFiles {

private static final Joiner CLASS_FILE_NAME_JOINER = Joiner.on('_');

/**
* Compares elements according to their declaration order among siblings. Only valid to compare
* elements enclosed by the same parent.
*/
// TODO(bcorso): Look into replacing DECLARATION_ORDER with something more efficient that doesn't
// have to re-iterate over all fields to find the index.
public static final Comparator<XElement> DECLARATION_ORDER =
comparing(element -> siblings(element).indexOf(element));

private static List<? extends XElement> siblings(XElement element) {
if (isTypeElement(element.getEnclosingElement())) {
return asTypeElement(element.getEnclosingElement()).getEnclosedElements();
} else if (isExecutable(element.getEnclosingElement())) {
// For parameter elements, element.getEnclosingElement().getEnclosedElements() is empty. So
// instead look at the parameter list of the enclosing executable.
return asExecutable(element.getEnclosingElement()).getParameters();
}
throw new AssertionError("Unexpected element type: " + element);
}

/**
* Generates names and keys for the factory class fields needed to hold the framework classes for
* all of the dependencies of {@code binding}. It is responsible for choosing a name that
Expand Down
24 changes: 22 additions & 2 deletions java/dagger/internal/codegen/validation/DiagnosticMessageGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,24 @@

package dagger.internal.codegen.validation;

import static androidx.room.compiler.processing.XElementKt.isTypeElement;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Predicates.equalTo;
import static com.google.common.base.Verify.verify;
import static com.google.common.collect.Iterables.filter;
import static com.google.common.collect.Iterables.getLast;
import static com.google.common.collect.Iterables.indexOf;
import static com.google.common.collect.Iterables.transform;
import static dagger.internal.codegen.base.ElementFormatter.elementToString;
import static dagger.internal.codegen.binding.SourceFiles.DECLARATION_ORDER;
import static dagger.internal.codegen.extension.DaggerGraphs.shortestPath;
import static dagger.internal.codegen.extension.DaggerStreams.instancesOf;
import static dagger.internal.codegen.extension.DaggerStreams.presentValues;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableList;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableSet;
import static dagger.internal.codegen.xprocessing.XElements.asExecutable;
import static dagger.internal.codegen.xprocessing.XElements.asTypeElement;
import static dagger.internal.codegen.xprocessing.XElements.closestEnclosingTypeElement;
import static dagger.internal.codegen.xprocessing.XElements.isExecutable;
import static java.util.Collections.min;
import static java.util.Comparator.comparing;
import static java.util.Comparator.comparingInt;
Expand Down Expand Up @@ -58,6 +61,7 @@
import dagger.spi.model.ComponentPath;
import dagger.spi.model.DaggerElement;
import java.util.Comparator;
import java.util.List;
import java.util.Set;
import java.util.function.Function;
import javax.inject.Inject;
Expand Down Expand Up @@ -388,7 +392,23 @@ private Comparator<DependencyEdge> requestEnclosingTypeName() {
*/
private Comparator<DependencyEdge> requestElementDeclarationOrder() {
return comparing(
edge -> edge.dependencyRequest().requestElement().get().xprocessing(), DECLARATION_ORDER);
edge -> edge.dependencyRequest().requestElement().get().xprocessing(),
// TODO(bcorso): This is inefficient as it requires each element to iterate through all of
// its siblings to find its order. Ideally, the order of all elements would be calculated in
// a single pass and cached, but the organization of the current code makes that a bit
// difficult. I'm leaving this for now since this is only called on failures.
comparing(
element -> {
XElement enclosingElement = element.getEnclosingElement();
checkState(isTypeElement(enclosingElement) || isExecutable(enclosingElement));
List<? extends XElement> siblings =
isTypeElement(enclosingElement)
? asTypeElement(enclosingElement).getEnclosedElements()
// For parameter elements, element.getEnclosingElement().getEnclosedElements()
// is empty, so instead look at the parameter list of the enclosing executable
: asExecutable(enclosingElement).getParameters();
return siblings.indexOf(element);
}));
}

private Node source(Edge edge) {
Expand Down

0 comments on commit 1a01575

Please sign in to comment.