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.
  • Loading branch information
fmeum committed Aug 31, 2023
1 parent 9e7aa13 commit 64297d6
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 @@ -21,6 +21,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import java.util.List;
import java.util.Optional;
import javax.annotation.Nullable;
import net.starlark.java.annot.Param;
import net.starlark.java.annot.ParamType;
Expand Down Expand Up @@ -365,6 +366,7 @@ public static Depset fromDirectAndTransitive(
builder.addAll(direct);

// Add transitive sets, checking that type is equal to elements already added.
long nonEmptyTransitives = 0;
for (Depset x : transitive) {
if (!x.isEmpty()) {
type = checkType(type, x.elemClass);
Expand All @@ -374,10 +376,22 @@ public static Depset fromDirectAndTransitive(
order.getStarlarkName(), x.getOrder().getStarlarkName());
}
builder.addTransitive(x.getSet());
nonEmptyTransitives++;
}
}

return new Depset(type, builder.build());
NestedSet<Object> set = builder.build();
// If the nested set was optimized to the only non-empty transitive element, reuse the
// corresponding depset.
if (nonEmptyTransitives == 1) {
Optional<Depset> unwrappedTransitive =
transitive.stream().filter(x -> x.getSet() == set).findFirst();
if (unwrappedTransitive.isPresent()) {
return unwrappedTransitive.get();
}
}

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 @@ -406,4 +406,12 @@ ev.new Scenario()
+ " NoneType'",
"depset(direct='hello')");
}

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

0 comments on commit 64297d6

Please sign in to comment.