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/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 9e82277..96a7745 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -29,10 +29,11 @@
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionGraph;
+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.ArtifactFactory;
-import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.PackageRoots;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
@@ -77,7 +78,6 @@
import com.google.devtools.build.lib.syntax.SkylarkImport.SkylarkImportSyntaxException;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.RegexFilter;
-import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.WalkableGraph;
import java.util.ArrayList;
import java.util.Collection;
@@ -489,10 +489,11 @@
allTargetsToTest,
baselineCoverageArtifacts,
getArtifactFactory(),
+ skyframeExecutor.getActionKeyContext(),
CoverageReportValue.COVERAGE_REPORT_KEY,
loadingResult.getWorkspaceName());
if (actionsWrapper != null) {
- ImmutableList<ActionAnalysisMetadata> actions = actionsWrapper.getActions();
+ Actions.GeneratingActions actions = actionsWrapper.getActions();
skyframeExecutor.injectCoverageReportData(actions);
actionsWrapper.getCoverageOutputs().forEach(artifactsToOwnerLabelsBuilder::addArtifact);
}
@@ -516,19 +517,25 @@
@Nullable
@Override
public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) {
- ArtifactOwner artifactOwner = artifact.getArtifactOwner();
- if (artifactOwner instanceof ActionLookupValue.ActionLookupKey) {
- SkyKey key = (ActionLookupValue.ActionLookupKey) artifactOwner;
- ActionLookupValue val;
- try {
- val = (ActionLookupValue) graph.getValue(key);
- } catch (InterruptedException e) {
- throw new IllegalStateException(
- "Interruption not expected from this graph: " + key, e);
- }
- return val == null ? null : val.getGeneratingActionDangerousReadJavadoc(artifact);
+ if (artifact.isSourceArtifact()) {
+ return null;
}
- return null;
+ ActionLookupData generatingActionKey =
+ ((Artifact.DerivedArtifact) artifact).getGeneratingActionKey();
+ ActionLookupValue val;
+ try {
+ val = (ActionLookupValue) graph.getValue(generatingActionKey.getActionLookupKey());
+ } catch (InterruptedException e) {
+ throw new IllegalStateException(
+ "Interruption not expected from this graph: " + generatingActionKey, e);
+ }
+ if (val == null) {
+ return null;
+ }
+ int actionIndex = generatingActionKey.getActionIndex();
+ return val.isActionTemplate(actionIndex)
+ ? val.getActionTemplate(actionIndex)
+ : val.getAction(actionIndex);
}
};
return new AnalysisResult(
@@ -666,8 +673,8 @@
ImmutableList.Builder<Artifact> artifacts = ImmutableList.builder();
// Add to 'artifacts' all extra-actions which were registered by aspects which 'topLevel'
// might have injected.
- for (Artifact artifact : provider.getTransitiveExtraActionArtifacts()) {
- ArtifactOwner owner = artifact.getArtifactOwner();
+ for (Artifact.DerivedArtifact artifact : provider.getTransitiveExtraActionArtifacts()) {
+ ActionLookupValue.ActionLookupKey owner = artifact.getArtifactOwner();
if (owner instanceof AspectKey) {
if (aspectClasses.contains(((AspectKey) owner).getAspectClass())) {
artifacts.add(artifact);
