Skip to content

Commit

Permalink
Intern empty Depsets
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 and copybara-github committed Sep 6, 2023
1 parent 2067a8b commit fa0ff49
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public final class Depset implements StarlarkValue, Debug.ValueWithDebugAttribut
@Nullable private final Class<?> elemClass;
private final NestedSet<?> set;

private Depset(@Nullable Class<?> elemClass, NestedSet<?> set) {
Depset(@Nullable Class<?> elemClass, NestedSet<?> set) {
this.elemClass = elemClass;
this.set = set;
}
Expand Down Expand Up @@ -145,9 +145,10 @@ private static void checkElement(Object x, boolean strict) throws EvalException
// Object.class means "any Starlark value" but in fact allows any Java value.
public static <T> Depset of(Class<T> elemClass, NestedSet<T> set) {
Preconditions.checkNotNull(elemClass, "elemClass cannot be null");
// Having a shared EMPTY_DEPSET instance, could reduce working heap. Empty depsets are not
// retained though, because of optimization in StarlarkProvider.optimizeField.
return new Depset(set.isEmpty() ? null : ElementType.getTypeClass(elemClass), set);
if (set.isEmpty()) {
return set.getOrder().emptyDepset();
}
return new Depset(ElementType.getTypeClass(elemClass), set);
}

/**
Expand Down Expand Up @@ -259,15 +260,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 @@ -377,6 +374,9 @@ public 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(null, 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 @@ -406,4 +406,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 fa0ff49

Please sign in to comment.