Automated rollback of commit 990d97e.

*** Reason for rollback ***

This is a roll forward CL which addressed the issue reported in b/289354550#comment9. I added another `LabelInterner#enabled()` method to check whether `globalPool` was set or not. If not, then `SkyframeExecutor` should not remove any labels from weak interner when PAKCAGE node is done.

A unit test is also added to verify the condition when `LabelInterner#enable()` returns false.

*** Original change description ***

Automated rollback of commit 0bda661.

*** Reason for rollback ***

b/289354550

*** Original change description ***

Clean up Label Interner flag and relevant unused code

PiperOrigin-RevId: 545793223
Change-Id: I4254c6ed910e31c2702a26ddec9c334470f7e1bf

src/main/java/com/google/devtools/build/lib/cmdline/Label.java

@@ -19,7 +19,6 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ComparisonChain;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Interner;
 import com.google.common.util.concurrent.Striped;
 import com.google.devtools.build.docgen.annot.DocCategory;
 import com.google.devtools.build.lib.actions.CommandLineItem;
@@ -34,7 +33,6 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
-import com.google.devtools.build.skyframe.UsePooledLabelInterningFlag;
 import java.util.Arrays;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReadWriteLock;
@@ -86,16 +84,10 @@ public final class Label implements Comparable<Label>, StarlarkValue, SkyKey, Co
   public static final SkyFunctionName TRANSITIVE_TRAVERSAL =
       SkyFunctionName.createHermetic("TRANSITIVE_TRAVERSAL");
 
-  @Nullable
-  private static final LabelInterner pooledInterner =
-      UsePooledLabelInterningFlag.usePooledLabelInterningFlag() ? new LabelInterner() : null;
+  private static final LabelInterner interner = new LabelInterner();
 
-  private static final Interner<Label> interner =
-      pooledInterner != null ? pooledInterner : BlazeInterners.newWeakInterner();
-
-  @Nullable
   public static LabelInterner getLabelInterner() {
-    return pooledInterner;
+    return interner;
   }
 
   /** The context of a current repo, necessary to parse a repo-relative label ("//foo:bar"). */
@@ -696,5 +688,9 @@ public Lock getLockForLabelTransferToPool(PackageIdentifier packageIdentifier) {
     protected Pool<Label> getPool() {
       return globalPool;
     }
+
+    public boolean enabled() {
+      return globalPool != null;
+    }
   }
 }

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -2906,7 +2906,7 @@ public void evaluated(
       // should have been added to the InMemoryGraph. So it is safe to remove relevant labels from
       // weak interner.
       LabelInterner labelInterner = Label.getLabelInterner();
-      if (labelInterner != null
+      if (labelInterner.enabled()
           && skyKey.functionName().equals(SkyFunctions.PACKAGE)
           && newValue != null) {
         checkState(newValue instanceof PackageValue, newValue);

src/main/java/com/google/devtools/build/skyframe/BUILD

@@ -17,7 +17,6 @@ SKYFRAME_OBJECT_SRCS = [
     "SkyFunctionName.java",
     "SkyKey.java",
     "SkyValue.java",
-    "UsePooledLabelInterningFlag.java",
     "Version.java",
 ]
 

src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java

@@ -80,9 +80,7 @@ private InMemoryGraphImpl(int initialCapacity, boolean usePooledInterning) {
     this.usePooledInterning = usePooledInterning;
     if (usePooledInterning) {
       SkyKeyInterner.setGlobalPool(new SkyKeyPool());
-      if (UsePooledLabelInterningFlag.usePooledLabelInterningFlag()) {
-        LabelInterner.setGlobalPool(new LabelPool());
-      }
+      LabelInterner.setGlobalPool(new LabelPool());
     }
   }
 
@@ -127,9 +125,6 @@ private void weakInternPackageTargetsLabels(@Nullable PackageValue packageValue)
       return;
     }
     LabelInterner interner = Label.getLabelInterner();
-    if (interner == null) {
-      return;
-    }
 
     ImmutableSortedMap<String, Target> targets = packageValue.getPackage().getTargets();
     targets.values().forEach(t -> interner.weakIntern(t.getLabel()));
@@ -257,9 +252,7 @@ public void cleanupInterningPool() {
           e -> {
             weakInternSkyKey(e.getKey());
 
-            if (!UsePooledLabelInterningFlag.usePooledLabelInterningFlag()
-                || !e.isDone()
-                || !e.getKey().functionName().equals(SkyFunctions.PACKAGE)) {
+            if (!e.isDone() || !e.getKey().functionName().equals(SkyFunctions.PACKAGE)) {
               return;
             }
 

src/test/java/com/google/devtools/build/lib/cmdline/BUILD

@@ -50,7 +50,6 @@ java_test(
 java_test(
     name = "LabelInternerIntegrationTest",
     srcs = ["LabelInternerIntegrationTest.java"],
-    jvm_flags = ["-DBAZEL_USE_POOLED_LABEL_INTERNER=1"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/concurrent",

src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java

@@ -15,11 +15,13 @@
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.truth.Truth.assertThat;
+import static java.util.stream.Collectors.toCollection;
 import static org.junit.Assume.assumeFalse;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.analysis.util.AnalysisMock;
 import com.google.devtools.build.lib.buildtool.util.SkyframeIntegrationTestBase;
 import com.google.devtools.build.lib.cmdline.Label.LabelInterner;
@@ -33,6 +35,7 @@
 import com.google.devtools.build.skyframe.QueryableGraph.Reason;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -173,6 +176,47 @@ public void labelInterner_removeDirtyPackageStillWeakInternItsLabels() throws Ex
                 .isSameInstanceAs(l));
   }
 
+  /**
+   * This test case addresses b/289354550.
+   *
+   * <p>Label interner can sometimes be disabled when blaze does not use {@link InMemoryGraph}. This
+   * test case deliberately disables label interner and check identical label are always weak
+   * interned.
+   */
+  @Test
+  public void labelInterner_alwaysRespectWeakInternerWhenLabelInternerDisabled() throws Exception {
+    write("hello/BUILD", "genrule(name = 'foo', outs = ['out'], cmd = '/bin/echo hello > $@')");
+    // Deliberately set label interner's global pool to null to disable labelInterner.
+    LabelInterner.setGlobalPool(null);
+    assertThat(labelInterner.enabled()).isFalse();
+
+    // Target label //hello:foo is definitely created when build hello:foo target. So create it
+    // before target build to ensure it is stored in weak interner.
+    Label preBuildLabel = Label.parseCanonical("//hello:foo");
+    buildTarget("//hello:foo");
+
+    InMemoryGraph graph = skyframeExecutor().getEvaluator().getInMemoryGraph();
+    PackageIdentifier packageKey = PackageIdentifier.createInMainRepo(/* name= */ "hello");
+    NodeEntry nodeEntry = graph.get(/* requestor= */ null, Reason.OTHER, packageKey);
+    assertThat(nodeEntry).isNotNull();
+
+    Set<Label> targetLabels =
+        ((PackageValue) nodeEntry.toValue())
+            .getPackage().getTargets().values().stream()
+                .map(TargetApi::getLabel)
+                .collect(toCollection(Sets::newIdentityHashSet));
+
+    // Target label //hello:foo stored in InMemoryGraph should exist and be the same instance as
+    // what is created for weak interner in advance.
+    assertThat(targetLabels).contains(preBuildLabel);
+
+    // Post build, we still need to make sure that target label //hello:foo was not removed from
+    // weak interner's underlying map. So create a new //hello:foo target label and expect it to be
+    // the same instance as the original one.
+    Label postBuildLabel = Label.parseCanonical("//hello:foo");
+    assertThat(postBuildLabel).isSameInstanceAs(preBuildLabel);
+  }
+
   @After
   public void cleanup() {
     skyframeExecutor().getEvaluator().getInMemoryGraph().cleanupInterningPool();

src/test/java/com/google/devtools/build/skyframe/BUILD

@@ -87,7 +87,6 @@ java_library(
 java_test(
     name = "SkyframeTests",
     size = "medium",
-    jvm_flags = ["-DBAZEL_USE_POOLED_LABEL_INTERNER=1"],
     shard_count = 2,
     tags = ["not_run:arm"],
     test_class = "com.google.devtools.build.skyframe.AllTests",