Skip to content

Commit

Permalink
Fix a memory performance issue when creating Dagger's BindingGraph.
Browse files Browse the repository at this point in the history
This CL fixes an issues where each subcomponent was storing its complete list of accessible bindings, which includes any bindings it needs from its ancestor components. This ends up requiring much more space than actually required if we just store the bindings owned by the subcomponent and delegate to the parent component for any bindings accessible there.

I think this can likely be done for our `LegacyBindingGraph` classes as well, but I'll save that for another CL.

RELNOTES=N/A
PiperOrigin-RevId: 578306878
  • Loading branch information
bcorso authored and Dagger Team committed Oct 31, 2023
1 parent 871eda7 commit d9d0a8e
Showing 1 changed file with 38 additions and 32 deletions.
70 changes: 38 additions & 32 deletions java/dagger/internal/codegen/binding/BindingGraph.java
Original file line number Diff line number Diff line change
Expand Up @@ -212,14 +212,7 @@ private static BindingGraph create(
// particular BindingNode.
Map<Key, BindingNode> contributionBindings = new LinkedHashMap<>();
Map<Key, BindingNode> membersInjectionBindings = new LinkedHashMap<>();

// Construct the maps of the ContributionBindings and MembersInjectionBindings by iterating
// bindings from this component and then from each successive parent. If a binding exists in
// multple components, this order ensures that the child-most binding is always chosen first.
Stream.iterate(componentNode.componentPath(), ComponentPath::parent)
// Stream.iterate is inifinte stream so we need limit it to the known size of the path.
.limit(componentNode.componentPath().components().size())
.flatMap(path -> topLevelBindingGraph.bindingsByComponent().get(path).stream())
topLevelBindingGraph.bindingsByComponent().get(componentNode.componentPath())
.forEach(
bindingNode -> {
if (bindingNode.delegate() instanceof ContributionBinding) {
Expand All @@ -233,16 +226,19 @@ private static BindingGraph create(

BindingGraph bindingGraph = new AutoValue_BindingGraph(componentNode, topLevelBindingGraph);

ImmutableSet<ModuleDescriptor> modules =
((ComponentNodeImpl) componentNode).componentDescriptor().modules();
ImmutableSet<XTypeElement> modules =
((ComponentNodeImpl) componentNode).componentDescriptor().modules().stream()
.map(ModuleDescriptor::moduleElement)
.collect(toImmutableSet());

ImmutableSet<ModuleDescriptor> inheritedModules =
ImmutableSet<XTypeElement> inheritedModules =
parent.isPresent()
? Sets.union(parent.get().ownedModules, parent.get().inheritedModules).immutableCopy()
: ImmutableSet.of();

// Set these fields directly on the instance rather than passing these in as input to the
// AutoValue to prevent exposing this data outside of the class.
bindingGraph.parent = parent;
bindingGraph.inheritedModules = inheritedModules;
bindingGraph.ownedModules = Sets.difference(modules, inheritedModules).immutableCopy();
bindingGraph.contributionBindings = ImmutableMap.copyOf(contributionBindings);
Expand All @@ -257,10 +253,11 @@ private static BindingGraph create(
return bindingGraph;
}

private Optional<BindingGraph> parent;
private ImmutableMap<Key, BindingNode> contributionBindings;
private ImmutableMap<Key, BindingNode> membersInjectionBindings;
private ImmutableSet<ModuleDescriptor> inheritedModules;
private ImmutableSet<ModuleDescriptor> ownedModules;
private ImmutableSet<XTypeElement> inheritedModules;
private ImmutableSet<XTypeElement> ownedModules;
private ImmutableSet<XTypeElement> bindingModules;

BindingGraph() {}
Expand All @@ -287,9 +284,7 @@ public final ComponentDescriptor componentDescriptor() {
*/
public final Optional<Binding> localContributionBinding(Key key) {
return contributionBindings.containsKey(key)
? Optional.of(contributionBindings.get(key))
.filter(bindingNode -> bindingNode.componentPath().equals(componentPath()))
.map(BindingNode::delegate)
? Optional.of(contributionBindings.get(key).delegate())
: Optional.empty();
}

Expand All @@ -299,25 +294,31 @@ public final Optional<Binding> localContributionBinding(Key key) {
*/
public final Optional<Binding> localMembersInjectionBinding(Key key) {
return membersInjectionBindings.containsKey(key)
? Optional.of(membersInjectionBindings.get(key))
.filter(bindingNode -> bindingNode.componentPath().equals(componentPath()))
.map(BindingNode::delegate)
? Optional.of(membersInjectionBindings.get(key).delegate())
: Optional.empty();
}

/** Returns the {@link ContributionBinding} for the given {@link Key}. */
public final ContributionBinding contributionBinding(Key key) {
return (ContributionBinding) contributionBindings.get(key).delegate();
if (contributionBindings.containsKey(key)) {
return (ContributionBinding) contributionBindings.get(key).delegate();
} else if (parent.isPresent()) {
return parent.get().contributionBinding(key);
}
throw new AssertionError("Contribution binding not found for key: " + key);
}

/**
* Returns the {@link MembersInjectionBinding} for the given {@link Key} or {@link
* Optional#empty()} if one does not exist.
*/
public final Optional<MembersInjectionBinding> membersInjectionBinding(Key key) {
return membersInjectionBindings.containsKey(key)
? Optional.of((MembersInjectionBinding) membersInjectionBindings.get(key).delegate())
: Optional.empty();
if (membersInjectionBindings.containsKey(key)) {
return Optional.of((MembersInjectionBinding) membersInjectionBindings.get(key).delegate());
} else if (parent.isPresent()) {
return parent.get().membersInjectionBinding(key);
}
return Optional.empty();
}

/** Returns the {@link XTypeElement} for the component this graph represents. */
Expand All @@ -334,9 +335,7 @@ public final XTypeElement componentTypeElement() {
* ancestors.
*/
public final ImmutableSet<XTypeElement> ownedModuleTypes() {
return ownedModules.stream()
.map(ModuleDescriptor::moduleElement)
.collect(toImmutableSet());
return ownedModules;
}

/**
Expand Down Expand Up @@ -394,7 +393,7 @@ public ImmutableSet<ComponentRequirement> componentRequirements() {
ImmutableSet<XTypeElement> requiredModules =
stream(Traverser.forTree(BindingGraph::subgraphs).depthFirstPostOrder(this))
.flatMap(graph -> graph.bindingModules.stream())
.filter(ownedModuleTypes()::contains)
.filter(ownedModules::contains)
.collect(toImmutableSet());
ImmutableSet.Builder<ComponentRequirement> requirements = ImmutableSet.builder();
componentDescriptor().requirements().stream()
Expand Down Expand Up @@ -433,11 +432,18 @@ public ImmutableList<BindingNode> localBindingNodes() {
return topLevelBindingGraph().bindingsByComponent().get(componentPath());
}

@Memoized
// TODO(bcorso): This method can be costly. Consider removing this method and inlining it into its
// only usage, BindingGraphJsonGenerator.
public ImmutableSet<BindingNode> bindingNodes() {
return ImmutableSet.<BindingNode>builder()
.addAll(contributionBindings.values())
.addAll(membersInjectionBindings.values())
.build();
// Construct the set of bindings by iterating bindings from this component and then from each
// successive parent. If a binding exists in multiple components, this order ensures that the
// child-most binding is always chosen first.
Map<Key, BindingNode> bindings = new LinkedHashMap<>();
Stream.iterate(componentPath(), ComponentPath::parent)
// Stream.iterate() is infinite stream so we need limit it to the known size of the path.
.limit(componentPath().components().size())
.flatMap(path -> topLevelBindingGraph().bindingsByComponent().get(path).stream())
.forEach(bindingNode -> bindings.putIfAbsent(bindingNode.key(), bindingNode));
return ImmutableSet.copyOf(bindings.values());
}
}

0 comments on commit d9d0a8e

Please sign in to comment.