Skip to content

Commit

Permalink
Refactor BindingGraphFactory to remove the need for `keysMatchingRe…
Browse files Browse the repository at this point in the history
…quest` and corresponding cache.

This CL refactors `BindingGraphFactory` so that we no longer need to loop through all variations of a multibindings key when getting the `@Multibindings` declarations or `@IntoMap`/`@IntoSet` contributions from `ComponentDeclarations`. Instead, `ComponentDeclarations` now indexes these declarations by the unwrapped multibinding key (i.e. `Map<K, V>` or `Set<V>`) which we can use to get all declarations with a single key. In addition, the unwrapping of the key is done within `ComponentDeclarations` so that each caller doesn't have to remember to unwrap the key.

RELNOTES=Fixes missing variance in KSP for multibindings which can break KSP users who forgot the `@JvmSuppressWildcards`.
PiperOrigin-RevId: 660963882
  • Loading branch information
bcorso authored and Dagger Team committed Aug 8, 2024
1 parent 78be2f5 commit 93201e0
Show file tree
Hide file tree
Showing 5 changed files with 342 additions and 149 deletions.
5 changes: 0 additions & 5 deletions java/dagger/internal/codegen/DelegateComponentProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import dagger.internal.codegen.base.SourceFileGenerationException;
import dagger.internal.codegen.base.SourceFileGenerator;
import dagger.internal.codegen.base.SourceFileHjarGenerator;
import dagger.internal.codegen.binding.BindingGraphFactory;
import dagger.internal.codegen.binding.ComponentDescriptor;
import dagger.internal.codegen.binding.ContributionBinding;
import dagger.internal.codegen.binding.InjectBindingRegistry;
Expand Down Expand Up @@ -184,10 +183,6 @@ interface ProcessingRoundCacheModule {
@IntoSet
ClearableCache monitoringModules(MonitoringModules cache);

@Binds
@IntoSet
ClearableCache bindingGraphFactory(BindingGraphFactory cache);

@Binds
@IntoSet
ClearableCache componentValidator(ComponentValidator cache);
Expand Down
73 changes: 6 additions & 67 deletions java/dagger/internal/codegen/binding/BindingGraphFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import dagger.Reusable;
import dagger.internal.codegen.base.ClearableCache;
import dagger.internal.codegen.base.ContributionType;
import dagger.internal.codegen.base.Keys;
import dagger.internal.codegen.base.MapType;
Expand All @@ -64,19 +63,16 @@
import java.util.Queue;
import java.util.Set;
import javax.inject.Inject;
import javax.inject.Singleton;

/** A factory for {@link BindingGraph} objects. */
@Singleton
public final class BindingGraphFactory implements ClearableCache {
public final class BindingGraphFactory {

private final InjectBindingRegistry injectBindingRegistry;
private final KeyFactory keyFactory;
private final BindingFactory bindingFactory;
private final BindingNode.Factory bindingNodeFactory;
private final ComponentDeclarations.Factory componentDeclarationsFactory;
private final BindingGraphConverter bindingGraphConverter;
private final Map<Key, ImmutableSet<Key>> keysMatchingRequestCache = new HashMap<>();
private final CompilerOptions compilerOptions;

@Inject
Expand Down Expand Up @@ -158,11 +154,6 @@ private LegacyBindingGraph createLegacyBindingGraph(
return new LegacyBindingGraph(requestResolver, subgraphs.build());
}

@Override
public void clearCache() {
keysMatchingRequestCache.clear();
}

/** Represents a fully resolved binding graph. */
static final class LegacyBindingGraph {
private final Resolver resolver;
Expand Down Expand Up @@ -278,20 +269,15 @@ ResolvedBindings lookUpBindings(Key requestKey) {
Set<SubcomponentDeclaration> subcomponentDeclarations = new LinkedHashSet<>();

// Gather all bindings, multibindings, optional, and subcomponent declarations/contributions.
ImmutableSet<Key> multibindingKeysMatchingRequest =
multibindingKeysMatchingRequest(requestKey);
for (Resolver resolver : getResolverLineage()) {
bindings.addAll(resolver.getLocalExplicitBindings(requestKey));
multibindingContributions.addAll(resolver.getLocalMultibindingContributions(requestKey));
multibindingDeclarations.addAll(resolver.declarations.multibindings(requestKey));
subcomponentDeclarations.addAll(resolver.declarations.subcomponents(requestKey));
// The optional binding declarations are keyed by the unwrapped type.
keyFactory.unwrapOptional(requestKey)
.map(resolver.declarations::optionalBindings)
.ifPresent(optionalBindingDeclarations::addAll);

for (Key key : multibindingKeysMatchingRequest) {
multibindingContributions.addAll(resolver.getLocalMultibindingContributions(key));
multibindingDeclarations.addAll(resolver.declarations.multibindings(key));
}
}

// Add synthetic multibinding
Expand Down Expand Up @@ -410,41 +396,6 @@ private void addSubcomponentToOwningResolver(ContributionBinding subcomponentCre
owningResolver.componentDescriptor.getChildComponentWithBuilderType(builderType));
}

/**
* Profiling has determined that computing the keys matching {@code requestKey} has measurable
* performance impact. It is called repeatedly (at least 3 times per key resolved per {@link
* BindingGraph}. {@code javac}'s name-checking performance seems suboptimal (converting byte
* strings to Strings repeatedly), and the matching keys creations relies on that. This also
* ensures that the resulting keys have their hash codes cached on successive calls to this
* method.
*
* <p>This caching may become obsolete if:
*
* <ul>
* <li>We decide to intern all {@link Key} instances
* <li>We fix javac's name-checking peformance (though we may want to keep this for older
* javac users)
* </ul>
*/
private ImmutableSet<Key> multibindingKeysMatchingRequest(Key requestKey) {
return keysMatchingRequestCache.computeIfAbsent(
requestKey, this::multibindingKeysMatchingRequestUncached);
}

private ImmutableSet<Key> multibindingKeysMatchingRequestUncached(Key requestKey) {
ImmutableSet.Builder<Key> keys = ImmutableSet.builder();
keys.add(requestKey);
keyFactory.unwrapSetKey(requestKey, TypeNames.PRODUCED).ifPresent(keys::add);
keyFactory
.rewrapMapKey(requestKey, TypeNames.PRODUCER, TypeNames.PROVIDER)
.ifPresent(keys::add);
keyFactory
.rewrapMapKey(requestKey, TypeNames.PROVIDER, TypeNames.PRODUCER)
.ifPresent(keys::add);
keys.addAll(keyFactory.implicitFrameworkMapKeys(requestKey));
return keys.build();
}

private ImmutableSet<ContributionBinding> createDelegateBindings(
ImmutableSet<DelegateDeclaration> delegateDeclarations) {
ImmutableSet.Builder<ContributionBinding> builder = ImmutableSet.builder();
Expand Down Expand Up @@ -641,15 +592,7 @@ private ImmutableSet<ContributionBinding> getLocalExplicitBindings(Key key) {
private ImmutableSet<ContributionBinding> getLocalMultibindingContributions(Key key) {
return ImmutableSet.<ContributionBinding>builder()
.addAll(declarations.multibindingContributions(key))
// @Binds @IntoMap declarations have key Map<K, V>, unlike @Provides @IntoMap or @Produces
// @IntoMap, which have Map<K, Provider/Producer<V>> keys. So unwrap the key's type's
// value type if it's a Map<K, Provider/Producer<V>> before looking in
// delegate declarations. createDelegateBindings() will create bindings with the properly
// wrapped key type.
.addAll(
createDelegateBindings(
declarations.delegateMultibindingContributions(
keyFactory.unwrapMapValueType(key))))
.addAll(createDelegateBindings(declarations.delegateMultibindingContributions(key)))
.build();
}

Expand Down Expand Up @@ -868,12 +811,8 @@ private boolean hasLocalBindings(ResolvedBindings resolvedBindings) {
* this component's modules that matches the key.
*/
private boolean hasLocalMultibindingContributions(Key requestKey) {
return multibindingKeysMatchingRequest(requestKey).stream()
.anyMatch(
multibindingKey ->
!declarations.multibindingContributions(multibindingKey).isEmpty()
|| !declarations.delegateMultibindingContributions(
keyFactory.unwrapMapValueType(multibindingKey)).isEmpty());
return !declarations.multibindingContributions(requestKey).isEmpty()
|| !declarations.delegateMultibindingContributions(requestKey).isEmpty();
}

/**
Expand Down
Loading

0 comments on commit 93201e0

Please sign in to comment.