diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java index 6098fdf8e0f669..dff42c49cf921b 100644 --- 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 targets = - ((PackageValue) packageValue).getPackage().getTargets(); + + ImmutableSortedMap 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 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 index d25a33b28f6e73..94ea4e6b45f90b 100644 --- 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 index 82918ada7d263d..7ab3395c2228ae 100644 --- 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