This change addresses the potential issues when some Package node is …

…removed from `InMemoryGraph` * When package value is missing for the removed package node; * When the node is still dirty; Refactor is also done in `InMemoryGraphImpl#remove(key)` method so that `ConcurrentHashMap` lookup only happens once. PiperOrigin-RevId: 524048911 Change-Id: I497bc3fb715d093579db92c7eee68bdcea798b19
fweikert · May 25, 2023 · 65d8860 · 65d8860
1 parent 6777cd2
commit 65d8860
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 23 deletions.
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
@@ -88,10 +88,11 @@ private InMemoryGraphImpl(int initialCapacity, boolean usePooledInterning) {
   @Override
   public void remove(SkyKey skyKey) {
     weakInternSkyKey(skyKey);
-    if (skyKey instanceof PackageValue.Key) {
-      weakInternPackageTargetsLabels((PackageValue.Key) skyKey);
+    InMemoryNodeEntry nodeEntry = nodeMap.remove(skyKey);
+    if (skyKey instanceof PackageValue.Key && nodeEntry != null) {
+      weakInternPackageTargetsLabels(
+          (PackageValue) nodeEntry.toValue()); // Dirty or changed value are needed.
     }
-    nodeMap.remove(skyKey);
   }
 
   @Override
@@ -102,7 +103,7 @@ public void removeIfDone(SkyKey key) {
           if (e.isDone()) {
             weakInternSkyKey(k);
             if (k instanceof PackageValue.Key) {
-              weakInternPackageTargetsLabels((PackageValue.Key) k);
+              weakInternPackageTargetsLabels((PackageValue) e.toValue());
             }
             return null;
           }
@@ -120,18 +121,16 @@ private void weakInternSkyKey(SkyKey skyKey) {
     }
   }
 
-  private void weakInternPackageTargetsLabels(PackageValue.Key packageKey) {
-    if (!usePooledInterning) {
+  private void weakInternPackageTargetsLabels(@Nullable PackageValue packageValue) {
+    if (!usePooledInterning || packageValue == null) {
       return;
     }
     LabelInterner interner = Label.getLabelInterner();
     if (interner == null) {
       return;
     }
-    SkyValue packageValue = nodeMap.get(packageKey).getValue();
-    checkState(packageValue instanceof PackageValue);
-    ImmutableSortedMap<String, Target> targets =
-        ((PackageValue) packageValue).getPackage().getTargets();
+
+    ImmutableSortedMap<String, Target> targets = packageValue.getPackage().getTargets();
     targets.values().forEach(t -> interner.weakIntern(t.getLabel()));
   }
 
@@ -262,7 +261,8 @@ public void cleanupInterningPool() {
                 || !e.getKey().functionName().equals(SkyFunctions.PACKAGE)) {
               return;
             }
-            weakInternPackageTargetsLabels((PackageValue.Key) e.getKey());
+
+            weakInternPackageTargetsLabels((PackageValue) e.toValue());
           });
     }
 
@@ -318,14 +318,11 @@ public Label getOrWeakIntern(Label sample) {
         return interner.weakIntern(sample);
       }
 
-      SkyValue value = inMemoryNodeEntry.getValue();
+      SkyValue value = inMemoryNodeEntry.toValue();
       checkState(value instanceof PackageValue, value);
       ImmutableSortedMap<String, Target> targets = ((PackageValue) value).getPackage().getTargets();
-      String labelName = sample.getName();
-
-      return targets.containsKey(labelName)
-          ? targets.get(labelName).getLabel()
-          : interner.weakIntern(sample);
+      Target target = targets.get(sample.getName());
+      return target != null ? target.getLabel() : interner.weakIntern(sample);
     }
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/BUILD b/src/test/java/com/google/devtools/build/lib/cmdline/BUILD
@@ -56,6 +56,8 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/skyframe:package_value",
+        "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
+        "//src/main/java/com/google/devtools/build/skyframe",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//src/test/java/com/google/devtools/build/lib/buildtool/util",
         "//third_party:guava",

diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java
@@ -13,9 +13,11 @@
 // limitations under the License.
 package com.google.devtools.build.lib.cmdline;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.truth.Truth.assertThat;
 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.devtools.build.lib.analysis.util.AnalysisMock;
@@ -24,26 +26,38 @@
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.skyframe.PackageValue;
+import com.google.devtools.build.lib.starlarkbuildapi.TargetApi;
+import com.google.devtools.build.skyframe.InMemoryGraph;
+import com.google.devtools.build.skyframe.NodeEntry;
+import com.google.devtools.build.skyframe.NodeEntry.DirtyType;
+import com.google.devtools.build.skyframe.QueryableGraph.Reason;
 import java.util.ArrayList;
 import java.util.List;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 @RunWith(JUnit4.class)
 public final class LabelInternerIntegrationTest extends SkyframeIntegrationTestBase {
-  @Test
-  public void labelInterner_noDuplicatesAndNoWeakInterned() throws Exception {
-    assumeFalse(AnalysisMock.get().isThisBazel());
+  private final LabelInterner labelInterner = Label.getLabelInterner();
 
-    LabelInterner labelInterner = Label.getLabelInterner();
+  @Before
+  public void setup() {
+    // Skip running with bazel due to cpp compilation is not currently supported on bazel.
+    // TODO(b/195425240): Remove the line below when bazel supports cpp compilation.
+    assumeFalse(AnalysisMock.get().isThisBazel());
     assertThat(labelInterner).isNotNull();
+  }
 
+  @Test
+  public void labelInterner_noDuplicatesAndNoWeakInterned() throws Exception {
     // Create a structure where package hello depends on dep1 and dep2, package dep1 also depends on
     // dep 2.
     write(
         "hello/BUILD",
-        "cc_binary(name = 'foo', srcs = ['foo.cc'], deps = ['//dep1:bar', '//dep2:baz'] )");
+        "cc_binary(name = 'foo', srcs = ['foo.cc'], deps = ['//dep1:bar', '//dep2:baz'])");
     write(
         "hello/foo.cc",
         "#include \"dep1/bar.h\"",
@@ -103,4 +117,40 @@ public void labelInterner_noDuplicatesAndNoWeakInterned() throws Exception {
       labelInterner.removeWeak(duplicate);
     }
   }
+
+  @Test
+  public void labelInterner_removeDirtyPackageStillWeakInternItsLabels() throws Exception {
+    write("hello/BUILD", "cc_binary(name = 'foo', srcs = ['foo.cc'])");
+    write("hello/foo.cc", "int main() {", "  return 0;", "}");
+    buildTarget("//hello:foo");
+
+    InMemoryGraph graph = skyframeExecutor().getEvaluator().getInMemoryGraph();
+
+    PackageValue.Key packageKey =
+        PackageValue.key(PackageIdentifier.createInMainRepo(/* name= */ "hello"));
+    NodeEntry nodeEntry = graph.get(/* requestor= */ null, Reason.OTHER, packageKey);
+    assertThat(nodeEntry).isNotNull();
+
+    ImmutableSet<Label> targetLabels =
+        ((PackageValue) nodeEntry.toValue())
+            .getPackage().getTargets().values().stream()
+                .map(TargetApi::getLabel)
+                .collect(toImmutableSet());
+
+    nodeEntry.markDirty(DirtyType.DIRTY);
+    graph.remove(packageKey);
+
+    // Expect removing dirty package node from node map will also weak intern labels associated with
+    // its targets. So re-weak intern these `targetLabels` should get its canonical instance.
+    assertThat(graph.get(/* requestor= */ null, Reason.OTHER, packageKey)).isNull();
+    targetLabels.forEach(
+        l ->
+            assertThat(Label.createUnvalidated(l.getPackageIdentifier(), l.getName()))
+                .isSameInstanceAs(l));
+  }
+
+  @After
+  public void cleanup() {
+    skyframeExecutor().getEvaluator().getInMemoryGraph().cleanupInterningPool();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/skyframe/BUILD b/src/test/java/com/google/devtools/build/skyframe/BUILD
@@ -55,9 +55,11 @@ java_library(
     deps = [
         ":testutil",
         "//src/main/java/com/google/devtools/build/lib/bugreport",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/events",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:package_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils",
         "//src/main/java/com/google/devtools/build/lib/util",
@@ -85,6 +87,7 @@ java_library(
 java_test(
     name = "SkyframeTests",
     size = "medium",
+    jvm_flags = ["-DBAZEL_USE_POOLED_LABEL_INTERNER=1"],
     shard_count = 2,
     test_class = "com.google.devtools.build.skyframe.AllTests",
     runtime_deps = [":skyframe_tests"],

diff --git a/src/test/java/com/google/devtools/build/skyframe/GraphTest.java b/src/test/java/com/google/devtools/build/skyframe/GraphTest.java
@@ -524,7 +524,7 @@ public void testDoneToDirty(@TestParameter BatchMethod batchMethod) throws Excep
     }
   }
 
-  private static DependencyState startEvaluation(NodeEntry entry) throws InterruptedException {
+  protected static DependencyState startEvaluation(NodeEntry entry) throws InterruptedException {
     return entry.addReverseDepAndCheckIfDone(null);
   }
 }
diff --git a/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java b/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java
@@ -17,6 +17,8 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.skyframe.PackageValue;
 import com.google.devtools.build.skyframe.InMemoryGraphImpl.EdgelessInMemoryGraphImpl;
 import com.google.devtools.build.skyframe.QueryableGraph.Reason;
 import org.junit.Test;
@@ -127,4 +129,26 @@ public void cleanupPool_weakInternerReintern() throws InterruptedException {
     // is created.
     assertThat(SkyKeyWithSkyKeyInterner.create("cat")).isSameInstanceAs(cat);
   }
+
+  @Test
+  public void removePackageNode_notPresentInGraph() throws Exception {
+    PackageIdentifier packageIdentifier = PackageIdentifier.createUnchecked("repo", "hello");
+    PackageValue.Key packageKey = PackageValue.key(packageIdentifier);
+
+    graph.remove(packageKey);
+    assertThat(graph.get(null, Reason.OTHER, packageKey)).isNull();
+  }
+
+  @Test
+  public void removePackageNode_noValueWeakInternLabelsNoCrash() throws Exception {
+    PackageIdentifier packageIdentifier = PackageIdentifier.createUnchecked("repo", "hello");
+    PackageValue.Key packageKey = PackageValue.key(packageIdentifier);
+
+    graph.createIfAbsentBatch(null, Reason.OTHER, ImmutableList.of(packageKey));
+    NodeEntry entry = graph.get(null, Reason.OTHER, packageKey);
+    assertThat(entry.toValue()).isNull();
+
+    graph.remove(packageKey);
+    assertThat(graph.get(null, Reason.OTHER, packageKey)).isNull();
+  }
 }