Limit the size of the switch statements in SwitchingProvider classes
In devices using ART, the method size greatly affects runtime performance, so we control the method size by limiting the size of the switch statements in SwitchingProviders.
RELNOTES=n/a
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=193097061
diff --git a/java/dagger/internal/codegen/ComponentBindingExpressions.java b/java/dagger/internal/codegen/ComponentBindingExpressions.java
index 4cb230f..8a775d0 100644
--- a/java/dagger/internal/codegen/ComponentBindingExpressions.java
+++ b/java/dagger/internal/codegen/ComponentBindingExpressions.java
@@ -30,6 +30,8 @@
import static dagger.internal.codegen.TypeNames.REFERENCE_RELEASING_PROVIDER;
import static dagger.internal.codegen.TypeNames.SINGLE_CHECK;
import static dagger.model.BindingKind.DELEGATE;
+import static dagger.model.BindingKind.MULTIBOUND_MAP;
+import static dagger.model.BindingKind.MULTIBOUND_SET;
import com.google.auto.common.MoreTypes;
import com.google.common.collect.HashBasedTable;
@@ -114,7 +116,7 @@
this.compilerOptions = checkNotNull(compilerOptions);
this.membersInjectionMethods =
new MembersInjectionMethods(generatedComponentModel, this, graph, elements, types);
- this.switchingProviders = new SwitchingProviders(generatedComponentModel, this, graph, types);
+ this.switchingProviders = new SwitchingProviders(generatedComponentModel, this, types);
}
/**
@@ -279,7 +281,11 @@
*/
private FrameworkInstanceBindingExpression frameworkInstanceBindingExpression(
ResolvedBindings resolvedBindings, RequestKind requestKind) {
- Optional<MemberSelect> staticMethod = staticFactoryCreation(resolvedBindings);
+ // TODO(user): Consider merging the static factory creation logic into CreationExpressions?
+ Optional<MemberSelect> staticMethod =
+ useStaticFactoryCreation(resolvedBindings.contributionBinding())
+ ? staticFactoryCreation(resolvedBindings)
+ : Optional.empty();
FrameworkInstanceCreationExpression frameworkInstanceCreationExpression =
resolvedBindings.scope().isPresent()
? scope(resolvedBindings, frameworkInstanceCreationExpression(resolvedBindings))
@@ -574,6 +580,20 @@
}
/**
+ * Returns {@code true} if the binding should use the static factory creation strategy.
+ *
+ * In default mode, we always use the static factory creation strategy. In Android mode, we
+ * prefer to use the SwitchingProvider than the static factories to reduce class loading; however,
+ * we allow static factories that can reused across multiple bindings, e.g. {@code MapFactory} or
+ * {@code SetFactory}.
+ */
+ private boolean useStaticFactoryCreation(ContributionBinding binding) {
+ return !compilerOptions.experimentalAndroidMode()
+ || binding.kind().equals(MULTIBOUND_MAP)
+ || binding.kind().equals(MULTIBOUND_SET);
+ }
+
+ /**
* Returns {@code true} if we can use a direct (not {@code Provider.get()}) expression for this
* binding. If the binding doesn't {@linkplain #needsCaching(ResolvedBindings) need to be cached},
* we can.
@@ -609,7 +629,7 @@
return wrapInMethod(
resolvedBindings,
RequestKind.PROVIDER,
- switchingProviders.newBindingExpression(resolvedBindings.key()));
+ switchingProviders.newBindingExpression(resolvedBindings.contributionBinding()));
}
} else if (resolvedBindings.contributionBinding().kind().equals(DELEGATE)
&& !needsCaching(resolvedBindings)) {
diff --git a/java/dagger/internal/codegen/GeneratedComponentModel.java b/java/dagger/internal/codegen/GeneratedComponentModel.java
index 3130696..8f8dde1 100644
--- a/java/dagger/internal/codegen/GeneratedComponentModel.java
+++ b/java/dagger/internal/codegen/GeneratedComponentModel.java
@@ -16,7 +16,6 @@
package dagger.internal.codegen;
-import static com.google.common.base.Preconditions.checkState;
import static com.squareup.javapoet.TypeSpec.classBuilder;
import static dagger.internal.codegen.Accessibility.isTypeAccessibleFrom;
import static javax.lang.model.element.Modifier.FINAL;
@@ -35,7 +34,6 @@
import dagger.internal.ReferenceReleasingProviderManager;
import java.util.ArrayList;
import java.util.List;
-import java.util.Optional;
import javax.lang.model.element.Modifier;
import javax.lang.model.element.Name;
import javax.lang.model.element.TypeElement;
@@ -119,7 +117,7 @@
MultimapBuilder.enumKeys(MethodSpecKind.class).arrayListValues().build();
private final ListMultimap<TypeSpecKind, TypeSpec> typeSpecsMap =
MultimapBuilder.enumKeys(TypeSpecKind.class).arrayListValues().build();
- private Optional<Supplier<TypeSpec>> switchingProviderSupplier = Optional.empty();
+ private final List<Supplier<TypeSpec>> switchingProviderSupplier = new ArrayList<>();
private GeneratedComponentModel(ClassName name, Modifier... modifiers) {
this.name = name;
@@ -183,8 +181,7 @@
/** Adds a {@link Supplier} for the SwitchingProvider for the component. */
void addSwitchingProvider(Supplier<TypeSpec> typeSpecSupplier) {
- checkState(!switchingProviderSupplier.isPresent());
- switchingProviderSupplier = Optional.of(typeSpecSupplier);
+ switchingProviderSupplier.add(typeSpecSupplier);
}
/** Adds the given code block to the initialize methods of the component. */
@@ -217,7 +214,7 @@
fieldSpecsMap.asMap().values().forEach(component::addFields);
methodSpecsMap.asMap().values().forEach(component::addMethods);
typeSpecsMap.asMap().values().forEach(component::addTypes);
- switchingProviderSupplier.map(Supplier::get).ifPresent(component::addType);
+ switchingProviderSupplier.stream().map(Supplier::get).forEach(component::addType);
return component;
}
}
diff --git a/java/dagger/internal/codegen/SwitchingProviders.java b/java/dagger/internal/codegen/SwitchingProviders.java
index 29eac3f..306ec61 100644
--- a/java/dagger/internal/codegen/SwitchingProviders.java
+++ b/java/dagger/internal/codegen/SwitchingProviders.java
@@ -17,25 +17,31 @@
package dagger.internal.codegen;
import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.collect.Iterables.getLast;
+import static com.google.common.collect.Iterables.getOnlyElement;
import static com.squareup.javapoet.MethodSpec.constructorBuilder;
import static com.squareup.javapoet.MethodSpec.methodBuilder;
import static com.squareup.javapoet.TypeSpec.classBuilder;
import static dagger.internal.codegen.AnnotationSpecs.Suppression.UNCHECKED;
import static dagger.internal.codegen.AnnotationSpecs.suppressWarnings;
-import static dagger.internal.codegen.CodeBlocks.toConcatenatedCodeBlock;
+import static dagger.internal.codegen.DaggerStreams.toImmutableList;
import static dagger.internal.codegen.TypeNames.providerOf;
import static dagger.model.RequestKind.INSTANCE;
import static javax.lang.model.element.Modifier.FINAL;
import static javax.lang.model.element.Modifier.PRIVATE;
import static javax.lang.model.element.Modifier.PUBLIC;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
import com.squareup.javapoet.ClassName;
import com.squareup.javapoet.CodeBlock;
+import com.squareup.javapoet.MethodSpec;
import com.squareup.javapoet.TypeName;
import com.squareup.javapoet.TypeSpec;
import com.squareup.javapoet.TypeVariableName;
import dagger.model.Key;
import java.util.HashMap;
+import java.util.LinkedHashMap;
import java.util.Map;
import java.util.TreeMap;
import javax.inject.Provider;
@@ -47,95 +53,174 @@
* class that can provide instances for all types by switching on an id.
*/
final class SwitchingProviders {
+ /**
+ * Each switch size is fixed at 100 cases each and put in its own method. This is to limit the
+ * size of the methods so that we don't reach the "huge" method size limit for Android that will
+ * prevent it from being AOT compiled in some versions of Android (b/77652521). This generally
+ * starts to happen around 1500 cases, but we are choosing 100 to be safe.
+ */
+ // TODO(user): Include a proguard_spec in the Dagger library to prevent inlining these methods?
+ // TODO(ronshapiro): Consider making this configurable via a flag.
+ private static final int MAX_CASES_PER_SWITCH = 100;
+
+ private static final long MAX_CASES_PER_CLASS = MAX_CASES_PER_SWITCH * MAX_CASES_PER_SWITCH;
private static final TypeVariableName T = TypeVariableName.get("T");
- // Keep the switch cases ordered by switch id.
- private final Map<Integer, CodeBlock> switchCases = new TreeMap<>();
- private final Map<Key, Integer> switchIds = new HashMap<>();
+ /**
+ * Maps a {@link Key} to an instance of a {@link SwitchingProviderExpressions}. Each group of
+ * {@code MAX_CASES_PER_CLASS} keys will share the same instance.
+ */
+ private final Map<Key, SwitchingProviderExpressions> switchingProviderExpressionsMap =
+ new LinkedHashMap<>();
private final ComponentBindingExpressions componentBindingExpressions;
- private final BindingGraph graph;
private final GeneratedComponentModel generatedComponentModel;
private final ClassName owningComponent;
- private final ClassName switchingProviderType;
private final DaggerTypes types;
+ private final UniqueNameSet switchingProviderNames = new UniqueNameSet();
SwitchingProviders(
GeneratedComponentModel generatedComponentModel,
ComponentBindingExpressions componentBindingExpressions,
- BindingGraph graph,
DaggerTypes types) {
this.generatedComponentModel = checkNotNull(generatedComponentModel);
this.componentBindingExpressions = checkNotNull(componentBindingExpressions);
- this.graph = checkNotNull(graph);
this.types = checkNotNull(types);
this.owningComponent = checkNotNull(generatedComponentModel).name();
- this.switchingProviderType = owningComponent.nestedClass("SwitchingProvider");
}
/**
* Returns the binding expression for a binding that satisfies its {link Provider} requests with
* the generated {@code SwitchingProvider}.
*/
- BindingExpression newBindingExpression(Key key) {
+ BindingExpression newBindingExpression(ContributionBinding binding) {
return new BindingExpression() {
@Override
Expression getDependencyExpression(ClassName requestingClass) {
- if (!switchIds.containsKey(key)) {
- // Register the SwitchingProvider creation method the first time it's requested.
- if (switchIds.isEmpty()) {
- generatedComponentModel.addSwitchingProvider(
- SwitchingProviders.this::createSwitchingProviderType);
- }
-
- int switchId = switchIds.size();
- switchIds.put(key, switchId);
- switchCases.put(switchId, createSwitchCaseCodeBlock(key));
- }
-
- return Expression.create(
- types.wrapType(key.type(), Provider.class),
- CodeBlock.of("new $T<>($L)", switchingProviderType, switchIds.get(key)));
+ return switchingProviderExpressionsMap
+ .computeIfAbsent(binding.key(), key -> getSwitchingProviderExpressions())
+ .getExpression(binding);
}
};
}
- private CodeBlock createSwitchCaseCodeBlock(Key key) {
- CodeBlock instanceCodeBlock =
+ private SwitchingProviderExpressions getSwitchingProviderExpressions() {
+ if (switchingProviderExpressionsMap.size() % MAX_CASES_PER_CLASS == 0) {
+ String name = switchingProviderNames.getUniqueName("SwitchingProvider");
+ SwitchingProviderExpressions switchingProviderExpressions =
+ new SwitchingProviderExpressions(owningComponent.nestedClass(name));
+ generatedComponentModel.addSwitchingProvider(
+ switchingProviderExpressions::createSwitchingProviderType);
+ return switchingProviderExpressions;
+ }
+ return getLast(switchingProviderExpressionsMap.values());
+ }
+
+ // TODO(user): Consider just merging this class with SwitchingProviders.
+ private final class SwitchingProviderExpressions {
+ // Keep the switch cases ordered by switch id. The switch Ids are assigned in pre-order
+ // traversal, but the switch cases are assigned in post-order traversal of the binding graph.
+ private final Map<Integer, CodeBlock> switchCases = new TreeMap<>();
+ private final Map<Key, Integer> switchIds = new HashMap<>();
+ private final ClassName switchingProviderType;
+
+ SwitchingProviderExpressions(ClassName switchingProviderType) {
+ this.switchingProviderType = checkNotNull(switchingProviderType);
+ }
+
+ private Expression getExpression(ContributionBinding binding) {
+ if (!switchIds.containsKey(binding.key())) {
+ int switchId = switchIds.size();
+ switchIds.put(binding.key(), switchId);
+ switchCases.put(switchId, createSwitchCaseCodeBlock(binding));
+ }
+
+ return Expression.create(
+ types.wrapType(binding.key().type(), Provider.class),
+ CodeBlock.of("new $T<>($L)", switchingProviderType, switchIds.get(binding.key())));
+ }
+
+ private CodeBlock createSwitchCaseCodeBlock(ContributionBinding binding) {
+ CodeBlock instanceCodeBlock =
componentBindingExpressions
- .getDependencyExpression(key, INSTANCE, owningComponent)
+ .getDependencyExpression(binding.key(), INSTANCE, owningComponent)
.box(types)
.codeBlock();
- return CodeBlock.builder()
- // TODO(user): Is there something else more useful than the key?
- .add("case $L: // $L \n", switchIds.get(key), key)
- .addStatement("return ($T) $L", T, instanceCodeBlock)
- .build();
- }
+ return CodeBlock.builder()
+ // TODO(user): Is there something else more useful than the key?
+ .add("case $L: // $L \n", switchIds.get(binding.key()), binding.key())
+ .addStatement("return ($T) $L", T, instanceCodeBlock)
+ .build();
+ }
- private TypeSpec createSwitchingProviderType() {
- return classBuilder(switchingProviderType)
- .addModifiers(PRIVATE, FINAL)
- .addTypeVariable(T)
- .addSuperinterface(providerOf(T))
- .addField(TypeName.INT, "id", PRIVATE, FINAL)
- .addMethod(
- constructorBuilder()
- .addParameter(TypeName.INT, "id")
- .addStatement("this.id = id")
- .build())
- .addMethod(
+ private TypeSpec createSwitchingProviderType() {
+ return classBuilder(switchingProviderType)
+ .addModifiers(PRIVATE, FINAL)
+ .addTypeVariable(T)
+ .addSuperinterface(providerOf(T))
+ .addField(TypeName.INT, "id", PRIVATE, FINAL)
+ .addMethod(
+ constructorBuilder()
+ .addParameter(TypeName.INT, "id")
+ .addStatement("this.id = id")
+ .build())
+ .addMethods(getMethods())
+ .build();
+ }
+
+ private ImmutableList<MethodSpec> getMethods() {
+ ImmutableList<CodeBlock> switchCodeBlockPartitions = switchCodeBlockPartitions();
+ if (switchCodeBlockPartitions.size() == 1) {
+ // There are less than MAX_CASES_PER_SWITCH cases, so no need for extra get methods.
+ return ImmutableList.of(
methodBuilder("get")
.addModifiers(PUBLIC)
.addAnnotation(suppressWarnings(UNCHECKED))
.addAnnotation(Override.class)
.returns(T)
- .beginControlFlow("switch (id)")
- .addCode(switchCases.values().stream().collect(toConcatenatedCodeBlock()))
- .addStatement("default: throw new $T(id)", AssertionError.class)
- .endControlFlow()
- .build())
- .build();
+ .addCode(getOnlyElement(switchCodeBlockPartitions))
+ .build());
+ }
+
+ // This is the main public "get" method that will route to private getter methods.
+ MethodSpec.Builder routerMethod =
+ methodBuilder("get")
+ .addModifiers(PUBLIC)
+ .addAnnotation(Override.class)
+ .returns(T)
+ .beginControlFlow("switch (id / $L)", MAX_CASES_PER_SWITCH);
+
+ ImmutableList.Builder<MethodSpec> getMethods = ImmutableList.builder();
+ for (int i = 0; i < switchCodeBlockPartitions.size(); i++) {
+ MethodSpec method =
+ methodBuilder("get" + i)
+ .addModifiers(PRIVATE)
+ .addAnnotation(suppressWarnings(UNCHECKED))
+ .returns(T)
+ .addCode(switchCodeBlockPartitions.get(i))
+ .build();
+ getMethods.add(method);
+ routerMethod.addStatement("case $L: return $N()", i, method);
+ }
+
+ routerMethod.addStatement("default: throw new $T(id)", AssertionError.class).endControlFlow();
+
+ return getMethods.add(routerMethod.build()).build();
+ }
+
+ private ImmutableList<CodeBlock> switchCodeBlockPartitions() {
+ return Lists.partition(ImmutableList.copyOf(switchCases.values()), MAX_CASES_PER_SWITCH)
+ .stream()
+ .map(
+ partitionCases ->
+ CodeBlock.builder()
+ .beginControlFlow("switch (id)")
+ .add(CodeBlocks.concat(partitionCases))
+ .addStatement("default: throw new $T(id)", AssertionError.class)
+ .endControlFlow()
+ .build())
+ .collect(toImmutableList());
+ }
}
}