Skip to content

Commit

Permalink
Exclude ActionLookupData nodes in the active set from uploads.
Browse files Browse the repository at this point in the history 
The frontier and under frontier nodes are still being uploaded.

PiperOrigin-RevId: 699973151
Change-Id: I1f9a971fed2bd142946063eb551d05bca08cb421
  • Loading branch information
@jin @copybara-github
jin authored and copybara-github committed Nov 25, 2024
1 parent 649f8a2 commit 06fab2f
Show file tree
Hide file tree
Showing 4 changed files with 240 additions and 5 deletions.
23 changes: 18 additions & 5 deletions ...ava/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,22 @@ public static Optional<FailureDetail> serializeAndUploadFrontier(
return;
}

// Filter for ActionExecutionValues owned by active analysis nodes and skip them, because
// they should be evaluated locally.
if (key instanceof ActionLookupData ald) {
switch (selection.get(ald.getActionLookupKey())) {
case ACTIVE: // Active set. Always evaluated locally.
return;
case FRONTIER_CANDIDATE:
case null:
// null / under the frontier: still necessary as inputs to nested sets / actions,
// and the parent ActionLookupKey might not be available..
break;
}
}

// TODO: b/371508153 - only upload nodes that were freshly computed by this invocation and
// unaffected by local, un-submitted changes.

try {
SerializationResult<ByteString> keyBytes =
codecs.serializeMemoizedAndBlocking(fingerprintValueService, key, profileCollector);
Expand Down Expand Up @@ -269,7 +282,7 @@ static ConcurrentHashMap<SkyKey, SelectionMarking> computeSelection(
// are never built by Skyframe directly, and the function will return
// ActionLookupData as the canonical key for those artifacts instead.
SkyKey aKey = Artifact.key(key);
if (Artifact.key(key) instanceof ActionLookupData) {
if (aKey instanceof ActionLookupData) {
// Already handled in the ActionLookupData switch case above.
return;
}
Expand Down Expand Up @@ -314,7 +327,7 @@ private static void markActiveAndTraverseEdges(
}

for (SkyKey dep : node.getDirectDeps()) {
if (!(dep instanceof ActionLookupKey child)) {
if (!(dep instanceof ActionLookupKey actionLookupKey)) {
continue;
}

Expand All @@ -329,8 +342,8 @@ private static void markActiveAndTraverseEdges(
//
// In all cases, frontier candidates will never include nodes in the active directories. This
// is enforced after selection completes.
if (child.getLabel() != null) {
selection.putIfAbsent(child, FRONTIER_CANDIDATE);
if (actionLookupKey.getLabel() != null) {
selection.putIfAbsent(actionLookupKey, FRONTIER_CANDIDATE);
}
}
for (SkyKey rdep : node.getReverseDepsForDoneEntry()) {
Expand Down
5 changes: 5 additions & 0 deletions ...evtools/build/lib/skyframe/serialization/analysis/RemoteAnalysisCachingEventListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.collect.HashMultiset;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multiset;
import com.google.common.eventbus.AllowConcurrentEvents;
import com.google.common.eventbus.Subscribe;
Expand Down Expand Up @@ -77,6 +78,10 @@ public int getSerializedKeysCount() {
return serializedKeys.size();
}

public Set<SkyKey> getSerializedKeys() {
return ImmutableSet.copyOf(serializedKeys);
}

@ThreadSafe
public void recordRetrievalResult(RetrievalResult result, SkyKey key) {
if (result instanceof Restart) {
Expand Down
3 changes: 3 additions & 0 deletions src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ java_library(
srcs = ["FrontierSerializerTestBase.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data",
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
"//src/main/java/com/google/devtools/build/lib/cmdline",
Expand All @@ -26,6 +27,7 @@ java_library(
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/test/java/com/google/devtools/build/lib/buildtool/util",
"//third_party:error_prone_annotations",
"//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
Expand All @@ -37,6 +39,7 @@ java_library(
java_test(
name = "FrontierSerializerTest",
srcs = ["FrontierSerializerTest.java"],
tags = ["no_windows"], # b/380393822 - disable until PathFragmentPrefixTrie handles windows correctly.
deps = [
":FrontierSerializerTestBase",
"//src/main/java/com/google/devtools/build/lib:runtime",
Expand Down
214 changes: 214 additions & 0 deletions ...google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,20 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.devtools.build.lib.cmdline.Label.parseCanonicalUnchecked;
import static java.util.stream.Collectors.joining;
import static org.junit.Assert.assertThrows;
import static org.junit.Assume.assumeTrue;

import com.google.common.base.Ascii;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionLookupData;
import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
import com.google.devtools.build.lib.buildtool.BuildTool;
import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
import com.google.devtools.build.lib.runtime.commands.CqueryCommand;
Expand All @@ -45,6 +48,7 @@
import com.google.errorprone.annotations.ForOverride;
import com.google.perftools.profiles.ProfileProto.Profile;
import com.google.protobuf.ExtensionRegistry;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.HashMap;
Expand Down Expand Up @@ -698,6 +702,216 @@ protected void setupScenarioWithConfiguredTargets() throws Exception {
filegroup(name = "F", srcs = ["//foo:G"])
filegroup(name = "H")
filegroup(name = "I")
""");
}

protected static <T> ImmutableSet<T> filterKeys(Set<SkyKey> from, Class<? extends T> klass) {
return from.stream().filter(klass::isInstance).map(klass::cast).collect(toImmutableSet());
}

protected static ImmutableSet<Label> getLabels(Set<ActionLookupKey> from) {
return from.stream().map(ActionLookupKey::getLabel).collect(toImmutableSet());
}

protected static ImmutableSet<Label> getOwningLabels(Set<ActionLookupData> from) {
return from.stream()
.map(data -> data.getActionLookupKey().getLabel())
.collect(toImmutableSet());
}

@Test
public void actionLookupKey_ownedByActiveSetAndUnderFrontier_areNotUploaded() throws Exception {
setupGenruleGraph();
addOptions("--experimental_remote_analysis_cache_mode=upload");
buildTarget("//A");
var serializedKeys =
getCommandEnvironment().getRemoteAnalysisCachingEventListener().getSerializedKeys();
ImmutableSet<Label> labels = getLabels(filterKeys(serializedKeys, ActionLookupKey.class));

// Active set
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//A"));

// Frontier
assertThat(labels)
.containsAtLeast(
parseCanonicalUnchecked("//C:C.txt"), // output file CT
parseCanonicalUnchecked("//E"));

// Under the frontier
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//C"));
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//D"));

// Different top level target
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//B"));
}

@Test
public void frontierSelectionSucceeds_forTopLevelGenruleConfiguredTargetWithUniqueName()
throws Exception {
setupGenruleGraph();
write(
"A/BUILD",
"""
genrule(
name = "copy_of_A", # renamed
srcs = ["in.txt", "//C:C.txt", "//E"],
outs = ["A"],
cmd = "cat $(SRCS) > $@",
)
""");
addOptions("--experimental_remote_analysis_cache_mode=upload");
buildTarget("//A");
var serializedKeys =
getCommandEnvironment().getRemoteAnalysisCachingEventListener().getSerializedKeys();
ImmutableSet<Label> labels = getLabels(filterKeys(serializedKeys, ActionLookupKey.class));

// Active set
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//A"));
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//A:copy_of_A"));
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//A:in.txt"));

// Frontier
assertThat(labels)
.containsAtLeast(parseCanonicalUnchecked("//C:C.txt"), parseCanonicalUnchecked("//E"));

// Under the frontier
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//C"));
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//D"));

// Different top level target
assertThat(labels).doesNotContain(parseCanonicalUnchecked("//B"));
}

@Test
public void dumpUploadManifestOnlyMode_forTopLevelGenruleConfiguredTarget() throws Exception {
setupGenruleGraph();
write(
"A/BUILD",
"""
genrule(
name = "copy_of_A",
srcs = ["in.txt", "//C:C.txt", "//E"],
outs = ["A"],
cmd = "cat $(SRCS) > $@",
)
""");

addOptions("--experimental_remote_analysis_cache_mode=dump_upload_manifest_only");

RecordingOutErr outErr = new RecordingOutErr();
this.outErr = outErr;

buildTarget("//A");

// Note that there are two //A:A - one each for target and exec configuration. The
// BuildConfigurationKey is omitted because it's too specific, but we test for the
// exact number of entries in the manifest later, so the two //A:A configured targets will be
// counted correctly.
var expectedActiveSet =
"""
ACTIVE: CONFIGURED_TARGET:ConfiguredTargetKey{label=//A:copy_of_A, config=
ACTIVE: CONFIGURED_TARGET:ConfiguredTargetKey{label=//A:A, config=
ACTIVE: CONFIGURED_TARGET:ConfiguredTargetKey{label=//A:A, config=
ACTIVE: CONFIGURED_TARGET:ConfiguredTargetKey{label=//A:in.txt, config=null}
"""
.lines()
.collect(toImmutableList());

var actualActiveSet =
outErr.outAsLatin1().lines().filter(l -> l.startsWith("ACTIVE:")).collect(joining("\n"));

expectedActiveSet.forEach(line -> assertThat(actualActiveSet).contains(line));

assertThat(actualActiveSet.lines()).hasSize(expectedActiveSet.size());
}

@Test
public void actionLookupData_ownedByActiveSet_areNotUploaded() throws Exception {
setupGenruleGraph();
addOptions("--experimental_remote_analysis_cache_mode=upload");
buildTarget("//A");
var serializedKeys =
getCommandEnvironment().getRemoteAnalysisCachingEventListener().getSerializedKeys();
var actionLookupDatas = filterKeys(serializedKeys, ActionLookupData.class);
var owningLabels = getOwningLabels(actionLookupDatas);

// Active set
assertThat(owningLabels).doesNotContain(parseCanonicalUnchecked("//A"));

// Frontier
assertThat(owningLabels)
.containsAtLeast(parseCanonicalUnchecked("//C"), parseCanonicalUnchecked("//E"));

// Under the frontier
assertThat(owningLabels).contains(parseCanonicalUnchecked("//D"));

// Different top level target
assertThat(owningLabels).doesNotContain(parseCanonicalUnchecked("//B"));
}

protected final void setupGenruleGraph() throws IOException {
// /--> E
// A -> C -> D
// B ---^
write("A/in.txt", "A");
write(
"A/BUILD",
"""
genrule(
name = "A",
srcs = ["in.txt", "//C:C.txt", "//E"],
outs = ["A"],
cmd = "cat $(SRCS) > $@",
)
""");
write("B/in.txt", "B");
write(
"B/BUILD",
"""
genrule(
name = "B",
srcs = ["in.txt", "//C:C.txt"],
outs = ["B"],
cmd = "cat $(SRCS) > $@",
)
""");
write("C/in.txt", "C");
write(
"C/BUILD",
"""
genrule(
name = "C",
srcs = ["in.txt", "//D:D.txt"],
outs = ["C.txt"],
cmd = "cat $(SRCS) > $@",
)
""");
write("D/in.txt", "D");
write(
"D/BUILD",
"""
genrule(
name = "D",
srcs = ["in.txt"],
outs = ["D.txt"],
cmd = "cat $(SRCS) > $@",
)
""");
write("E/in.txt", "E");
write(
"E/BUILD",
"""
genrule(
name = "E",
srcs = ["in.txt"],
outs = ["E.txt"],
cmd = "cat $(SRCS) > $@",
)
""");
write(
"A/PROJECT.scl",
"""
active_directories = {"default": ["A"]}
""");
}
}

0 comments on commit 06fab2f

Please sign in to comment.