Skip to content

Commit

Permalink
Improve performance of artifact conflict detection.
Browse files Browse the repository at this point in the history 
* Use the action count to presize data structures that grow with the number of outputs.
* Short-circuit output registration when detecting that actions can be shared - there is no need to check for each output, since shareable actions must have identical outputs.
* Instead of using `ConcurrentSkipListMap` to order and deduplicate artifacts, aggregate all artifacts in a list and use `Arrays.parallelSort` followed by a length check to skip calling `startsWith` on equal exec paths.

PiperOrigin-RevId: 379403122
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Jun 15, 2021
1 parent c0f0f8d commit 4801bf5
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 181 deletions.
202 changes: 79 additions & 123 deletions src/main/java/com/google/devtools/build/lib/actions/Actions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterators;
import com.google.common.escape.Escaper;
import com.google.common.escape.Escapers;
import com.google.common.flogger.GoogleLogger;
Expand All @@ -28,14 +29,14 @@
import com.google.devtools.build.lib.skyframe.SkyframeAwareAction;
import com.google.devtools.build.lib.vfs.OsPathPolicy;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Arrays;
import java.util.Collection;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Optional;
import java.util.SortedMap;
import javax.annotation.Nullable;

/** Utility class for actions. */
Expand Down Expand Up @@ -83,36 +84,17 @@ public static boolean dependsOnBuildId(ActionAnalysisMetadata action) {
public static boolean canBeShared(
ActionKeyContext actionKeyContext, ActionAnalysisMetadata a, ActionAnalysisMetadata b)
throws InterruptedException {
if (!a.isShareable() || !b.isShareable()) {
return false;
}

if (!a.getMnemonic().equals(b.getMnemonic())) {
return false;
}

// Non-Actions cannot be shared.
if (!(a instanceof Action) || !(b instanceof Action)) {
return false;
}

Action actionA = (Action) a;
Action actionB = (Action) b;
if (!actionA
.getKey(actionKeyContext, /*artifactExpander=*/ null)
.equals(actionB.getKey(actionKeyContext, /*artifactExpander=*/ null))) {
return false;
}
// Don't bother to check input and output counts first; the expected result for these tests is
// to always be true (i.e., that this method returns true).
if (!artifactsEqualWithoutOwner(
actionA.getMandatoryInputs().toList(), actionB.getMandatoryInputs().toList())) {
return false;
}
if (!artifactsEqualWithoutOwner(actionA.getOutputs(), actionB.getOutputs())) {
return false;
}
return true;
return a.isShareable()
&& b.isShareable()
&& a.getMnemonic().equals(b.getMnemonic())
// Non-Actions cannot be shared.
&& a instanceof Action
&& b instanceof Action
&& a.getKey(actionKeyContext, /*artifactExpander=*/ null)
.equals(b.getKey(actionKeyContext, /*artifactExpander=*/ null))
&& artifactsEqualWithoutOwner(
a.getMandatoryInputs().toList(), b.getMandatoryInputs().toList())
&& artifactsEqualWithoutOwner(a.getOutputs(), b.getOutputs());
}

/**
Expand All @@ -128,45 +110,36 @@ static boolean canBeSharedLogForPotentialFalsePositives(
ActionAnalysisMetadata actionA,
ActionAnalysisMetadata actionB)
throws InterruptedException {
boolean canBeShared = canBeShared(actionKeyContext, actionA, actionB);
if (canBeShared) {
Optional<Artifact> treeArtifactInput =
actionA.getMandatoryInputs().toList().stream()
.filter(Artifact::isTreeArtifact)
.findFirst();
treeArtifactInput.ifPresent(
treeArtifact ->
logger.atInfo().atMostEvery(5, MINUTES).log(
"Shared action: %s has a tree artifact input: %s -- shared actions"
+ " detection is overly permissive in this case and may allow"
+ " sharing of different actions",
actionA, treeArtifact));
if (!canBeShared(actionKeyContext, actionA, actionB)) {
return false;
}
return canBeShared;
Optional<Artifact> treeArtifactInput =
actionA.getMandatoryInputs().toList().stream().filter(Artifact::isTreeArtifact).findFirst();
treeArtifactInput.ifPresent(
treeArtifact ->
logger.atInfo().atMostEvery(5, MINUTES).log(
"Shared action: %s has a tree artifact input: %s -- shared actions"
+ " detection is overly permissive in this case and may allow"
+ " sharing of different actions",
actionA, treeArtifact));
return true;
}

private static boolean artifactsEqualWithoutOwner(
Iterable<Artifact> iterable1, Iterable<Artifact> iterable2) {
if (iterable1 instanceof Collection && iterable2 instanceof Collection) {
Collection<?> collection1 = (Collection<?>) iterable1;
Collection<?> collection2 = (Collection<?>) iterable2;
if (collection1.size() != collection2.size()) {
return false;
}
Collection<Artifact> collection1, Collection<Artifact> collection2) {
if (collection1.size() != collection2.size()) {
return false;
}
Iterator<Artifact> iterator1 = iterable1.iterator();
Iterator<Artifact> iterator2 = iterable2.iterator();
Iterator<Artifact> iterator1 = collection1.iterator();
Iterator<Artifact> iterator2 = collection2.iterator();
while (iterator1.hasNext()) {
if (!iterator2.hasNext()) {
return false;
}
Artifact artifact1 = iterator1.next();
Artifact artifact2 = iterator2.next();
if (!artifact1.equalsWithoutOwner(artifact2)) {
return false;
}
}
return !iterator2.hasNext();
return true;
}

/**
Expand Down Expand Up @@ -341,96 +314,78 @@ private static GeneratingActions assignOwnersAndMaybeFilterSharedActionsAndThrow
artifactsByOutputLabel != null ? artifactsByOutputLabel.build() : ImmutableMap.of());
}

/**
* Returns a comparator for use with {@link #findArtifactPrefixConflicts(ActionGraph, SortedMap,
* boolean)}.
*/
public static Comparator<PathFragment> comparatorForPrefixConflicts() {
return PathFragmentPrefixComparator.INSTANCE;
}

private static class PathFragmentPrefixComparator implements Comparator<PathFragment> {
private static final PathFragmentPrefixComparator INSTANCE = new PathFragmentPrefixComparator();

@Override
public int compare(PathFragment lhs, PathFragment rhs) {
// We need to use the OS path policy in case the OS is case insensitive.
OsPathPolicy os = OsPathPolicy.getFilePathOs();
String str1 = lhs.getPathString();
String str2 = rhs.getPathString();
int len1 = str1.length();
int len2 = str2.length();
int n = Math.min(len1, len2);
for (int i = 0; i < n; ++i) {
char c1 = str1.charAt(i);
char c2 = str2.charAt(i);
int res = os.compare(c1, c2);
if (res != 0) {
if (c1 == PathFragment.SEPARATOR_CHAR) {
return -1;
} else if (c2 == PathFragment.SEPARATOR_CHAR) {
return 1;
private static final Comparator<Artifact> EXEC_PATH_PREFIX_COMPARATOR =
(lhs, rhs) -> {
// We need to use the OS path policy in case the OS is case insensitive.
OsPathPolicy os = OsPathPolicy.getFilePathOs();
String str1 = lhs.getExecPathString();
String str2 = rhs.getExecPathString();
int len1 = str1.length();
int len2 = str2.length();
int n = Math.min(len1, len2);
for (int i = 0; i < n; ++i) {
char c1 = str1.charAt(i);
char c2 = str2.charAt(i);
int res = os.compare(c1, c2);
if (res != 0) {
if (c1 == PathFragment.SEPARATOR_CHAR) {
return -1;
} else if (c2 == PathFragment.SEPARATOR_CHAR) {
return 1;
}
return res;
}
return res;
}
}
return len1 - len2;
}
}
return len1 - len2;
};

/**
* Finds Artifact prefix conflicts between generated artifacts. An artifact prefix conflict
* happens if one action generates an artifact whose path is a prefix of another artifact's path.
* Those two artifacts cannot exist simultaneously in the output tree.
* happens if one action generates an artifact whose path is a strict prefix of another artifact's
* path. Those two artifacts cannot exist simultaneously in the output tree.
*
* @param actionGraph the {@link ActionGraph} to query for artifact conflicts
* @param artifactPathMap a map mapping generated artifacts to their exec paths. The map must be
* sorted using the comparator from {@link #comparatorForPrefixConflicts()}.
* @param strictConflictChecks report path prefix conflicts, regardless of
* shouldReportPathPrefixConflict().
* @param artifacts all generated artifacts in the build
* @param strictConflictChecks report path prefix conflicts, regardless of {@link
* ActionAnalysisMetadata#shouldReportPathPrefixConflict}
* @return An immutable map between actions that generated the conflicting artifacts and their
* associated {@link ArtifactPrefixConflictException}.
* associated {@link ArtifactPrefixConflictException}
*/
public static ImmutableMap<ActionAnalysisMetadata, ArtifactPrefixConflictException>
findArtifactPrefixConflicts(
ActionGraph actionGraph,
SortedMap<PathFragment, Artifact> artifactPathMap,
boolean strictConflictChecks) {
// You must construct the sorted map using this comparator for the algorithm to work.
// The algorithm requires subdirectories to immediately follow parent directories,
// before any files in that directory.
// Example: "foo", "foo.obj", foo/bar" must be sorted
// "foo", "foo/bar", foo.obj"
Preconditions.checkArgument(
artifactPathMap.comparator() instanceof PathFragmentPrefixComparator,
"artifactPathMap must be sorted with PathFragmentPrefixComparator");
ActionGraph actionGraph, Collection<Artifact> artifacts, boolean strictConflictChecks) {
// No actions in graph -- currently happens only in tests. Special-cased because .next() call
// below is unconditional.
if (artifactPathMap.isEmpty()) {
return ImmutableMap.<ActionAnalysisMetadata, ArtifactPrefixConflictException>of();
if (artifacts.isEmpty()) {
return ImmutableMap.of();
}

Artifact[] artifactArray = artifacts.toArray(new Artifact[0]);
Arrays.parallelSort(artifactArray, EXEC_PATH_PREFIX_COMPARATOR);

// Keep deterministic ordering of bad actions.
Map<ActionAnalysisMetadata, ArtifactPrefixConflictException> badActions = new LinkedHashMap<>();
Iterator<PathFragment> iter = artifactPathMap.keySet().iterator();
Iterator<Artifact> iter = Iterators.forArray(artifactArray);

// Report an error for every derived artifact which is a prefix of another.
// Report an error for every derived artifact which is a strict prefix of another.
// If x << y << z (where x << y means "y starts with x"), then we only report (x,y), (x,z), but
// not (y,z).
for (PathFragment pathJ = iter.next(); iter.hasNext(); ) {
for (Artifact artifactJ = iter.next(); iter.hasNext(); ) {
// For each comparison, we have a prefix candidate (pathI) and a suffix candidate (pathJ).
// At the beginning of the loop, we set pathI to the last suffix candidate, since it has not
// yet been tested as a prefix candidate, and then set pathJ to the paths coming after pathI,
// until we come to one that does not contain pathI as a prefix. pathI is then verified not to
// be the prefix of any path, so we start the next run of the loop.
PathFragment pathI = pathJ;
Artifact artifactI = artifactJ;
PathFragment pathI = artifactI.getExecPath();
// Compare pathI to the paths coming after it.
while (iter.hasNext()) {
pathJ = iter.next();
if (pathJ.startsWith(pathI)) { // prefix conflict.
Artifact artifactI = Preconditions.checkNotNull(artifactPathMap.get(pathI), pathI);
Artifact artifactJ = Preconditions.checkNotNull(artifactPathMap.get(pathJ), pathJ);

artifactJ = iter.next();
PathFragment pathJ = artifactJ.getExecPath();
// Check length first so that we only detect strict prefix conflicts. Equal exec paths are
// possible from shared actions.
if (pathJ.getPathString().length() > pathI.getPathString().length()
&& pathJ.startsWith(pathI)) {
// TODO(b/159733792): Test this check with compressed tree artifact input.
// We ignore the artifact prefix conflict between a TreeFileArtifact and its parent
// TreeArtifact.
Expand All @@ -449,8 +404,9 @@ public int compare(PathFragment lhs, PathFragment rhs) {
ActionAnalysisMetadata actionJ =
Preconditions.checkNotNull(actionGraph.getGeneratingAction(artifactJ), artifactJ);
if (strictConflictChecks || actionI.shouldReportPathPrefixConflict(actionJ)) {
ArtifactPrefixConflictException exception = new ArtifactPrefixConflictException(pathI,
pathJ, actionI.getOwner().getLabel(), actionJ.getOwner().getLabel());
ArtifactPrefixConflictException exception =
new ArtifactPrefixConflictException(
pathI, pathJ, actionI.getOwner().getLabel(), actionJ.getOwner().getLabel());
badActions.put(actionI, exception);
badActions.put(actionJ, exception);
}
Expand Down
22 changes: 13 additions & 9 deletions src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,15 @@
public final class MapBasedActionGraph implements MutableActionGraph {

private final ActionKeyContext actionKeyContext;
private final ConcurrentMap<OwnerlessArtifactWrapper, ActionAnalysisMetadata>
generatingActionMap = new ConcurrentHashMap<>();
private final ConcurrentMap<OwnerlessArtifactWrapper, ActionAnalysisMetadata> generatingActionMap;

public MapBasedActionGraph(ActionKeyContext actionKeyContext) {
this(actionKeyContext, /*sizeHint=*/ 16);
}

public MapBasedActionGraph(ActionKeyContext actionKeyContext, int sizeHint) {
this.actionKeyContext = actionKeyContext;
this.generatingActionMap = new ConcurrentHashMap<>(sizeHint);
}

@Override
Expand All @@ -42,13 +46,13 @@ public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) {
public void registerAction(ActionAnalysisMetadata action)
throws ActionConflictException, InterruptedException {
for (Artifact artifact : action.getOutputs()) {
OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact);
ActionAnalysisMetadata previousAction = generatingActionMap.putIfAbsent(wrapper, action);
if (previousAction != null
&& previousAction != action
&& !Actions.canBeSharedLogForPotentialFalsePositives(
actionKeyContext, action, previousAction)) {
generatingActionMap.remove(wrapper, action);
ActionAnalysisMetadata previousAction =
generatingActionMap.putIfAbsent(new OwnerlessArtifactWrapper(artifact), action);
if (previousAction != null && previousAction != action) {
if (Actions.canBeSharedLogForPotentialFalsePositives(
actionKeyContext, action, previousAction)) {
return; // All outputs can be shared. No need to register the remaining outputs.
}
throw new ActionConflictException(actionKeyContext, artifact, previousAction, action);
}
}
Expand Down
10 changes: 1 addition & 9 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue.ActionTemplateExpansionKey;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
Expand All @@ -43,7 +42,6 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -198,15 +196,9 @@ private static ImmutableMap<Artifact, ActionAnalysisMetadata> getMapForConsisten
*/
private static Map<ActionAnalysisMetadata, ArtifactPrefixConflictException>
findArtifactPrefixConflicts(Map<Artifact, ActionAnalysisMetadata> generatingActions) {
TreeMap<PathFragment, Artifact> artifactPathMap =
new TreeMap<>(Actions.comparatorForPrefixConflicts());
for (Artifact artifact : generatingActions.keySet()) {
artifactPathMap.put(artifact.getExecPath(), artifact);
}

return Actions.findArtifactPrefixConflicts(
new MapBasedImmutableActionGraph(generatingActions),
artifactPathMap,
generatingActions.keySet(),
/*strictConflictChecks=*/ true);
}

Expand Down
Loading

0 comments on commit 4801bf5

Please sign in to comment.