Skip to content

Commit

Permalink
Also apply NestedSet optimizations to Depset
Browse files Browse the repository at this point in the history
If the construction of a `NestedSet` involves only a single non-empty transitive set, this set may be returned directly as an optimization. This commit checks for this case when constructing a `Depset` and reuses the corresponding `Depset` if possible.

Since `Depset`s that are direct elements of providers are usually unwrapped into their `NestedSet`, this optimization only applies to instances stored in `struct`s as well as reduces allocations.

Realizes an optimization proposed by @brentleyjones in https://bazelbuild.slack.com/archives/CA31HN1T3/p1693487067454409?thread_ts=1693484609.534579&cid=CA31HN1T3.

Closes #19379.

PiperOrigin-RevId: 563679197
Change-Id: I542bbf56784832768ca3bbbd550cc78bfb981ffe
  • Loading branch information
fmeum authored and copybara-github committed Sep 8, 2023
1 parent d57e0a7 commit 216fce5
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,16 @@ public static Depset fromDirectAndTransitive(
if (builder.isEmpty()) {
return builder.getOrder().emptyDepset();
}
return new Depset(type, builder.build());
NestedSet<Object> set = builder.build();
// If the nested set was optimized to one of the transitive elements, reuse the corresponding
// depset.
for (Depset x : transitive) {
if (x.getSet() == set) {
return x;
}
}

return new Depset(type, set);
}

/** An exception thrown when validation fails on the type of elements of a nested set. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -419,4 +419,17 @@ public void testEmptyDepsetInternedPerOrder() throws Exception {
assertThat(ev.lookup("stable1")).isNotSameInstanceAs(ev.lookup("preorder1"));
assertThat(ev.lookup("stable2")).isNotSameInstanceAs(ev.lookup("preorder2"));
}

@Test
public void testSingleNonEmptyTransitiveAndNoDirectsUnwrapped() throws Exception {
ev.exec(
"inner = depset([1, 2, 3])", "outer = depset(transitive = [depset(), inner, depset()])");
assertThat(ev.lookup("outer")).isSameInstanceAs(ev.lookup("inner"));
}

@Test
public void testSingleNonEmptyTransitiveAndMatchingDirectUnwrapped() throws Exception {
ev.exec("inner = depset([1])", "outer = depset([1], transitive = [depset(), inner, depset()])");
assertThat(ev.lookup("outer")).isSameInstanceAs(ev.lookup("inner"));
}
}

0 comments on commit 216fce5

Please sign in to comment.