Skip to content

Commit

Permalink
[6.4.0] Intern empty Depsets (#19443)
Browse files Browse the repository at this point in the history
Empty `Depset`s are interned per order, which decreases allocations as
well as retained heap when a `Depset` is stored in a `struct` in a
provider (providers with schema already unwrap most `Depset`s).

Fixes #19380

Closes #19387.

PiperOrigin-RevId: 563104923
Change-Id: If66fb4a108ef7569a4f95e7af3a74ac84d1c4636
  • Loading branch information
fmeum authored Sep 11, 2023
1 parent fb4cc46 commit bc8bb95
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public final class Depset implements StarlarkValue, Debug.ValueWithDebugAttribut
private final ElementType elemType;
private final NestedSet<?> set;

private Depset(ElementType elemType, NestedSet<?> set) {
Depset(ElementType elemType, NestedSet<?> set) {
this.elemType = Preconditions.checkNotNull(elemType, "element type cannot be null");
this.set = set;
}
Expand Down Expand Up @@ -189,6 +189,9 @@ private static void checkElement(Object x, boolean strict) throws EvalException
// two arguments: of(Class<T> elemType, NestedSet<T> set). We could also avoid the allocations
// done by ElementType.of().
public static <T> Depset of(ElementType elemType, NestedSet<T> set) {
if (set.isEmpty()) {
return set.getOrder().emptyDepset();
}
return new Depset(elemType, set);
}

Expand Down Expand Up @@ -280,15 +283,11 @@ public static <T> NestedSet<T> cast(Object x, Class<T> type, String what) throws
public static <T> NestedSet<T> noneableCast(Object x, Class<T> type, String what)
throws EvalException {
if (x == Starlark.NONE) {
@SuppressWarnings("unchecked")
NestedSet<T> empty = (NestedSet<T>) EMPTY;
return empty;
return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
}
return cast(x, type, what);
}

private static final NestedSet<?> EMPTY = NestedSetBuilder.<Object>emptySet(Order.STABLE_ORDER);

public boolean isEmpty() {
return set.isEmpty();
}
Expand Down Expand Up @@ -390,6 +389,9 @@ static Depset fromDirectAndTransitive(
}
}

if (builder.isEmpty()) {
return builder.getOrder().emptyDepset();
}
return new Depset(type, builder.build());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,12 @@ public enum Order {

private final String starlarkName;
private final NestedSet<?> emptySet;
private final Depset emptyDepset;

private Order(String starlarkName) {
Order(String starlarkName) {
this.starlarkName = starlarkName;
this.emptySet = new NestedSet<>(this);
this.emptyDepset = new Depset(Depset.ElementType.EMPTY, this.emptySet);
}

@SerializationConstant @AutoCodec.VisibleForSerialization
Expand Down Expand Up @@ -150,6 +152,11 @@ <E> NestedSet<E> emptySet() {
return (NestedSet<E>) emptySet;
}

/** Returns an empty depset of the given ordering. */
Depset emptyDepset() {
return emptyDepset;
}

public String getStarlarkName() {
return starlarkName;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -404,4 +404,17 @@ ev.new Scenario()
+ " NoneType'",
"depset(direct='hello')");
}

@Test
public void testEmptyDepsetInternedPerOrder() throws Exception {
ev.exec(
"stable1 = depset()",
"stable2 = depset()",
"preorder1 = depset(order = 'preorder')",
"preorder2 = depset(order = 'preorder')");
assertThat(ev.lookup("stable1")).isSameInstanceAs(ev.lookup("stable2"));
assertThat(ev.lookup("preorder1")).isSameInstanceAs(ev.lookup("preorder2"));
assertThat(ev.lookup("stable1")).isNotSameInstanceAs(ev.lookup("preorder1"));
assertThat(ev.lookup("stable2")).isNotSameInstanceAs(ev.lookup("preorder2"));
}
}

0 comments on commit bc8bb95

Please sign in to comment.