Automated rollback of commit 3290e22356b59371274849ee51297635b9435285.
*** Reason for rollback ***
Rolling forward with fix: keep passing artifact owner into generated artifact, sacrificing a bit of type safety for actual safety. This effectively reverts a few files from the original CL.
Also fix memory regression: key map of output file artifacts by label, since we can reuse existing labels. Moreover, this allows us to have an empty map for many configured targets: a build of a large internal target had the following numbers of targets per # of output files (apologies for the only partially sorted map):
{0=63649, 1=67148, 2=89175, 3=49347, 4=12712, 5=3027, 261=1, 6=247, 7=9, 775=1, 8=10, 9=82, 10=6, 11=3, 12=54, 13=4, 14=2, 15=24, 655=1, 271=2, 144=1, 17=2, 18=24, 276=1, 21=9, 3990=2, 24=18, 27=11, 156=3, 30=6, 31=2, 33=12, 34=1, 163=1, 36=5, 37=1, 38=2, 294=1, 39=5, 41=1, 42=3, 43=2, 45=9, 302=1, 47=1, 48=4, 561=1, 306=2, 54=2, 183=1, 57=1, 573=2, 318=2, 63=2, 65=1, 66=1, 198=1, 71=1, 72=2, 330=2, 75=2, 204=1, 76=1, 77=2, 80=1, 82=1, 84=3, 87=2, 346=2, 90=4, 480=1, 99=5, 102=1, 231=2, 363=1, 624=2, 117=1, 120=2, 123=2, 252=2}
So those are the sizes of the maps that are actually needed. With the regression fixed, this change actually has a significant progression because we no longer create a duplicate derived artifact (with PathFragments, etc.) per OutputFileConfiguredTarget. Moreover, we don't need to keep a map with every artifact as keys inside ActionLookupValue and RuleConfiguredTarget. The only regression remaining is that we eagerly create ActionLookupData objects for every artifact, versus only on demand during execution. That's not a major issue, and it's ameliorated by no longer needing to intern ActionLookupData objects, which has memory and CPU benefits. Total memory diff for analysis of our large internal target looks very nice (net -125M out of 5.88G, or 2% gain):
objsize chg instances space KB class name
------------------------------------------------------
24 +0 1345958 31545 KB com.google.devtools.build.lib.actions.ActionLookupData
70 +0 -334 1992 KB [Ljava.lang.Object;
16 +0 -19073 -298 KB java.lang.Integer
40 +0 -12795 -499 KB com.google.common.collect.SingletonImmutableBiMap
40 +0 -45433 -1774 KB com.google.common.collect.MapMakerInternalMap$WeakKeyDummyValueEntry
32 -8 0 -1852 KB com.google.devtools.build.lib.analysis.ConfiguredAspect
24 +0 -79943 -1873 KB com.google.common.collect.SingletonImmutableSet
24 +0 -79943 -1873 KB com.google.common.collect.ImmutableEntry
40 +0 -61756 -2412 KB com.google.common.collect.RegularImmutableMap
16 +0 -216644 -3385 KB com.google.common.collect.RegularImmutableList
24 +0 -216650 -5077 KB com.google.common.collect.ImmutableMapEntrySet$RegularEntrySet
44 -4 -61755 -7376 KB [Ljava.util.Map$Entry;
44 -3 -61757 -7409 KB [Lcom.google.common.collect.ImmutableMapEntry;
32 +0 -259112 -8097 KB com.google.devtools.build.lib.actions.Artifact$DerivedArtifact
24 +0 -522911 -12255 KB com.google.devtools.build.lib.vfs.PathFragment
24 +0 -525254 -12310 KB java.lang.String
24 +0 -541285 -12686 KB com.google.common.collect.ImmutableMapEntry$NonTerminalImmutableMapEntry
24 +0 -999865 -23434 KB com.google.common.collect.ImmutableMapEntry
85 +0 -524326 -56276 KB [B
------------------------------------------------------
total change: -125514 KB
PiperOrigin-RevId: 251755552
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index 3be7aad..f589d15 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.base.Objects;
import com.google.common.base.Preconditions;
@@ -22,8 +21,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.actions.ActionInputHelper;
-import com.google.devtools.build.lib.actions.ActionLookupData;
-import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
@@ -37,7 +34,6 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.util.BigIntegerFingerprint;
import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.math.BigInteger;
import java.util.Comparator;
@@ -236,18 +232,6 @@
.sorted(Comparator.comparing(Entry::getKey, Artifact.EXEC_PATH_COMPARATOR));
}
- /**
- * @param lookupKey A {@link SkyKey} whose argument is an {@code ActionLookupKey}, whose
- * corresponding {@code ActionLookupValue} contains the action to be executed.
- * @param index the index of the action to be executed in the {@code ActionLookupValue}, to be
- * passed to {@code ActionLookupValue#getAction}.
- */
- @ThreadSafe
- @VisibleForTesting
- public static ActionLookupData key(ActionLookupValue.ActionLookupKey lookupKey, int index) {
- return ActionLookupData.create(lookupKey, index);
- }
-
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
index 26ca7e2..d5771fc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
@@ -19,6 +19,7 @@
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionInputMapSink;
+import com.google.devtools.build.lib.actions.ActionLookupData;
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
import com.google.devtools.build.lib.actions.Artifact;
@@ -27,7 +28,6 @@
import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Collection;
import java.util.Map;
@@ -93,27 +93,44 @@
static ImmutableList<FilesetOutputSymlink> getFilesets(
Environment env, Artifact.SpecialArtifact actionInput) throws InterruptedException {
Preconditions.checkState(actionInput.isFileset(), actionInput);
- ActionLookupKey filesetActionLookupKey = (ActionLookupKey) actionInput.getArtifactOwner();
+ ActionLookupData generatingActionKey = actionInput.getGeneratingActionKey();
+ ActionLookupKey filesetActionLookupKey = generatingActionKey.getActionLookupKey();
ActionLookupValue filesetActionLookupValue =
(ActionLookupValue) env.getValue(filesetActionLookupKey);
ActionAnalysisMetadata generatingAction =
- filesetActionLookupValue.getGeneratingActionDangerousReadJavadoc(actionInput);
- int filesetManifestActionIndex;
+ filesetActionLookupValue.getAction(generatingActionKey.getActionIndex());
+ ActionLookupData filesetActionKey;
if (generatingAction instanceof SymlinkAction) {
- Artifact outputManifest = Iterables.getOnlyElement(generatingAction.getInputs());
+ Artifact.DerivedArtifact outputManifest =
+ (Artifact.DerivedArtifact) Iterables.getOnlyElement(generatingAction.getInputs());
+ ActionLookupData manifestGeneratingKey = outputManifest.getGeneratingActionKey();
+ Preconditions.checkState(
+ manifestGeneratingKey.getActionLookupKey().equals(filesetActionLookupKey),
+ "Mismatched actions and artifacts: %s %s %s %s",
+ actionInput,
+ outputManifest,
+ filesetActionLookupKey,
+ manifestGeneratingKey);
ActionAnalysisMetadata symlinkTreeAction =
- filesetActionLookupValue.getGeneratingActionDangerousReadJavadoc(outputManifest);
- Artifact inputManifest = Iterables.getOnlyElement(symlinkTreeAction.getInputs());
- filesetManifestActionIndex = filesetActionLookupValue.getGeneratingActionIndex(inputManifest);
+ filesetActionLookupValue.getAction(manifestGeneratingKey.getActionIndex());
+ Artifact.DerivedArtifact inputManifest =
+ (Artifact.DerivedArtifact) Iterables.getOnlyElement(symlinkTreeAction.getInputs());
+ ActionLookupData inputManifestGeneratingKey = inputManifest.getGeneratingActionKey();
+ Preconditions.checkState(
+ inputManifestGeneratingKey.getActionLookupKey().equals(filesetActionLookupKey),
+ "Mismatched actions and artifacts: %s %s %s %s",
+ actionInput,
+ inputManifest,
+ filesetActionLookupKey,
+ inputManifestGeneratingKey);
+ filesetActionKey = inputManifestGeneratingKey;
} else {
- filesetManifestActionIndex = filesetActionLookupValue.getGeneratingActionIndex(actionInput);
+ filesetActionKey = generatingActionKey;
}
- SkyKey filesetActionKey =
- ActionExecutionValue.key(filesetActionLookupKey, filesetManifestActionIndex);
ActionExecutionValue filesetValue = (ActionExecutionValue) env.getValue(filesetActionKey);
if (filesetValue == null) {
// At this point skyframe does not guarantee that the filesetValue will be ready, since
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
index d8b5b5f..41bed76 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
@@ -14,14 +14,18 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
+import com.google.devtools.build.lib.actions.ActionGraph;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.ActionTemplate;
import com.google.devtools.build.lib.actions.ActionTemplate.ActionTemplateExpansionException;
import com.google.devtools.build.lib.actions.Actions;
import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
+import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException;
import com.google.devtools.build.lib.actions.ArtifactSkyKey;
@@ -29,11 +33,15 @@
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;
import com.google.devtools.build.skyframe.SkyValue;
+import java.util.HashMap;
+import java.util.List;
import java.util.Map;
+import java.util.TreeMap;
import javax.annotation.Nullable;
/**
@@ -80,7 +88,7 @@
// of the ActionTemplate.
generatingActions =
checkActionAndArtifactConflicts(
- actionTemplate.generateActionForInputArtifacts(inputTreeFileArtifacts, key));
+ actionTemplate.generateActionForInputArtifacts(inputTreeFileArtifacts, key), key);
} catch (ActionConflictException e) {
e.reportTo(env.getListener());
throw new ActionTemplateExpansionFunctionException(e);
@@ -110,14 +118,14 @@
}
}
- private GeneratingActions checkActionAndArtifactConflicts(Iterable<? extends Action> actions)
+ private GeneratingActions checkActionAndArtifactConflicts(
+ Iterable<? extends Action> actions, ActionLookupValue.ActionLookupKey actionLookupKey)
throws ActionConflictException, ArtifactPrefixConflictException {
GeneratingActions generatingActions =
- Actions.findAndThrowActionConflict(actionKeyContext, ImmutableList.copyOf(actions));
+ Actions.assignOwnersAndFindAndThrowActionConflict(
+ actionKeyContext, ImmutableList.copyOf(actions), actionLookupKey);
Map<ActionAnalysisMetadata, ArtifactPrefixConflictException> artifactPrefixConflictMap =
- Actions.findArtifactPrefixConflicts(
- ActionLookupValue.getMapForConsistencyCheck(
- generatingActions.getGeneratingActionIndex(), generatingActions.getActions()));
+ findArtifactPrefixConflicts(getMapForConsistencyCheck(generatingActions.getActions()));
if (!artifactPrefixConflictMap.isEmpty()) {
throw artifactPrefixConflictMap.values().iterator().next();
@@ -125,6 +133,57 @@
return generatingActions;
}
+ private static Map<Artifact, ActionAnalysisMetadata> getMapForConsistencyCheck(
+ List<? extends ActionAnalysisMetadata> actions) {
+ if (actions.isEmpty()) {
+ return ImmutableMap.of();
+ }
+ HashMap<Artifact, ActionAnalysisMetadata> result =
+ Maps.newHashMapWithExpectedSize(actions.size() * actions.get(0).getOutputs().size());
+ for (ActionAnalysisMetadata action : actions) {
+ for (Artifact output : action.getOutputs()) {
+ result.put(output, action);
+ }
+ }
+ return result;
+ }
+
+ /**
+ * 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.
+ *
+ * @param generatingActions a map between generated artifacts and their associated generating
+ * actions.
+ * @return a map between actions that generated the conflicting artifacts and their associated
+ * {@link ArtifactPrefixConflictException}.
+ */
+ 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);
+ }
+
+ private static class MapBasedImmutableActionGraph implements ActionGraph {
+ private final Map<Artifact, ActionAnalysisMetadata> generatingActions;
+
+ MapBasedImmutableActionGraph(Map<Artifact, ActionAnalysisMetadata> generatingActions) {
+ this.generatingActions = ImmutableMap.copyOf(generatingActions);
+ }
+
+ @Nullable
+ @Override
+ public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) {
+ return generatingActions.get(artifact);
+ }
+ }
+
@Nullable
@Override
public String extractTag(SkyKey skyKey) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index bfec907..b2b4d96 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -27,6 +27,7 @@
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.ArtifactOwner;
@@ -428,21 +429,12 @@
// TODO(b/19539699): extend this to comprehensively support all special artifact types (e.g.
// middleman, etc).
static class ArtifactDependencies {
-
- private final Artifact artifact;
- private final ActionLookupKey actionLookupKey;
+ private final DerivedArtifact artifact;
private final ActionLookupValue actionLookupValue;
- private final int actionIndex;
- private ArtifactDependencies(
- Artifact artifact,
- ActionLookupKey actionLookupKey,
- ActionLookupValue actionLookupValue,
- int actionIndex) {
+ private ArtifactDependencies(DerivedArtifact artifact, ActionLookupValue actionLookupValue) {
this.artifact = artifact;
- this.actionLookupKey = actionLookupKey;
this.actionLookupValue = actionLookupValue;
- this.actionIndex = actionIndex;
}
/**
@@ -454,25 +446,15 @@
Artifact.DerivedArtifact derivedArtifact, SkyFunction.Environment env)
throws InterruptedException {
- ActionLookupKey actionLookupKey = ArtifactFunction.getActionLookupKey(derivedArtifact);
+ ActionLookupData generatingActionKey = derivedArtifact.getGeneratingActionKey();
ActionLookupValue actionLookupValue =
- ArtifactFunction.getActionLookupValue(actionLookupKey, env, derivedArtifact);
+ ArtifactFunction.getActionLookupValue(
+ generatingActionKey.getActionLookupKey(), env, derivedArtifact);
if (actionLookupValue == null) {
return null;
}
- Integer actionIndex = actionLookupValue.getGeneratingActionIndex(derivedArtifact);
- if (derivedArtifact.hasParent() && actionIndex == null) {
- // If a TreeFileArtifact is created by a templated action, then it should have the proper
- // reference to its owner. However, if it was created as part of a directory, by the first
- // TreeArtifact-generating action in a chain, then its parent's generating action also
- // generated it. This catches that case.
- actionIndex = actionLookupValue.getGeneratingActionIndex(derivedArtifact.getParent());
- }
- Preconditions.checkNotNull(
- actionIndex, "%s %s %s", derivedArtifact, actionLookupKey, actionLookupValue);
- return new ArtifactDependencies(
- derivedArtifact, actionLookupKey, actionLookupValue, actionIndex);
+ return new ArtifactDependencies(derivedArtifact, actionLookupValue);
}
Artifact getArtifact() {
@@ -480,21 +462,21 @@
}
ActionLookupKey getActionLookupKey() {
- return actionLookupKey;
+ return artifact.getArtifactOwner();
}
int getActionIndex() {
- return actionIndex;
+ return artifact.getGeneratingActionKey().getActionIndex();
}
boolean isTemplateActionForTreeArtifact() {
- return artifact.isTreeArtifact() && actionLookupValue.isActionTemplate(actionIndex);
+ return artifact.isTreeArtifact() && actionLookupValue.isActionTemplate(getActionIndex());
}
ActionLookupData getNontemplateActionExecutionKey() {
Preconditions.checkState(
!isTemplateActionForTreeArtifact(), "Action is unexpectedly template: %s", this);
- return ActionExecutionValue.key(actionLookupKey, actionIndex);
+ return artifact.getGeneratingActionKey();
}
/**
@@ -509,7 +491,7 @@
Preconditions.checkState(
isTemplateActionForTreeArtifact(), "Action is unexpectedly non-template: %s", this);
ActionTemplateExpansionValue.ActionTemplateExpansionKey key =
- ActionTemplateExpansionValue.key(actionLookupKey, actionIndex);
+ ActionTemplateExpansionValue.key(getActionLookupKey(), getActionIndex());
ActionTemplateExpansionValue value = (ActionTemplateExpansionValue) env.getValue(key);
if (value == null) {
return null;
@@ -518,16 +500,15 @@
}
Action getAction() {
- return actionLookupValue.getAction(actionIndex);
+ return actionLookupValue.getAction(getActionIndex());
}
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("artifact", artifact)
- .add("actionLookupKey", actionLookupKey)
+ .add("generatingActionKey", artifact.getGeneratingActionKey())
.add("actionLookupValue", actionLookupValue)
- .add("actionIndex", actionIndex)
.toString();
}
}
@@ -555,8 +536,9 @@
int numActions = value.getNumActions();
ImmutableList.Builder<ActionLookupData> expandedActionExecutionKeys =
ImmutableList.builderWithExpectedSize(numActions);
- for (int i = 0; i < numActions; i++) {
- expandedActionExecutionKeys.add(ActionExecutionValue.key(key, i));
+ for (ActionAnalysisMetadata action : value.getActions()) {
+ expandedActionExecutionKeys.add(
+ ((DerivedArtifact) action.getPrimaryOutput()).getGeneratingActionKey());
}
return expandedActionExecutionKeys.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index b46fd13..50843d2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -444,7 +444,7 @@
ConfiguredAspect configuredAspect,
NestedSet<Package> transitivePackagesForPackageRootResolution,
BigInteger nonceVersion) {
- super(configuredAspect.getActions(), configuredAspect.getGeneratingActionIndex(), nonceVersion);
+ super(configuredAspect.getActions(), nonceVersion);
this.label = Preconditions.checkNotNull(label, actions);
this.aspect = Preconditions.checkNotNull(aspect, label);
this.location = Preconditions.checkNotNull(location, label);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java
index 641e01b..3425c32 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java
@@ -98,8 +98,8 @@
GeneratingActions generatingActions;
try {
generatingActions =
- Actions.filterSharedActionsAndThrowActionConflict(
- actionKeyContext, collection.getActions());
+ Actions.assignOwnersAndFilterSharedActionsAndThrowActionConflict(
+ actionKeyContext, collection.getActions(), keyAndConfig, /*outputFiles=*/ null);
} catch (ActionConflictException e) {
throw new IllegalStateException("Action conflicts not expected in build info: " + skyKey, e);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index a26a891..94231f6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -942,9 +942,11 @@
// rule implementation).
try {
generatingActions =
- Actions.filterSharedActionsAndThrowActionConflict(
+ Actions.assignOwnersAndFilterSharedActionsAndThrowActionConflict(
analysisEnvironment.getActionKeyContext(),
- analysisEnvironment.getRegisteredActions());
+ analysisEnvironment.getRegisteredActions(),
+ configuredTargetKey,
+ /*outputFiles=*/ null);
} catch (ActionConflictException e) {
throw new ConfiguredTargetFunctionException(e);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java
index c83069e..9a5ca17 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java
@@ -16,12 +16,10 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.Actions;
import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
-import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
@@ -50,16 +48,14 @@
return null;
}
- ImmutableSet.Builder<Artifact> outputs = new ImmutableSet.Builder<>();
-
- for (ActionAnalysisMetadata action : actions) {
- outputs.addAll(action.getOutputs());
- }
-
GeneratingActions generatingActions;
try {
generatingActions =
- Actions.filterSharedActionsAndThrowActionConflict(actionKeyContext, actions);
+ Actions.assignOwnersAndFilterSharedActionsAndThrowActionConflict(
+ actionKeyContext,
+ actions,
+ CoverageReportValue.COVERAGE_REPORT_KEY,
+ /*outputFiles=*/ null);
} catch (ActionConflictException e) {
throw new IllegalStateException("Action conflicts not expected in coverage: " + skyKey, e);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java
index 9b70e17..46151e5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java
@@ -16,10 +16,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
-import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
import com.google.devtools.build.lib.actions.BasicActionLookupValue;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -53,9 +51,8 @@
@VisibleForSerialization
NonRuleConfiguredTargetValue(
ImmutableList<ActionAnalysisMetadata> actions,
- ImmutableMap<Artifact, Integer> generatingActionIndex,
ConfiguredTarget configuredTarget) {
- super(actions, generatingActionIndex, /*nonceVersion=*/ null);
+ super(actions, /*nonceVersion=*/ null);
this.configuredTarget = configuredTarget;
// Transitive packages are not serialized.
this.transitivePackagesForPackageRootResolution = null;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java
index 4c8a431..1542b9e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java
@@ -16,10 +16,8 @@
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionLookupValue;
-import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -41,7 +39,6 @@
// clear(true) is called.
@Nullable private RuleConfiguredTarget configuredTarget;
private final ImmutableList<ActionAnalysisMetadata> actions;
- private final ImmutableMap<Artifact, Integer> generatingActionIndex;
// May be null either after clearing or because transitive packages are not tracked.
@Nullable private NestedSet<Package> transitivePackagesForPackageRootResolution;
@@ -64,7 +61,6 @@
this.transitivePackagesForPackageRootResolution = transitivePackagesForPackageRootResolution;
// These are specifically *not* copied to save memory.
this.actions = configuredTarget.getActions();
- this.generatingActionIndex = configuredTarget.getGeneratingActionIndex();
}
@Override
@@ -79,11 +75,6 @@
}
@Override
- protected ImmutableMap<Artifact, Integer> getGeneratingActionIndex() {
- return generatingActionIndex;
- }
-
- @Override
public NestedSet<Package> getTransitivePackagesForPackageRootResolution() {
return Preconditions.checkNotNull(transitivePackagesForPackageRootResolution);
}
@@ -100,7 +91,6 @@
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
- .add("generatingActionIndex", generatingActionIndex)
.add("actions", actions)
.add("configuredTarget", configuredTarget)
.toString();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index e3cc635..f24127a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -356,12 +356,9 @@
ExecutorService executor = Executors.newFixedThreadPool(
numJobs,
new ThreadFactoryBuilder().setNameFormat("ActionLookupValue Processor %d").build());
- Set<ActionAnalysisMetadata> registeredActions = Sets.newConcurrentHashSet();
for (List<ActionLookupValue> shard : actionShards) {
executor.execute(
- wrapper.wrap(
- actionRegistration(
- shard, actionGraph, artifactPathMap, badActionMap, registeredActions)));
+ wrapper.wrap(actionRegistration(shard, actionGraph, artifactPathMap, badActionMap)));
}
boolean interrupted = ExecutorUtil.interruptibleShutdown(executor);
Throwables.propagateIfPossible(wrapper.getFirstThrownError());
@@ -375,31 +372,22 @@
final List<ActionLookupValue> values,
final MutableActionGraph actionGraph,
final ConcurrentMap<PathFragment, Artifact> artifactPathMap,
- final ConcurrentMap<ActionAnalysisMetadata, ConflictException> badActionMap,
- final Set<ActionAnalysisMetadata> registeredActions) {
- return new Runnable() {
- @Override
- public void run() {
- for (ActionLookupValue value : values) {
- for (Map.Entry<Artifact, ActionAnalysisMetadata> entry :
- value.getMapForConsistencyCheck().entrySet()) {
- ActionAnalysisMetadata action = entry.getValue();
- // We have an entry for each <action, artifact> pair. Only try to register each action
- // once.
- if (registeredActions.add(action)) {
- try {
- actionGraph.registerAction(action);
- } catch (ActionConflictException e) {
- Exception oldException = badActionMap.put(action, new ConflictException(e));
- Preconditions.checkState(
- oldException == null, "%s | %s | %s", action, e, oldException);
- // 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;
- }
- }
- artifactPathMap.put(entry.getKey().getExecPath(), entry.getKey());
+ final ConcurrentMap<ActionAnalysisMetadata, ConflictException> badActionMap) {
+ return () -> {
+ for (ActionLookupValue value : values) {
+ for (ActionAnalysisMetadata action : value.getActions()) {
+ try {
+ actionGraph.registerAction(action);
+ } catch (ActionConflictException e) {
+ Exception oldException = badActionMap.put(action, new ConflictException(e));
+ Preconditions.checkState(oldException == null, "%s | %s | %s", action, e, oldException);
+ // 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;
+ }
+ for (Artifact output : action.getOutputs()) {
+ artifactPathMap.put(output.getExecPath(), output);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index a8448c1..39e483e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -44,11 +44,12 @@
import com.google.devtools.build.lib.actions.ActionInputPrefetcher;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator;
+import com.google.devtools.build.lib.actions.ActionLookupData;
import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.Actions;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
import com.google.devtools.build.lib.actions.ArtifactFactory;
-import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
import com.google.devtools.build.lib.actions.ArtifactRoot;
@@ -1113,8 +1114,8 @@
: (WorkspaceStatusAction) workspaceStatusValue.getAction(0);
}
- public void injectCoverageReportData(ImmutableList<ActionAnalysisMetadata> actions) {
- PrecomputedValue.COVERAGE_REPORT_KEY.set(injectable(), actions);
+ public void injectCoverageReportData(Actions.GeneratingActions actions) {
+ PrecomputedValue.COVERAGE_REPORT_KEY.set(injectable(), actions.getActions());
}
private void setDefaultVisibility(RuleVisibility defaultVisibility) {
@@ -2457,13 +2458,8 @@
return null;
}
- ArtifactOwner artifactOwner = artifact.getArtifactOwner();
- Preconditions.checkState(
- artifactOwner instanceof ActionLookupValue.ActionLookupKey,
- "%s %s",
- artifact,
- artifactOwner);
- SkyKey actionLookupKey = (ActionLookupValue.ActionLookupKey) artifactOwner;
+ ActionLookupData generatingActionKey =
+ ((Artifact.DerivedArtifact) artifact).getGeneratingActionKey();
synchronized (valueLookupLock) {
// Note that this will crash (attempting to run a configured target value builder after
@@ -2472,13 +2468,18 @@
// this action. We don't expect callers to query generating actions in such cases.
EvaluationResult<ActionLookupValue> result =
evaluate(
- ImmutableList.of(actionLookupKey),
+ ImmutableList.of(generatingActionKey.getActionLookupKey()),
/*keepGoing=*/ false,
/*numThreads=*/ ResourceUsage.getAvailableProcessors(),
eventHandler);
- return result.hasError()
- ? null
- : result.get(actionLookupKey).getGeneratingActionDangerousReadJavadoc(artifact);
+ if (result.hasError()) {
+ return null;
+ }
+ ActionLookupValue actionLookupValue = result.get(generatingActionKey.getActionLookupKey());
+ int actionIndex = generatingActionKey.getActionIndex();
+ return actionLookupValue.isActionTemplate(actionIndex)
+ ? actionLookupValue.getActionTemplate(actionIndex)
+ : actionLookupValue.getAction(actionIndex);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
index 112fd7e..0524167 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
@@ -13,21 +13,15 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Multimap;
-import com.google.common.collect.Multimaps;
-import com.google.devtools.build.lib.actions.ActionLookupData;
-import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.test.TestProvider;
-import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-import java.util.Map;
/**
* TestCompletionFunction builds all relevant test artifacts of a {@link
@@ -53,49 +47,17 @@
ConfiguredTarget ct = ctValue.getConfiguredTarget();
if (key.exclusiveTesting()) {
// Request test execution iteratively if testing exclusively.
- for (Artifact testArtifact : TestProvider.getTestStatusArtifacts(ct)) {
- ActionLookupValue.ActionLookupKey actionLookupKey =
- ArtifactFunction.getActionLookupKey(testArtifact);
- ActionLookupValue actionLookupValue =
- ArtifactFunction.getActionLookupValue(actionLookupKey, env, testArtifact);
- if (actionLookupValue == null) {
- BugReport.sendBugReport(
- new IllegalStateException(
- "Unexpected absent value for "
- + actionLookupKey
- + " from "
- + testArtifact
- + " and "
- + ct
- + " for "
- + skyKey));
- return null;
- }
- env.getValue(getActionLookupData(testArtifact, actionLookupKey, actionLookupValue));
+ for (Artifact.DerivedArtifact testArtifact : TestProvider.getTestStatusArtifacts(ct)) {
+ env.getValue(testArtifact.getGeneratingActionKey());
if (env.valuesMissing()) {
return null;
}
}
} else {
- Multimap<ActionLookupValue.ActionLookupKey, Artifact.DerivedArtifact> keyToArtifactMap =
- Multimaps.index(
- TestProvider.getTestStatusArtifacts(ct), ArtifactFunction::getActionLookupKey);
- Map<SkyKey, SkyValue> actionLookupValues = env.getValues(keyToArtifactMap.keySet());
- if (env.valuesMissing()) {
- return null;
- }
env.getValues(
- keyToArtifactMap
- .entries()
- .stream()
- .map(
- entry ->
- getActionLookupData(
- entry.getValue(),
- entry.getKey(),
- (ActionLookupValue) actionLookupValues.get(entry.getKey())))
- .distinct()
- .collect(ImmutableSet.toImmutableSet()));
+ Iterables.transform(
+ TestProvider.getTestStatusArtifacts(ct),
+ Artifact.DerivedArtifact::getGeneratingActionKey));
if (env.valuesMissing()) {
return null;
}
@@ -103,14 +65,6 @@
return TestCompletionValue.TEST_COMPLETION_MARKER;
}
- private static ActionLookupData getActionLookupData(
- Artifact artifact,
- ActionLookupValue.ActionLookupKey actionLookupKey,
- ActionLookupValue actionLookupValue) {
- return ActionExecutionValue.key(
- actionLookupKey, actionLookupValue.getGeneratingActionIndex(artifact));
- }
-
@Override
public String extractTag(SkyKey skyKey) {
return Label.print(((ConfiguredTargetKey) skyKey.argument()).getLabel());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java
index 55876e3..da22b10 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java
@@ -39,7 +39,8 @@
Artifact volatileArtifact,
WorkspaceStatusAction workspaceStatusAction) {
super(
- Actions.GeneratingActions.fromSingleAction(workspaceStatusAction), /*nonceVersion=*/ null);
+ Actions.GeneratingActions.fromSingleAction(workspaceStatusAction, BUILD_INFO_KEY),
+ /*nonceVersion=*/ null);
this.stableArtifact = stableArtifact;
this.volatileArtifact = volatileArtifact;
}
