From b31b1d97622911000829bdfc9aba054194d8023f Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Thu, 31 Aug 2023 23:39:41 +0200
Subject: [PATCH] Intern empty `Depset`s

---
 .../build/lib/collect/nestedset/Depset.java   | 19 ++++++++++++++++---
 .../lib/collect/nestedset/DepsetTest.java     | 13 +++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java
index 13dd5cfa85ceb7..4ae649d33072b2 100644
--- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java
+++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java
@@ -13,13 +13,17 @@
 // limitations under the License.
 package com.google.devtools.build.lib.collect.nestedset;
 
+import static com.google.common.collect.Maps.toImmutableEnumMap;
+
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.docgen.annot.DocCategory;
 import com.google.devtools.build.docgen.annot.GlobalMethods;
 import com.google.devtools.build.docgen.annot.GlobalMethods.Environment;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
+import java.util.Arrays;
 import java.util.List;
 import javax.annotation.Nullable;
 import net.starlark.java.annot.Param;
@@ -97,6 +101,11 @@
             + " may interfere with the ordering semantics.")
 @Immutable
 public final class Depset implements StarlarkValue, Debug.ValueWithDebugAttributes {
+  private static final ImmutableMap<Order, Depset> EMPTY_DEPSETS =
+      Arrays.stream(Order.values())
+          .map(Order::emptySet)
+          .collect(toImmutableEnumMap(NestedSet::getOrder, e -> new Depset(null, e)));
+
   // The value of elemClass is set to getTypeClass(actualElemClass)
   // null is used for empty Depset-s
   @Nullable private final Class<?> elemClass;
@@ -145,9 +154,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 EMPTY_DEPSETS.get(set.getOrder());
+    }
+    return new Depset(ElementType.getTypeClass(elemClass), set);
   }
 
   /**
@@ -377,6 +387,9 @@ public static Depset fromDirectAndTransitive(
       }
     }
 
+    if (builder.isEmpty()) {
+      return EMPTY_DEPSETS.get(builder.getOrder());
+    }
     return new Depset(type, builder.build());
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/collect/nestedset/DepsetTest.java b/src/test/java/com/google/devtools/build/lib/collect/nestedset/DepsetTest.java
index 44b6a51b0fd140..87b18b02860b70 100644
--- a/src/test/java/com/google/devtools/build/lib/collect/nestedset/DepsetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/collect/nestedset/DepsetTest.java
@@ -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"));
+  }
 }