Skip to content

Commit

Permalink
[Skymeld] Skip organizing ALVs into shards before conflict checking.
Browse files Browse the repository at this point in the history 
For regular blaze, conflict checking lies on the critical path. It's therefore
beneficial to organize the ActionLookupValues into shards to minimize the wall
time there.

With Skymeld, the conflict checking process is done per top level target and
does not necessarily lie on the critical path. The cost of forming the Shards
(essentially Lists of Lists) isn't trivial and it adds up when we do it
multiple times per build, as observed in one of the garbage profiles [1].

This CL skips the organization of ALVs into shards before conflict checking for
Skymeld.

PiperOrigin-RevId: 523676154
Change-Id: Iad433a9998ea0de65220904930c9588463fdee7b
  • Loading branch information
@joeleba @copybara-github
joeleba authored and copybara-github committed Apr 12, 2023
1 parent 6de73af commit 20ebd5c
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 75 deletions.
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:exception",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:registry",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_creator",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution",
Expand Down Expand Up @@ -636,6 +635,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
],
)
Expand Down Expand Up @@ -923,7 +923,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:extra_action_artifacts_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//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/profiler",
"//src/main/java/com/google/devtools/build/lib/util",
Expand Down
7 changes: 3 additions & 4 deletions src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Supplier;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.AspectValue;
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
Expand All @@ -44,7 +44,6 @@
import com.google.devtools.build.lib.analysis.constraints.TopLevelConstraintSemantics.PlatformCompatibility;
import com.google.devtools.build.lib.analysis.constraints.TopLevelConstraintSemantics.TargetCompatibilityCheckException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.Sharder;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Target;
Expand Down Expand Up @@ -571,12 +570,12 @@ interface TransitiveActionLookupValuesHelper {

@AutoValue
abstract static class ActionLookupValuesCollectionResult {
abstract Sharder<ActionLookupValue> collectedValues();
abstract ImmutableCollection<SkyValue> collectedValues();

abstract ImmutableSet<SkyKey> visitedKeys();

static ActionLookupValuesCollectionResult create(
Sharder<ActionLookupValue> collectedValues, ImmutableSet<SkyKey> visitedKeys) {
ImmutableCollection<SkyValue> collectedValues, ImmutableSet<SkyKey> visitedKeys) {
return new AutoValue_BuildDriverFunction_ActionLookupValuesCollectionResult(
collectedValues, visitedKeys);
}
Expand Down
65 changes: 30 additions & 35 deletions src/main/java/com/google/devtools/build/lib/skyframe/IncrementalArtifactConflictFinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.NUM_JOBS;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableMap;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
Expand All @@ -30,13 +31,13 @@
import com.google.devtools.build.lib.actions.MutableActionGraph;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.concurrent.ExecutorUtil;
import com.google.devtools.build.lib.concurrent.Sharder;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.ActionConflictsAndStats;
import com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.ConflictException;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
Expand All @@ -55,7 +56,7 @@
@ThreadSafe
public final class IncrementalArtifactConflictFinder {
private final MutableActionGraph threadSafeMutableActionGraph;
public final ConcurrentMap<String, Object> pathFragmentTrieRoot;
private final ConcurrentMap<String, Object> pathFragmentTrieRoot;
private final ListeningExecutorService executorService;

private IncrementalArtifactConflictFinder(MutableActionGraph threadSafeMutableActionGraph) {
Expand All @@ -80,7 +81,7 @@ public int getOutputArtifactCount() {
}

ActionConflictsAndStats findArtifactConflicts(
Sharder<ActionLookupValue> actionLookupValues, boolean strictConflictChecks)
ImmutableCollection<SkyValue> actionLookupValues, boolean strictConflictChecks)
throws InterruptedException {
ConcurrentMap<ActionAnalysisMetadata, ConflictException> temporaryBadActionMap =
new ConcurrentHashMap<>();
Expand All @@ -104,22 +105,17 @@ private static void constructActionGraphAndArtifactList(
ListeningExecutorService executorService,
MutableActionGraph actionGraph,
ConcurrentMap<String, Object> pathFragmentTrieRoot,
Sharder<ActionLookupValue> actionLookupValues,
ImmutableCollection<SkyValue> actionLookupValues,
boolean strictConflictChecks,
ConcurrentMap<ActionAnalysisMetadata, ConflictException> badActionMap)
throws InterruptedException {
// The max number of shards is NUM_JOBS.
List<ListenableFuture<Void>> futures = new ArrayList<>(NUM_JOBS);
for (List<ActionLookupValue> shard : actionLookupValues) {
List<ListenableFuture<Void>> futures = new ArrayList<>(actionLookupValues.size());
for (SkyValue alv : actionLookupValues) {
futures.add(
executorService.submit(
() ->
actionRegistration(
shard,
actionGraph,
pathFragmentTrieRoot,
strictConflictChecks,
badActionMap)));
alv, actionGraph, pathFragmentTrieRoot, strictConflictChecks, badActionMap)));
}
// Now wait on the futures.
try {
Expand All @@ -136,33 +132,32 @@ void shutdown() throws InterruptedException {
}

private static Void actionRegistration(
List<ActionLookupValue> values,
SkyValue value,
MutableActionGraph actionGraph,
ConcurrentMap<String, Object> pathFragmentTrieRoot,
boolean strictConflictChecks,
ConcurrentMap<ActionAnalysisMetadata, ConflictException> badActionMap) {
for (ActionLookupValue value : values) {
for (ActionAnalysisMetadata action : value.getActions()) {
try {
actionGraph.registerAction(action);
} catch (ActionConflictException e) {
// It may be possible that we detect a conflict for the same action more than once, if
// that action belongs to multiple aspect values. In this case we will harmlessly
// overwrite the badActionMap entry.
badActionMap.put(action, new ConflictException(e));
// We skip the rest of the loop, and do not add the path->artifact mapping for this
// artifact below -- we don't need to check it since this action is already in
// error.
continue;
} catch (InterruptedException e) {
// Bail.
Thread.currentThread().interrupt();
return null;
}
for (Artifact output : action.getOutputs()) {
checkOutputPrefix(
actionGraph, strictConflictChecks, pathFragmentTrieRoot, output, badActionMap);
}
Preconditions.checkState(value instanceof ActionLookupValue);
for (ActionAnalysisMetadata action : ((ActionLookupValue) value).getActions()) {
try {
actionGraph.registerAction(action);
} catch (ActionConflictException e) {
// It may be possible that we detect a conflict for the same action more than once, if
// that action belongs to multiple aspect values. In this case we will harmlessly
// overwrite the badActionMap entry.
badActionMap.put(action, new ConflictException(e));
// We skip the rest of the loop, and do not add the path->artifact mapping for this
// artifact below -- we don't need to check it since this action is already in
// error.
continue;
} catch (InterruptedException e) {
// Bail.
Thread.currentThread().interrupt();
return null;
}
for (Artifact output : action.getOutputs()) {
checkOutputPrefix(
actionGraph, strictConflictChecks, pathFragmentTrieRoot, output, badActionMap);
}
}
return null;
Expand Down
18 changes: 6 additions & 12 deletions src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,6 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
Expand Down Expand Up @@ -3368,24 +3367,19 @@ private ActionLookupValuesCollectionResult collectTransitiveActionLookupValuesOf
ActionLookupKey key) {
try (SilentCloseable c =
Profiler.instance().profile("SkyframeExecutor.collectTransitiveActionLookupValuesOfKey")) {
ActionLookupValuesTraversal result = new ActionLookupValuesTraversal();
if (tracksStateForIncrementality()) {
Map<ActionLookupKey, SkyValue> foundTransitiveActionLookupEntities =
ImmutableMap<ActionLookupKey, SkyValue> foundTransitiveActionLookupEntities =
incrementalTransitiveActionLookupKeysCollector.collect(key);
foundTransitiveActionLookupEntities.forEach(result::accumulate);
return ActionLookupValuesCollectionResult.create(
result.getActionLookupValueShards(),
foundTransitiveActionLookupEntities.values(),
ImmutableSet.copyOf(foundTransitiveActionLookupEntities.keySet()));
}
// No graph edges available when there's no incrementality. We get the ALVs collected
// since the last time this method was called.
ImmutableMap<SkyKey, SkyValue> batchOfActionLookupValues =
progressReceiver.getBatchedActionLookupValuesForConflictChecking();
for (Entry<SkyKey, SkyValue> entry : batchOfActionLookupValues.entrySet()) {
result.accumulate((ActionLookupKey) entry.getKey(), entry.getValue());
}
return ActionLookupValuesCollectionResult.create(
result.getActionLookupValueShards(), batchOfActionLookupValues.keySet());
batchOfActionLookupValues.values(), batchOfActionLookupValues.keySet());
}
}

Expand Down Expand Up @@ -3836,13 +3830,13 @@ Map<ActionLookupKey, SkyValue> collect(Iterable<ActionLookupKey> visitationRoots
* <p>IMPORTANT: due to pruning, the set of returned ActionLookupKey is a SUBSET of the set of
* elements in the transitive closure of the visitationRoot.
*/
Map<ActionLookupKey, SkyValue> collect(ActionLookupKey visitationRoot) {
ImmutableMap<ActionLookupKey, SkyValue> collect(ActionLookupKey visitationRoot) {
Map<ActionLookupKey, SkyValue> collected = Maps.newConcurrentMap();
if (seen(visitationRoot, collected)) {
return collected;
return ImmutableMap.copyOf(collected);
}
executorService.invoke(new VisitActionLookupKey(visitationRoot, collected));
return collected;
return ImmutableMap.copyOf(collected);
}

@Override
Expand Down
4 changes: 1 addition & 3 deletions src/test/java/com/google/devtools/build/lib/skyframe/BuildDriverFunctionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.actions.MutableActionGraph;
import com.google.devtools.build.lib.concurrent.Sharder;
import com.google.devtools.build.lib.skyframe.BuildDriverFunction.ActionLookupValuesCollectionResult;
import com.google.devtools.build.lib.skyframe.BuildDriverFunction.TransitiveActionLookupValuesHelper;
import com.google.devtools.build.skyframe.SkyKey;
Expand All @@ -42,9 +41,8 @@ public void checkActionConflicts_noConflict_conflictFreeKeysRegistered() throws
new TransitiveActionLookupValuesHelper() {
@Override
public ActionLookupValuesCollectionResult collect(ActionLookupKey key) {
// Return an empty sharder to have an easy "conflict free" scenario.
return ActionLookupValuesCollectionResult.create(
new Sharder<>(1, 1), ImmutableSet.of(key));
ImmutableSet.of(), ImmutableSet.of(key));
}

@Override
Expand Down
28 changes: 9 additions & 19 deletions ...st/java/com/google/devtools/build/lib/skyframe/IncrementalArtifactConflictFinderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@
import com.google.devtools.build.lib.actions.MutableActionGraph;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.actions.util.TestAction.DummyAction;
import com.google.devtools.build.lib.concurrent.Sharder;
import com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.ActionConflictsAndStats;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
Expand All @@ -59,14 +59,14 @@ public void testFindArtifactConflicts_sequential() throws Exception {
createTestSourceArtifactWithPath(PathFragment.create("in/a")),
createTestDerivedArtifactWithPath(PathFragment.create("out/conflicting")));
ActionLookupValue alv1 = createMockActionLookupValueThatContains(action1);
Sharder<ActionLookupValue> actionLookupValues1 = createALVSharderOf(alv1);
ImmutableList<SkyValue> actionLookupValues1 = ImmutableList.of(alv1);

ActionAnalysisMetadata action2 =
new DummyAction(
createTestSourceArtifactWithPath(PathFragment.create("in/b")),
createTestDerivedArtifactWithPath(PathFragment.create("out/conflicting")));
ActionLookupValue alv2 = createMockActionLookupValueThatContains(action2);
Sharder<ActionLookupValue> actionLookupValues2 = createALVSharderOf(alv2);
ImmutableList<SkyValue> actionLookupValues2 = ImmutableList.of(alv2);

MutableActionGraph actionGraph = new MapBasedActionGraph(new ActionKeyContext());
IncrementalArtifactConflictFinder conflictFinder =
Expand All @@ -91,14 +91,14 @@ public void testFindArtifactConflicts_multiThreaded() throws Exception {
createTestSourceArtifactWithPath(PathFragment.create("in/a")),
createTestDerivedArtifactWithPath(PathFragment.create("out/conflicting")));
ActionLookupValue alv1 = createMockActionLookupValueThatContains(action1);
Sharder<ActionLookupValue> actionLookupValues1 = createALVSharderOf(alv1);
ImmutableList<SkyValue> actionLookupValues1 = ImmutableList.of(alv1);

ActionAnalysisMetadata action2 =
new DummyAction(
createTestSourceArtifactWithPath(PathFragment.create("in/b")),
createTestDerivedArtifactWithPath(PathFragment.create("out/conflicting")));
ActionLookupValue alv2 = createMockActionLookupValueThatContains(action2);
Sharder<ActionLookupValue> actionLookupValues2 = createALVSharderOf(alv2);
ImmutableList<SkyValue> actionLookupValues2 = ImmutableList.of(alv2);

MutableActionGraph actionGraph = new MapBasedActionGraph(new ActionKeyContext());
IncrementalArtifactConflictFinder conflictFinder =
Expand Down Expand Up @@ -147,7 +147,7 @@ public void testFindPrefixArtifactConflicts_singleRun() throws Exception {
createTestDerivedArtifactWithPath(PathFragment.create("out/foo/bar")));
ActionLookupValue alv2 = createMockActionLookupValueThatContains(action2);

Sharder<ActionLookupValue> actionLookupValues = createALVSharderOf(alv1, alv2);
ImmutableList<SkyValue> actionLookupValues = ImmutableList.of(alv1, alv2);
MutableActionGraph actionGraph = new MapBasedActionGraph(new ActionKeyContext());
IncrementalArtifactConflictFinder conflictFinder =
IncrementalArtifactConflictFinder.createWithActionGraph(actionGraph);
Expand Down Expand Up @@ -176,7 +176,7 @@ public void testFindPrefixArtifactConflicts_noStrictChecks_expectNoConflict() th
createTestDerivedArtifactWithPath(PathFragment.create("out/foo/bar")));
ActionLookupValue alv2 = createMockActionLookupValueThatContains(action2);

Sharder<ActionLookupValue> actionLookupValues = createALVSharderOf(alv1, alv2);
ImmutableList<SkyValue> actionLookupValues = ImmutableList.of(alv1, alv2);
MutableActionGraph actionGraph = new MapBasedActionGraph(new ActionKeyContext());
IncrementalArtifactConflictFinder conflictFinder =
IncrementalArtifactConflictFinder.createWithActionGraph(actionGraph);
Expand All @@ -194,14 +194,14 @@ public void testFindPrefixArtifactConflicts_multithreaded() throws Exception {
createTestSourceArtifactWithPath(PathFragment.create("in/a")),
createTestDerivedArtifactWithPath(PathFragment.create("out/foo")));
ActionLookupValue alv1 = createMockActionLookupValueThatContains(action1);
Sharder<ActionLookupValue> actionLookupValues1 = createALVSharderOf(alv1);
ImmutableList<SkyValue> actionLookupValues1 = ImmutableList.of(alv1);

ActionAnalysisMetadata action2 =
new DummyAction(
createTestSourceArtifactWithPath(PathFragment.create("in/b")),
createTestDerivedArtifactWithPath(PathFragment.create("out/foo/bar")));
ActionLookupValue alv2 = createMockActionLookupValueThatContains(action2);
Sharder<ActionLookupValue> actionLookupValues2 = createALVSharderOf(alv2);
ImmutableList<SkyValue> actionLookupValues2 = ImmutableList.of(alv2);

MutableActionGraph actionGraph = new MapBasedActionGraph(new ActionKeyContext());
IncrementalArtifactConflictFinder conflictFinder =
Expand Down Expand Up @@ -230,16 +230,6 @@ public void testFindPrefixArtifactConflicts_multithreaded() throws Exception {
() -> withConflict.getConflicts().get(action1).rethrowTyped());
}

private Sharder<ActionLookupValue> createALVSharderOf(ActionLookupValue... actionLookupValues) {
// These sizes are arbitrary.
Sharder<ActionLookupValue> sharder =
new Sharder<>(/*maxNumShards=*/ 8, /*expectedTotalSize*/ 10);
for (ActionLookupValue actionLookupValue : actionLookupValues) {
sharder.add(actionLookupValue);
}
return sharder;
}

private DerivedArtifact createTestDerivedArtifactWithPath(PathFragment path) {
return DerivedArtifact.create(
ArtifactRoot.asDerivedRoot(scratch.getFileSystem().getPath("/"), RootType.Output, "out"),
Expand Down

0 comments on commit 20ebd5c

Please sign in to comment.