Remove blaze start-up flag UsePooledSkyKeyInterningFlag.

This commit also fixes some trivial typos in shell integration tests. PiperOrigin-RevId: 518379794 Change-Id: Iee6d06afb9468cc9d614610cd1719864dfe3fdd1
fweikert · Mar 21, 2023 · 65b3948 · 65b3948
1 parent 6ec2511
commit 65b3948
Show file tree

Hide file tree

Showing 13 changed files with 50 additions and 72 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -226,7 +226,8 @@ protected InMemoryMemoizingEvaluator createEvaluator(
         inconsistencyReceiver,
         trackIncrementalState ? DEFAULT_EVENT_FILTER_WITH_ACTIONS : EventFilter.NO_STORAGE,
         emittedEventState,
-        trackIncrementalState);
+        trackIncrementalState,
+        /* usePooledSkyKeyInterning= */ true);
   }
 
   @Override

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -141,6 +141,7 @@ public Diff getDiff(WalkableGraph fromGraph, Version fromVersion, Version toVers
   private final int nonSkyframeGlobbingThreads;
   @VisibleForTesting final ForkJoinPool forkJoinPoolForNonSkyframeGlobbing;
   private final int skyframeThreads;
+  private final boolean usePooledSkyKeyInterning;
 
   /** Abstract base class of a builder for {@link PackageLoader} instances. */
   public abstract static class Builder {
@@ -157,6 +158,7 @@ public abstract static class Builder {
     List<PrecomputedValue.Injected> extraPrecomputedValues = new ArrayList<>();
     int nonSkyframeGlobbingThreads = 1;
     int skyframeThreads = 1;
+    boolean usePooledSkyKeyInterning = true;
 
     protected Builder(
         Root workspaceDir,
@@ -242,6 +244,12 @@ public Builder setExternalFileAction(ExternalFileAction externalFileAction) {
       return this;
     }
 
+    @CanIgnoreReturnValue
+    public Builder disablePooledSkyKeyInterning() {
+      this.usePooledSkyKeyInterning = false;
+      return this;
+    }
+
     /** Throws {@link IllegalArgumentException} if builder args are incomplete/inconsistent. */
     protected void validate() {
       if (starlarkSemantics == null) {
@@ -273,6 +281,7 @@ public final PackageLoader build() {
         NamedForkJoinPool.newNamedPool(
             "package-loader-globbing-pool", builder.nonSkyframeGlobbingThreads);
     this.skyframeThreads = builder.skyframeThreads;
+    this.usePooledSkyKeyInterning = builder.usePooledSkyKeyInterning;
     this.directories = builder.directories;
     this.hashFunction = builder.workspaceDir.getFileSystem().getDigestFunction().getHashFunction();
 
@@ -408,7 +417,8 @@ private MemoizingEvaluator makeFreshEvaluator() {
         GraphInconsistencyReceiver.THROWING,
         EventFilter.FULL_STORAGE,
         new NestedSetVisitor.VisitedState(),
-        /*keepEdges=*/ false);
+        /* keepEdges= */ false,
+        usePooledSkyKeyInterning);
   }
 
   protected abstract ImmutableList<EnvironmentExtension> getEnvironmentExtensions();

diff --git a/src/main/java/com/google/devtools/build/skyframe/BUILD b/src/main/java/com/google/devtools/build/skyframe/BUILD
@@ -16,7 +16,6 @@ SKYFRAME_OBJECT_SRCS = [
     "SkyFunctionName.java",
     "SkyKey.java",
     "SkyValue.java",
-    "UsePooledSkyKeyInterningFlag.java",
     "Version.java",
 ]
 

diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
@@ -26,15 +26,19 @@ public interface InMemoryGraph extends ProcessableGraph, SkyKeyPool {
 
   /** Creates a new in-memory graph suitable for incremental builds. */
   static InMemoryGraph create() {
-    return new InMemoryGraphImpl();
+    return new InMemoryGraphImpl(/* usePooledSkyKeyInterning= */ true);
+  }
+
+  static InMemoryGraph create(boolean usePooledSkyKeyInterning) {
+    return new InMemoryGraphImpl(usePooledSkyKeyInterning);
   }
 
   /**
    * Creates a new in-memory graph that discards graph edges to save memory and cannot be used for
    * incremental builds.
    */
-  static InMemoryGraph createEdgeless() {
-    return new EdgelessInMemoryGraphImpl();
+  static InMemoryGraph createEdgeless(boolean usePooledSkyKeyInterning) {
+    return new EdgelessInMemoryGraphImpl(usePooledSkyKeyInterning);
   }
 
   @Override

diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.profiler.AutoProfiler;
 import com.google.devtools.build.lib.profiler.GoogleAutoProfilerUtils;
@@ -44,21 +43,22 @@ public class InMemoryGraphImpl implements InMemoryGraph {
   protected final ConcurrentHashMap<SkyKey, InMemoryNodeEntry> nodeMap;
   private final NodeBatch getBatch;
   private final NodeBatch createIfAbsentBatch;
-
-  // TODO(b/250641010): Remove this class member along with the startup flag.
   private final boolean usePooledSkyKeyInterning;
 
   InMemoryGraphImpl() {
     this(/* initialCapacity= */ 1 << 10);
   }
 
-  @VisibleForTesting
-  public InMemoryGraphImpl(boolean usePooledSkyKeyInterning) {
+  /**
+   * For some shell integration tests, we don't want to apply {@link SkyKeyInterner} created and
+   * bind {@code SkyKeyInterner#globalPool} to the second {@link InMemoryGraph}.
+   */
+  InMemoryGraphImpl(boolean usePooledSkyKeyInterning) {
     this(/* initialCapacity= */ 1 << 10, usePooledSkyKeyInterning);
   }
 
   protected InMemoryGraphImpl(int initialCapacity) {
-    this(initialCapacity, UsePooledSkyKeyInterningFlag.usePooledSkyKeyInterningFlag());
+    this(initialCapacity, /* usePooledSkyKeyInterning= */ true);
   }
 
   private InMemoryGraphImpl(int initialCapacity, boolean usePooledSkyKeyInterning) {
@@ -217,7 +217,7 @@ public SkyKey getOrWeakIntern(SkyKey key) {
   @Override
   public void cleanupPool() {
     if (!usePooledSkyKeyInterning) {
-      // No clean up is needed when UseSkyKeyInternerFlag startup flag is off.
+      // No clean up is needed when `usePooledSkyKeyInterning` is false for shell integration tests.
       return;
     }
     try (AutoProfiler ignored =
@@ -231,11 +231,6 @@ public void cleanupPool() {
 
   static final class EdgelessInMemoryGraphImpl extends InMemoryGraphImpl {
 
-    public EdgelessInMemoryGraphImpl() {
-      super();
-    }
-
-    @VisibleForTesting
     public EdgelessInMemoryGraphImpl(boolean usePooledSkyKeyInterning) {
       super(usePooledSkyKeyInterning);
     }

diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -92,7 +92,8 @@ public InMemoryMemoizingEvaluator(
         GraphInconsistencyReceiver.THROWING,
         EventFilter.FULL_STORAGE,
         new NestedSetVisitor.VisitedState(),
-        /*keepEdges=*/ true);
+        /* keepEdges= */ true,
+        /* usePooledSkyKeyInterning= */ true);
   }
 
   public InMemoryMemoizingEvaluator(
@@ -102,13 +103,17 @@ public InMemoryMemoizingEvaluator(
       GraphInconsistencyReceiver graphInconsistencyReceiver,
       EventFilter eventFilter,
       NestedSetVisitor.VisitedState emittedEventState,
-      boolean keepEdges) {
+      boolean keepEdges,
+      boolean usePooledSkyKeyInterning) {
     this.skyFunctions = ImmutableMap.copyOf(skyFunctions);
     this.differencer = Preconditions.checkNotNull(differencer);
     this.progressReceiver = new DirtyTrackingProgressReceiver(progressReceiver);
     this.graphInconsistencyReceiver = Preconditions.checkNotNull(graphInconsistencyReceiver);
     this.eventFilter = eventFilter;
-    this.graph = keepEdges ? InMemoryGraph.create() : InMemoryGraph.createEdgeless();
+    this.graph =
+        keepEdges
+            ? InMemoryGraph.create(usePooledSkyKeyInterning)
+            : InMemoryGraph.createEdgeless(usePooledSkyKeyInterning);
     this.emittedEventState = emittedEventState;
     this.keepEdges = keepEdges;
   }

diff --git a/src/main/java/com/google/devtools/build/skyframe/UsePooledSkyKeyInterningFlag.java b/src/main/java/com/google/devtools/build/skyframe/UsePooledSkyKeyInterningFlag.java
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -275,7 +275,7 @@ protected BuilderWithResult createBuilder(
                         null,
                         null,
                         null,
-                        /*packageProgress=*/ null,
+                        /* packageProgress= */ null,
                         PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException
                             .INSTANCE,
                         GlobbingStrategy.SKYFRAME_HYBRID,
@@ -295,7 +295,7 @@ protected BuilderWithResult createBuilder(
                             .builder(directories)
                             .build(TestRuleClassProvider.getRuleClassProvider(), fileSystem),
                         directories,
-                        /*bzlLoadFunctionForInlining=*/ null))
+                        /* bzlLoadFunctionForInlining= */ null))
                 .put(
                     SkyFunctions.EXTERNAL_PACKAGE,
                     new ExternalPackageFunction(
@@ -306,14 +306,15 @@ protected BuilderWithResult createBuilder(
                 .put(
                     SkyFunctions.ARTIFACT_NESTED_SET,
                     ArtifactNestedSetFunction.createInstance(
-                        /*valueBasedChangePruningEnabled=*/ true))
+                        /* valueBasedChangePruningEnabled= */ true))
                 .buildOrThrow(),
             differencer,
             evaluationProgressReceiver,
             graphInconsistencyReceiver,
             EventFilter.FULL_STORAGE,
             new NestedSetVisitor.VisitedState(),
-            /*keepEdges=*/ true);
+            /* keepEdges= */ true,
+            /* usePooledSkyKeyInterning= */ true);
     PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID());
     PrecomputedValue.ACTION_ENV.set(differencer, ImmutableMap.of());
     PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get());

diff --git a/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java b/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java
@@ -38,7 +38,7 @@ protected Version getNextVersion(Version v) {
 
   @Override
   protected void makeGraph() {
-    graph = new InMemoryGraphImpl(/* usePooledSkyKeyInterning= */ true);
+    graph = new InMemoryGraphImpl();
   }
 
   @Override

diff --git a/src/test/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluatorTest.java
@@ -35,6 +35,7 @@ protected InMemoryMemoizingEvaluator getMemoizingEvaluator(
         graphInconsistencyReceiver,
         eventFilter,
         emittedEventState,
-        /*keepEdges=*/ true);
+        /* keepEdges= */ true,
+        /* usePooledSkyKeyInterning= */ true);
   }
 }
diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -1004,7 +1004,10 @@ public void catastrophicBuild(@TestParameter boolean keepGoing, @TestParameter b
       throws Exception {
     Assume.assumeTrue(keepGoing || keepEdges);
 
-    graph = keepEdges ? InMemoryGraph.create() : InMemoryGraph.createEdgeless();
+    graph =
+        keepEdges
+            ? InMemoryGraph.create(/* usePooledSkyKeyInterning= */ true)
+            : InMemoryGraph.createEdgeless(/* usePooledSkyKeyInterning= */ true);
 
     SkyKey catastropheKey = GraphTester.toSkyKey("catastrophe");
     SkyKey otherKey = GraphTester.toSkyKey("someKey");

diff --git a/src/test/shell/integration/discard_graph_edges_test.sh b/src/test/shell/integration/discard_graph_edges_test.sh
@@ -260,7 +260,7 @@ function test_packages_cleared() {
   local edgeless_entry_count="$(extract_histogram_count "$histo_file" \
        'EdgelessInMemoryNodeEntry')"
   [[ "$edgeless_entry_count" -eq 0 ]] \
-      || fail "$edgless_entry_count EdgelessInMemoryNodeEntry instances found in build keeping edges"
+      || fail "$edgeless_entry_count EdgelessInMemoryNodeEntry instances found in build keeping edges"
   local node_entry_count="$(extract_histogram_count "$histo_file" \
        '\.InMemoryNodeEntry')"
   [[ "$node_entry_count" -ge 100 ]] \

diff --git a/src/test/shell/integration/starlark_configurations_test.sh b/src/test/shell/integration/starlark_configurations_test.sh
@@ -112,8 +112,8 @@ EOF
 #### TESTS #############################################################
 
 function test_default_flag() {
- local -r pkg=$FUNCNAME
- mkdir -p $pkg
+  local -r pkg=$FUNCNAME
+  mkdir -p $pkg
 
   write_build_setting_bzl