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 bazelbuild#19379.

PiperOrigin-RevId: 563679197
Change-Id: I542bbf56784832768ca3bbbd550cc78bfb981ffe
  • Loading branch information
fmeum authored and iancha1992 committed Sep 11, 2023
1 parent bc8bb95 commit c77296b
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 @@ -392,7 +392,16 @@ 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 @@ -417,4 +417,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 c77296b

Please sign in to comment.