Refactor the ActionLookupValue/ConfiguredObjectValue hierarchy to reflect the reality that some configured targets will never have associated actions (all non-rule configured targets, including input/output files, aliases, and package groups). This allows us to more explicitly track the different categories of configured targets for metrics. Because non-rule configured targets can depend on rule configured targets (an output file depends on its generating rule, an alias depends on a target), the traversal of the analysis graph that powers these metrics must still visit these non-rule configured targets, allowing them to be tracked with little additional overhead. As a small optimization, we can delete from the Skyframe graph any nodes corresponding to non-rule configured targets if dropping the analysis phase. PiperOrigin-RevId: 368457602
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKey.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKey.java index 62db95b..fa81239 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKey.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupKey.java
@@ -16,7 +16,12 @@ import com.google.devtools.build.skyframe.SkyKey; /** - * {@link SkyKey} for an {@link ActionLookupValue}. + * {@link SkyKey} for an "analysis object": either an {@link ActionLookupValue} or a {@link + * com.google.devtools.build.lib.analysis.ConfiguredTargetValue}. + * + * <p>Whether a configured target creates actions cannot be inferred from its {@link + * com.google.devtools.build.lib.cmdline.Label} without performing analysis, so this class is used + * for both types. Non-{@link ActionLookupValue} nodes are not accessed during the execution phase. * * <p>All subclasses of {@link ActionLookupValue} "own" artifacts with {@link ArtifactOwner}s that * are subclasses of {@link ActionLookupKey}. This allows callers to easily find the value key,
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java index 6dbc68a..ef536c8 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java
@@ -14,9 +14,7 @@ package com.google.devtools.build.lib.actions; import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; import com.google.devtools.build.skyframe.SkyValue; -import javax.annotation.Nullable; /** Base interface for all values which can provide the generating action of an artifact. */ public interface ActionLookupValue extends SkyValue { @@ -52,11 +50,4 @@ default int getNumActions() { return getActions().size(); } - - /** Returns a source artifact if the underlying configured target is an input file. */ - @Nullable - default SourceArtifact getSourceArtifact() { - return null; - } - }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AnalysisGraphStatsEvent.java b/src/main/java/com/google/devtools/build/lib/actions/AnalysisGraphStatsEvent.java index 45a4039..4d6b82b 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AnalysisGraphStatsEvent.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AnalysisGraphStatsEvent.java
@@ -13,34 +13,22 @@ // limitations under the License. package com.google.devtools.build.lib.actions; +import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; + /** * Event transporting data about the size/shape of the analysis graph. Only emitted when Bazel is * forced to visit the entire analysis graph (for action/artifact conflict checking). See {@link * com.google.devtools.build.lib.skyframe.SkyframeBuildView#shouldCheckForConflicts}. */ public final class AnalysisGraphStatsEvent { - private final TotalAndConfiguredTargetOnlyMetric actionLookupValueCount; - private final TotalAndConfiguredTargetOnlyMetric actionCount; - private final int outputArtifactCount; + private final BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics buildGraphMetrics; public AnalysisGraphStatsEvent( - TotalAndConfiguredTargetOnlyMetric actionLookupValueCount, - TotalAndConfiguredTargetOnlyMetric actionCount, - int outputArtifactCount) { - this.actionLookupValueCount = actionLookupValueCount; - this.actionCount = actionCount; - this.outputArtifactCount = outputArtifactCount; + BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics buildGraphMetrics) { + this.buildGraphMetrics = buildGraphMetrics; } - public TotalAndConfiguredTargetOnlyMetric getActionLookupValueCount() { - return actionLookupValueCount; - } - - public TotalAndConfiguredTargetOnlyMetric getActionCount() { - return actionCount; - } - - public int getOutputArtifactCount() { - return outputArtifactCount; + public BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics getBuildGraphMetrics() { + return buildGraphMetrics; } }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 9e9551d..153b81e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD
@@ -155,7 +155,7 @@ java_library( name = "analysis_graph_stats_event", srcs = ["AnalysisGraphStatsEvent.java"], - deps = [":total_and_configured_target_only_metric"], + deps = ["//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto"], ) java_library(
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectValue.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectValue.java index 58a84d0..0c18c7f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectValue.java
@@ -24,7 +24,7 @@ import net.starlark.java.syntax.Location; /** An aspect in the context of the Skyframe graph. */ -public final class AspectValue extends BasicActionLookupValue implements ConfiguredObjectValue { +public final class AspectValue extends BasicActionLookupValue implements RuleConfiguredObjectValue { // These variables are only non-final because they may be clear()ed to save memory. They are null // only after they are cleared except for transitivePackagesForPackageRootResolution. @Nullable private Aspect aspect;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 4a3d8c1..e41e289 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -313,7 +313,6 @@ ":config/transitions/split_transition", ":config/transitions/transition_factory", ":configurations_collector", - ":configured_object_value", ":configured_target", ":constraints/constraint_constants", ":constraints/constraint_semantics", @@ -341,6 +340,7 @@ ":provider_collection", ":required_config_fragments_provider", ":resolved_toolchain_context", + ":rule_configured_object_value", ":rule_definition_context", ":rule_definition_environment", ":starlark/args", @@ -689,7 +689,6 @@ srcs = ["ConfiguredObjectValue.java"], deps = [ ":provider_collection", - "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/skyframe", @@ -697,12 +696,20 @@ ) java_library( + name = "rule_configured_object_value", + srcs = ["RuleConfiguredObjectValue.java"], + deps = [ + ":configured_object_value", + "//src/main/java/com/google/devtools/build/lib/actions", + ], +) + +java_library( name = "configured_target", srcs = ["ConfiguredTarget.java"], deps = [ ":config/config_matching_provider", ":transitive_info_collection", - "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/skyframe:build_configuration_value", "//src/main/java/net/starlark/java/eval",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredObjectValue.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredObjectValue.java index 353cbd5..b06000a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredObjectValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredObjectValue.java
@@ -13,13 +13,15 @@ // limitations under the License. package com.google.devtools.build.lib.analysis; -import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.skyframe.NotComparableSkyValue; -/** Super-interface for {@link ConfiguredTargetValue} and {@link AspectValue}. */ -public interface ConfiguredObjectValue extends ActionLookupValue, NotComparableSkyValue { +/** + * Super-interface for {@link ConfiguredTargetValue} and {@link RuleConfiguredObjectValue} + * (transitively including {@link AspectValue}). + */ +public interface ConfiguredObjectValue extends NotComparableSkyValue { /** Returns the configured target/aspect for this value. */ ProviderCollection getConfiguredObject(); @@ -34,11 +36,10 @@ NestedSet<Package> getTransitivePackagesForPackageRootResolution(); /** - * Clears provider data from this value, leaving only the artifact->generating action map. + * Clears data from this value. * * <p>Should only be used when user specifies --discard_analysis_cache. Must be called at most - * once per value, after which {@link #getConfiguredObject} and {@link #getActions} cannot be - * called. + * once per value, after which this object's other methods cannot be called. */ void clear(boolean clearEverything); }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java index 4404f56..735276c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java
@@ -16,7 +16,6 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.skyframe.BuildConfigurationValue; @@ -71,12 +70,6 @@ @Override Object getValue(String name); - /** Returns a source artifact if this is an input file. */ - @Nullable - default SourceArtifact getSourceArtifact() { - return null; - } - /** * If the configured target is an alias, return the actual target, otherwise return the current * target. This follows alias chains.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredObjectValue.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredObjectValue.java new file mode 100644 index 0000000..74e1333 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredObjectValue.java
@@ -0,0 +1,23 @@ +// Copyright 2021 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.analysis; + +import com.google.devtools.build.lib.actions.ActionLookupValue; + +/** + * Common interface for {@link AspectValue} and {@link + * com.google.devtools.build.lib.skyframe.RuleConfiguredTargetValue} + */ +public interface RuleConfiguredObjectValue extends ConfiguredObjectValue, ActionLookupValue {}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/InputFileConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/InputFileConfiguredTarget.java index d980d27..e397337 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/InputFileConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/InputFileConfiguredTarget.java
@@ -101,7 +101,6 @@ printer.append("<input file target " + getLabel() + ">"); } - @Override public SourceArtifact getSourceArtifact() { return artifact; }
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto index 75c6b78..21c93a6 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
@@ -865,11 +865,14 @@ message BuildGraphMetrics { // How many configured targets/aspects were in this build, including any // that were analyzed on a prior build and are still valid. May not be - // populated if analysis phase was fully cached. + // populated if analysis phase was fully cached. Note: for historical + // reasons this includes input/output files and other configured targets + // that do not actually have associated actions. int32 action_lookup_value_count = 1; // How many configured targets alone were in this build: always at most // action_lookup_value_count. Useful mainly for historical comparisons to - // TargetMetrics.targets_configured, which used to not count aspects. + // TargetMetrics.targets_configured, which used to not count aspects. This + // also includes configured targets that do not have associated actions. int32 action_lookup_value_count_not_including_aspects = 5; // How many actions belonged to the configured targets/aspects above. It may // not be necessary to execute all of these actions to build the requested @@ -879,6 +882,15 @@ // action_count. Useful mainly for historical comparisons to // ActionMetrics.actions_created, which used to not count aspects' actions. int32 action_count_not_including_aspects = 6; + // How many "input file" configured targets there were: one per source file. + // Should agree with artifact_metrics.source_artifacts_read.count above, + int32 input_file_configured_target_count = 7; + // How many "output file" configured targets there were: output files that + // are targets (not implicit outputs). + int32 output_file_configured_target_count = 8; + // How many "other" configured targets there were (like alias, + // package_group, and other non-rule non-file configured targets). + int32 other_configured_target_count = 9; // How many artifacts are outputs of the above actions. May not be populated // if analysis phase was fully cached. int32 output_artifact_count = 3;
diff --git a/src/main/java/com/google/devtools/build/lib/metrics/BUILD b/src/main/java/com/google/devtools/build/lib/metrics/BUILD index 45af86c..dcb1be5 100644 --- a/src/main/java/com/google/devtools/build/lib/metrics/BUILD +++ b/src/main/java/com/google/devtools/build/lib/metrics/BUILD
@@ -37,6 +37,7 @@ "//src/main/java/com/google/devtools/build/lib/actions:analysis_graph_stats_event", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_phase_complete_event", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_phase_started_event", + "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/skyframe:execution_finished_event",
diff --git a/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java b/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java index 18d2b2d..4666d56 100644 --- a/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java +++ b/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.TotalAndConfiguredTargetOnlyMetric; import com.google.devtools.build.lib.analysis.AnalysisPhaseCompleteEvent; import com.google.devtools.build.lib.analysis.AnalysisPhaseStartedEvent; +import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.ActionSummary; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.ArtifactMetrics; @@ -100,15 +101,16 @@ @SuppressWarnings("unused") @Subscribe public synchronized void logAnalysisGraphStats(AnalysisGraphStatsEvent event) { - TotalAndConfiguredTargetOnlyMetric actionLookupValueCount = event.getActionLookupValueCount(); - TotalAndConfiguredTargetOnlyMetric actionCount = event.getActionCount(); - buildGraphMetrics - .setActionLookupValueCount(actionLookupValueCount.total()) - .setActionLookupValueCountNotIncludingAspects( - actionLookupValueCount.configuredTargetsOnly()) - .setActionCount(actionCount.total()) - .setActionCountNotIncludingAspects(actionCount.configuredTargetsOnly()) - .setOutputArtifactCount(event.getOutputArtifactCount()); + // Check only one event per build. No proto3 check for presence, so check for not-default value. + if (buildGraphMetrics.getActionLookupValueCount() > 0) { + BugReport.sendBugReport( + new IllegalStateException( + "Already initialized build graph metrics builder: " + + buildGraphMetrics + + ", " + + event.getBuildGraphMetrics())); + } + buildGraphMetrics.mergeFrom(event.getBuildGraphMetrics()); } @SuppressWarnings("unused")
diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphProtoOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphProtoOutputFormatterCallback.java index 945a240..e29e5b2 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphProtoOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphProtoOutputFormatterCallback.java
@@ -16,9 +16,9 @@ import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.analysis.AspectValue; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; -import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetAccessor; +import com.google.devtools.build.lib.skyframe.RuleConfiguredTargetValue; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; import com.google.devtools.build.lib.skyframe.actiongraph.v2.ActionGraphDump; import com.google.devtools.build.lib.skyframe.actiongraph.v2.AqueryOutputHandler; @@ -92,12 +92,15 @@ options.includeCommandline |= options.includeParamFiles; for (ConfiguredTargetValue configuredTargetValue : partialResult) { - actionGraphDump.dumpConfiguredTarget(configuredTargetValue); + if (!(configuredTargetValue instanceof RuleConfiguredTargetValue)) { + // We have to include non-rule values in the graph to visit their dependencies, but they + // don't have any actions to print out. + continue; + } + actionGraphDump.dumpConfiguredTarget((RuleConfiguredTargetValue) configuredTargetValue); if (options.useAspects) { - if (configuredTargetValue.getConfiguredTarget() instanceof RuleConfiguredTarget) { - for (AspectValue aspectValue : accessor.getAspectValues(configuredTargetValue)) { - actionGraphDump.dumpAspect(aspectValue, configuredTargetValue); - } + for (AspectValue aspectValue : accessor.getAspectValues(configuredTargetValue)) { + actionGraphDump.dumpAspect(aspectValue, configuredTargetValue); } } }
diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java index fccabc4..29ac05c 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java
@@ -29,12 +29,12 @@ import com.google.devtools.build.lib.analysis.AspectValue; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction; -import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.buildeventstream.BuildEvent; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.packages.AspectDescriptor; import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetAccessor; +import com.google.devtools.build.lib.skyframe.RuleConfiguredTargetValue; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; import com.google.devtools.build.lib.util.CommandDescriptionForm; import com.google.devtools.build.lib.util.CommandFailureUtils; @@ -79,15 +79,19 @@ options.includeCommandline |= options.includeParamFiles; for (ConfiguredTargetValue configuredTargetValue : partialResult) { - for (ActionAnalysisMetadata action : configuredTargetValue.getActions()) { + if (!(configuredTargetValue instanceof RuleConfiguredTargetValue)) { + // We have to include non-rule values in the graph to visit their dependencies, but they + // don't have any actions to print out. + continue; + } + for (ActionAnalysisMetadata action : + ((RuleConfiguredTargetValue) configuredTargetValue).getActions()) { writeAction(action, printStream); } if (options.useAspects) { - if (configuredTargetValue.getConfiguredTarget() instanceof RuleConfiguredTarget) { - for (AspectValue aspectValue : accessor.getAspectValues(configuredTargetValue)) { - for (ActionAnalysisMetadata action : aspectValue.getActions()) { - writeAction(action, printStream); - } + for (AspectValue aspectValue : accessor.getAspectValues(configuredTargetValue)) { + for (ActionAnalysisMetadata action : aspectValue.getActions()) { + writeAction(action, printStream); } } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java index 8eb9a8f..611eae5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactConflictFinder.java
@@ -23,19 +23,15 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Actions; -import com.google.devtools.build.lib.actions.AnalysisGraphStatsEvent; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException; import com.google.devtools.build.lib.actions.MapBasedActionGraph; import com.google.devtools.build.lib.actions.MutableActionGraph; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; -import com.google.devtools.build.lib.actions.TotalAndConfiguredTargetOnlyMetric; -import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper; import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed; -import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.List; import java.util.Map; @@ -50,6 +46,8 @@ class ArtifactConflictFinder { static final Precomputed<ImmutableMap<ActionAnalysisMetadata, ConflictException>> ACTION_CONFLICTS = new Precomputed<>("action_conflicts"); + // Action graph construction is CPU-bound. + static final int NUM_JOBS = Runtime.getRuntime().availableProcessors(); private ArtifactConflictFinder() {} @@ -65,7 +63,7 @@ * build as of 2014), so it should only be called when necessary. */ static ActionConflictsAndStats findAndStoreArtifactConflicts( - Iterable<ActionLookupValue> actionLookupValues, + Sharder<ActionLookupValue> actionLookupValues, boolean strictConflictChecks, ActionKeyContext actionKeyContext) throws InterruptedException { @@ -74,9 +72,8 @@ MapBasedActionGraph actionGraph = new MapBasedActionGraph(actionKeyContext); ConcurrentNavigableMap<PathFragment, Artifact> artifactPathMap = new ConcurrentSkipListMap<>(Actions.comparatorForPrefixConflicts()); - Pair<TotalAndConfiguredTargetOnlyMetric, TotalAndConfiguredTargetOnlyMetric> counts = - constructActionGraphAndPathMap( - actionGraph, artifactPathMap, actionLookupValues, temporaryBadActionMap); + constructActionGraphAndPathMap( + actionGraph, artifactPathMap, actionLookupValues, temporaryBadActionMap); Map<ActionAnalysisMetadata, ArtifactPrefixConflictException> actionsWithArtifactPrefixConflict = Actions.findArtifactPrefixConflicts(actionGraph, artifactPathMap, strictConflictChecks); @@ -87,8 +84,6 @@ } return ActionConflictsAndStats.create( ImmutableMap.copyOf(temporaryBadActionMap), - counts.first, - counts.second, actionGraph.getSize()); } @@ -97,53 +92,33 @@ * PathFragment}s to their respective {@link Artifact}s. We do this in a threadpool to save around * 1.5 seconds on a mid-sized build versus a single-threaded operation. */ - private static Pair<TotalAndConfiguredTargetOnlyMetric, TotalAndConfiguredTargetOnlyMetric> - constructActionGraphAndPathMap( - MutableActionGraph actionGraph, - ConcurrentNavigableMap<PathFragment, Artifact> artifactPathMap, - Iterable<ActionLookupValue> values, - ConcurrentMap<ActionAnalysisMetadata, ConflictException> badActionMap) - throws InterruptedException { - // Action graph construction is CPU-bound. - int numJobs = Runtime.getRuntime().availableProcessors(); - // No great reason for expecting 5000 action lookup values, but not worth counting size of - // values. - Sharder<ActionLookupValue> actionShards = new Sharder<>(numJobs, 5000); - int actionLookupValueCount = 0; - int configuredTargetValueCount = 0; - int actionCount = 0; - int configuredTargetActionCount = 0; - for (ActionLookupValue value : values) { - actionShards.add(value); - actionLookupValueCount++; - actionCount += value.getNumActions(); - if (value instanceof ConfiguredTargetValue) { - configuredTargetValueCount++; - configuredTargetActionCount += value.getNumActions(); - } - } - + private static void constructActionGraphAndPathMap( + MutableActionGraph actionGraph, + ConcurrentNavigableMap<PathFragment, Artifact> artifactPathMap, + Sharder<ActionLookupValue> actionShards, + ConcurrentMap<ActionAnalysisMetadata, ConflictException> badActionMap) + throws InterruptedException { ThrowableRecordingRunnableWrapper wrapper = new ThrowableRecordingRunnableWrapper( "ArtifactConflictFinder#constructActionGraphAndPathMap"); ExecutorService executor = Executors.newFixedThreadPool( - numJobs, + NUM_JOBS, new ThreadFactoryBuilder().setNameFormat("ActionLookupValue Processor %d").build()); for (List<ActionLookupValue> shard : actionShards) { executor.execute( wrapper.wrap(actionRegistration(shard, actionGraph, artifactPathMap, badActionMap))); } boolean interrupted = ExecutorUtil.interruptibleShutdown(executor); - Throwables.propagateIfPossible(wrapper.getFirstThrownError()); + Throwable firstThrownError = wrapper.getFirstThrownError(); + if (firstThrownError != null) { + Throwables.throwIfUnchecked(firstThrownError); + throw new IllegalStateException(firstThrownError); + } if (interrupted) { throw new InterruptedException(); } - return Pair.of( - TotalAndConfiguredTargetOnlyMetric.create( - actionLookupValueCount, configuredTargetValueCount), - TotalAndConfiguredTargetOnlyMetric.create(actionCount, configuredTargetActionCount)); } private static Runnable actionRegistration( @@ -215,15 +190,12 @@ abstract static class ActionConflictsAndStats { abstract ImmutableMap<ActionAnalysisMetadata, ConflictException> getConflicts(); - abstract AnalysisGraphStatsEvent getStats(); + abstract int getOutputArtifactCount(); private static ActionConflictsAndStats create( ImmutableMap<ActionAnalysisMetadata, ConflictException> conflicts, - TotalAndConfiguredTargetOnlyMetric actionValueCount, - TotalAndConfiguredTargetOnlyMetric actionCount, int artifactCount) { - return new AutoValue_ArtifactConflictFinder_ActionConflictsAndStats( - conflicts, new AnalysisGraphStatsEvent(actionValueCount, actionCount, artifactCount)); + return new AutoValue_ArtifactConflictFinder_ActionConflictsAndStats(conflicts, artifactCount); } } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 2f7a38d..6ff4409 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -219,6 +219,7 @@ "//src/main/java/com/google/devtools/build/lib:keep-going-option", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", + "//src/main/java/com/google/devtools/build/lib/actions:analysis_graph_stats_event", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", @@ -255,6 +256,7 @@ "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:platform_options", "//src/main/java/com/google/devtools/build/lib/analysis:resolved_toolchain_context", + "//src/main/java/com/google/devtools/build/lib/analysis:rule_configured_object_value", "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection", "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_context", "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context", @@ -760,11 +762,8 @@ deps = [ ":precomputed_value", "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/actions:analysis_graph_stats_event", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", - "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value", "//src/main/java/com/google/devtools/build/lib/concurrent", - "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:auto_value", "//third_party:guava",
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 e33b6a9..950693a 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
@@ -22,8 +22,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.flogger.GoogleLogger; -import com.google.devtools.build.lib.actions.Actions; -import com.google.devtools.build.lib.actions.Actions.GeneratingActions; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.AnalysisRootCauseEvent; import com.google.devtools.build.lib.analysis.AspectResolver; @@ -242,7 +240,6 @@ // not prepared for it. return new NonRuleConfiguredTargetValue( new EmptyConfiguredTarget(target.getLabel(), configuredTargetKey.getConfigurationKey()), - GeneratingActions.EMPTY, transitivePackagesForPackageRootResolution == null ? null : transitivePackagesForPackageRootResolution.build()); @@ -1059,22 +1056,14 @@ ? null : transitivePackagesForPackageRootResolution.build()); } else { - GeneratingActions generatingActions; - // Check for conflicting actions within this configured target (that indicates a bug in the - // rule implementation). - try { - generatingActions = - Actions.assignOwnersAndFilterSharedActionsAndThrowActionConflict( - analysisEnvironment.getActionKeyContext(), - analysisEnvironment.getRegisteredActions(), - configuredTargetKey, - /*outputFiles=*/ null); - } catch (ActionConflictException e) { - throw new ConfiguredTargetFunctionException(e); - } + Preconditions.checkState( + analysisEnvironment.getRegisteredActions().isEmpty(), + "Non-rule can't have actions: %s %s %s %s", + configuredTargetKey, + analysisEnvironment.getRegisteredActions(), + configuredTarget); return new NonRuleConfiguredTargetValue( configuredTarget, - generatingActions, transitivePackagesForPackageRootResolution == null ? null : transitivePackagesForPackageRootResolution.build());
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 3826ef9..e1c96d2 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
@@ -13,13 +13,8 @@ // 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.Preconditions; -import com.google.common.collect.ImmutableList; -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.SourceArtifact; -import com.google.devtools.build.lib.actions.BasicActionLookupValue; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; @@ -36,10 +31,7 @@ @ThreadSafe // Reached via OutputFileConfiguredTarget. @AutoCodec(explicitlyAllowClass = RuleConfiguredTarget.class) -@VisibleForTesting -public final class NonRuleConfiguredTargetValue extends BasicActionLookupValue - implements ConfiguredTargetValue { - +public final class NonRuleConfiguredTargetValue implements ConfiguredTargetValue { // These variables are only non-final because they may be clear()ed to save memory. // configuredTarget is null only after it is cleared. @Nullable private ConfiguredTarget configuredTarget; @@ -50,20 +42,15 @@ @AutoCodec.Instantiator @VisibleForSerialization NonRuleConfiguredTargetValue( - ImmutableList<ActionAnalysisMetadata> actions, ConfiguredTarget configuredTarget) { - super(actions); - this.configuredTarget = configuredTarget; // Transitive packages are not serialized. - this.transitivePackagesForPackageRootResolution = null; + this(configuredTarget, null); } NonRuleConfiguredTargetValue( ConfiguredTarget configuredTarget, - GeneratingActions generatingActions, @Nullable NestedSet<Package> transitivePackagesForPackageRootResolution) { - super(generatingActions); - this.configuredTarget = Preconditions.checkNotNull(configuredTarget, generatingActions); + this.configuredTarget = Preconditions.checkNotNull(configuredTarget); this.transitivePackagesForPackageRootResolution = transitivePackagesForPackageRootResolution; } @@ -74,12 +61,6 @@ } @Override - public ImmutableList<ActionAnalysisMetadata> getActions() { - Preconditions.checkNotNull(configuredTarget, this); - return actions; - } - - @Override public NestedSet<Package> getTransitivePackagesForPackageRootResolution() { return Preconditions.checkNotNull(transitivePackagesForPackageRootResolution); } @@ -95,11 +76,6 @@ @Override public String toString() { - return getStringHelper().add("configuredTarget", configuredTarget).toString(); - } - - @Override - public SourceArtifact getSourceArtifact() { - return configuredTarget == null ? null : configuredTarget.getSourceArtifact(); + return MoreObjects.toStringHelper(this).add("configuredTarget", configuredTarget).toString(); } }
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 e94b49f..a1ca50c 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
@@ -17,9 +17,9 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; +import com.google.devtools.build.lib.analysis.RuleConfiguredObjectValue; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; @@ -32,7 +32,8 @@ @Immutable @ThreadSafe @AutoCodec(explicitlyAllowClass = RuleConfiguredTarget.class) -public final class RuleConfiguredTargetValue implements ActionLookupValue, ConfiguredTargetValue { +public final class RuleConfiguredTargetValue + implements RuleConfiguredObjectValue, ConfiguredTargetValue { // This variable is non-final because it may be clear()ed to save memory. It is null only after // clear(true) is called.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index 8e29c08..9e9638b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -762,8 +762,8 @@ SkyValue value = skyKeyAndValue.getValue(); SkyKey key = skyKeyAndValue.getKey(); SkyFunctionName functionName = key.functionName(); - if (functionName.equals(SkyFunctions.CONFIGURED_TARGET)) { - ConfiguredTargetValue ctValue = (ConfiguredTargetValue) value; + if (value instanceof RuleConfiguredTargetValue) { + RuleConfiguredTargetValue ctValue = (RuleConfiguredTargetValue) value; ConfiguredTarget configuredTarget = ctValue.getConfiguredTarget(); if (configuredTarget instanceof RuleConfiguredTarget) { @@ -808,20 +808,20 @@ memoizingEvaluator.getDoneValues().entrySet()) { SkyKey key = skyKeyAndValue.getKey(); SkyValue skyValue = skyKeyAndValue.getValue(); - SkyFunctionName functionName = key.functionName(); - try { + if (skyValue == null) { // The skyValue may be null in case analysis of the previous build failed. - if (skyValue != null) { - if (functionName.equals(SkyFunctions.CONFIGURED_TARGET)) { - actionGraphDump.dumpConfiguredTarget((ConfiguredTargetValue) skyValue); - } else if (functionName.equals(SkyFunctions.ASPECT)) { - AspectValue aspectValue = (AspectValue) skyValue; - AspectKey aspectKey = (AspectKey) key; - ConfiguredTargetValue configuredTargetValue = - (ConfiguredTargetValue) - memoizingEvaluator.getExistingValue(aspectKey.getBaseConfiguredTargetKey()); - actionGraphDump.dumpAspect(aspectValue, configuredTargetValue); - } + continue; + } + try { + if (skyValue instanceof RuleConfiguredTargetValue) { + actionGraphDump.dumpConfiguredTarget((RuleConfiguredTargetValue) skyValue); + } else if (key.functionName().equals(SkyFunctions.ASPECT)) { + AspectValue aspectValue = (AspectValue) skyValue; + AspectKey aspectKey = (AspectKey) key; + ConfiguredTargetValue configuredTargetValue = + (ConfiguredTargetValue) + memoizingEvaluator.getExistingValue(aspectKey.getBaseConfiguredTargetKey()); + actionGraphDump.dumpAspect(aspectValue, configuredTargetValue); } } catch (InterruptedException e) { Thread.currentThread().interrupt(); @@ -840,20 +840,20 @@ memoizingEvaluator.getDoneValues().entrySet()) { SkyKey key = skyKeyAndValue.getKey(); SkyValue skyValue = skyKeyAndValue.getValue(); - SkyFunctionName functionName = key.functionName(); - try { + if (skyValue == null) { // The skyValue may be null in case analysis of the previous build failed. - if (skyValue != null) { - if (functionName.equals(SkyFunctions.CONFIGURED_TARGET)) { - actionGraphDump.dumpConfiguredTarget((ConfiguredTargetValue) skyValue); - } else if (functionName.equals(SkyFunctions.ASPECT)) { - AspectValue aspectValue = (AspectValue) skyValue; - AspectKey aspectKey = (AspectKey) key; - ConfiguredTargetValue configuredTargetValue = - (ConfiguredTargetValue) - memoizingEvaluator.getExistingValue(aspectKey.getBaseConfiguredTargetKey()); - actionGraphDump.dumpAspect(aspectValue, configuredTargetValue); - } + continue; + } + try { + if (skyValue instanceof RuleConfiguredTargetValue) { + actionGraphDump.dumpConfiguredTarget((RuleConfiguredTargetValue) skyValue); + } else if (key.functionName().equals(SkyFunctions.ASPECT)) { + AspectValue aspectValue = (AspectValue) skyValue; + AspectKey aspectKey = (AspectKey) key; + ConfiguredTargetValue configuredTargetValue = + (ConfiguredTargetValue) + memoizingEvaluator.getExistingValue(aspectKey.getBaseConfiguredTargetKey()); + actionGraphDump.dumpAspect(aspectValue, configuredTargetValue); } } catch (InterruptedException e) { Thread.currentThread().interrupt();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index 1a90a8a..2cce56f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.ActionLookupValue; +import com.google.devtools.build.lib.actions.AnalysisGraphStatsEvent; import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; @@ -59,6 +60,7 @@ import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.FragmentClassSet; import com.google.devtools.build.lib.bugreport.BugReport; +import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId.ConfigurationId; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.causes.AnalysisFailedCause; @@ -142,7 +144,7 @@ private BuildConfiguration topLevelHostConfiguration; // Fragment-limited versions of the host configuration. It's faster to create/cache these here // than to store them in Skyframe. - private Map<BuildConfiguration, BuildConfiguration> hostConfigurationCache = + private final Map<BuildConfiguration, BuildConfiguration> hostConfigurationCache = Maps.newConcurrentMap(); private BuildConfigurationCollection configurations; @@ -174,8 +176,7 @@ public TotalAndConfiguredTargetOnlyMetric getEvaluatedCounts() { return TotalAndConfiguredTargetOnlyMetric.create( - progressReceiver.actionLookupValueCount.get(), - progressReceiver.configuredTargetCount.get()); + progressReceiver.configuredObjectCount.get(), progressReceiver.configuredTargetCount.get()); } ConfiguredTargetFactory getConfiguredTargetFactory() { @@ -448,12 +449,17 @@ // This operation is somewhat expensive, so we only do it if the graph might have changed in // some way -- either we analyzed a new target or we invalidated an old one or are building // targets together that haven't been built before. + SkyframeExecutor.AnalysisTraversalResult analysisTraversalResult = + skyframeExecutor.getActionLookupValuesInBuild(ctKeys, aspectKeys); ArtifactConflictFinder.ActionConflictsAndStats conflictsAndStats = ArtifactConflictFinder.findAndStoreArtifactConflicts( - skyframeExecutor.getActionLookupValuesInBuild(ctKeys, aspectKeys), - strictConflictChecks, - actionKeyContext); - eventBus.post(conflictsAndStats.getStats()); + analysisTraversalResult.getActionShards(), strictConflictChecks, actionKeyContext); + BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics buildGraphMetrics = + analysisTraversalResult + .getMetrics() + .setOutputArtifactCount(conflictsAndStats.getOutputArtifactCount()) + .build(); + eventBus.post(new AnalysisGraphStatsEvent(buildGraphMetrics)); actionConflicts = conflictsAndStats.getConflicts(); someActionLookupValueEvaluated = false; } @@ -1135,7 +1141,7 @@ } private final class ActionLookupValueProgressReceiver implements EvaluationProgressReceiver { - private final AtomicInteger actionLookupValueCount = new AtomicInteger(); + private final AtomicInteger configuredObjectCount = new AtomicInteger(); private final AtomicInteger actionCount = new AtomicInteger(); private final AtomicInteger configuredTargetCount = new AtomicInteger(); private final AtomicInteger configuredTargetActionCount = new AtomicInteger(); @@ -1165,16 +1171,17 @@ } switch (state) { case BUILT: + if (!evaluationSuccessState.get().succeeded()) { + return; + } + configuredObjectCount.incrementAndGet(); boolean isConfiguredTarget = skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET); - if (evaluationSuccessState.get().succeeded()) { - actionLookupValueCount.incrementAndGet(); - if (isConfiguredTarget) { - configuredTargetCount.incrementAndGet(); - } - // During multithreaded operation, this is only set to true, so no concurrency issues. - someActionLookupValueEvaluated = true; + if (isConfiguredTarget) { + configuredTargetCount.incrementAndGet(); } if (newValue instanceof ActionLookupValue) { + // During multithreaded operation, this is only set to true, so no concurrency issues. + someActionLookupValueEvaluated = true; int numActions = ((ActionLookupValue) newValue).getNumActions(); actionCount.addAndGet(numActions); if (isConfiguredTarget) { @@ -1190,7 +1197,7 @@ } public void reset() { - actionLookupValueCount.set(0); + configuredObjectCount.set(0); actionCount.set(0); configuredTargetCount.set(0); configuredTargetActionCount.set(0);
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 0965be7..74c0d5b 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
@@ -15,6 +15,7 @@ import static com.google.devtools.build.lib.concurrent.Uninterruptibles.callUninterruptibly; import static com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.ACTION_CONFLICTS; +import static com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.NUM_JOBS; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -97,10 +98,14 @@ import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; import com.google.devtools.build.lib.analysis.config.transitions.NoTransition; import com.google.devtools.build.lib.analysis.config.transitions.NullTransition; +import com.google.devtools.build.lib.analysis.configuredtargets.InputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.MergedConfiguredTarget; +import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition; import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition.TransitionException; +import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.bugreport.BugReporter; +import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; @@ -108,6 +113,7 @@ import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetExpander; import com.google.devtools.build.lib.concurrent.NamedForkJoinPool; +import com.google.devtools.build.lib.concurrent.Sharder; import com.google.devtools.build.lib.concurrent.ThreadSafety; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.events.ErrorSensingEventHandler; @@ -1090,7 +1096,17 @@ } // ctValue may be null if target was not successfully analyzed. if (ctValue != null) { - ctValue.clear(!topLevelTargets.contains(ctValue.getConfiguredTarget())); + if (!(ctValue instanceof ActionLookupValue) + && discardType.discardsLoading() + && !topLevelTargets.contains(ctValue.getConfiguredTarget())) { + // If loading is already being deleted, deleting these nodes doesn't hurt. Morally + // we should always be able to delete these, since they're not used for execution, + // but it leaves the graph inconsistent, and the --discard_analysis_cache with + // --track_incremental_state case isn't worth optimizing for. + it.remove(); + } else { + ctValue.clear(!topLevelTargets.contains(ctValue.getConfiguredTarget())); + } } } else if (functionName.equals(SkyFunctions.ASPECT)) { AspectValue aspectValue; @@ -3142,18 +3158,24 @@ return ExecutionFinishedEvent.builderWithDefaults(); } - final Iterable<ActionLookupValue> getActionLookupValuesInBuild( + final AnalysisTraversalResult getActionLookupValuesInBuild( List<ConfiguredTargetKey> topLevelCtKeys, List<AspectValueKey> aspectKeys) throws InterruptedException { + AnalysisTraversalResult result = new AnalysisTraversalResult(); if (!isAnalysisIncremental()) { - // If we do not track incremental state we do not have graph edges, so we cannot traverse the - // graph and find only actions in the current build. In this case we can simply return all - // ActionLookupValues in the graph, since the graph's lifetime is a single build anyway. - return Iterables.filter(memoizingEvaluator.getDoneValues().values(), ActionLookupValue.class); + // If we do not have incremental analysis state we do not have graph edges, so we cannot + // traverse the graph and find only actions in the current build. In this case we can simply + // return all ActionLookupValues in the graph, since the graph's lifetime is a single build + // anyway. + for (Map.Entry<SkyKey, SkyValue> entry : memoizingEvaluator.getDoneValues().entrySet()) { + if ((entry.getKey() instanceof ActionLookupKey) && entry.getValue() != null) { + result.accumulate(entry.getValue(), (ActionLookupKey) entry.getKey()); + } + } + return result; } WalkableGraph walkableGraph = SkyframeExecutorWrappingWalkableGraph.of(this); Set<SkyKey> seen = CompactHashSet.create(); - List<ActionLookupValue> result = new ArrayList<>(); for (ConfiguredTargetKey key : topLevelCtKeys) { findActionsRecursively(walkableGraph, key, seen, result); } @@ -3163,22 +3185,95 @@ return result; } + static class AnalysisTraversalResult { + // Some metrics indicate this is a rough average # of ALVs in a build. + private final Sharder<ActionLookupValue> actionShards = new Sharder<>(NUM_JOBS, 200_000); + + // Metrics. + private int configuredObjectCount = 0; + private int configuredTargetCount = 0; + private int actionCount = 0; + private int actionCountNotIncludingAspects = 0; + private int inputFileConfiguredTargetCount = 0; + private int outputFileConfiguredTargetCount = 0; + private int otherConfiguredTargetCount = 0; + + private AnalysisTraversalResult() {} + + private void accumulate(SkyValue value, ActionLookupKey keyForDebugging) { + boolean isConfiguredTarget = value instanceof ConfiguredTargetValue; + boolean isActionLookupValue = value instanceof ActionLookupValue; + if (!isConfiguredTarget && !isActionLookupValue) { + BugReport.sendBugReport( + new IllegalStateException( + String.format( + "Should only be called with ConfiguredTargetValue or ActionLookupValue: %s %s" + + " %s", + value.getClass(), keyForDebugging, value))); + return; + } + configuredObjectCount++; + if (isConfiguredTarget) { + configuredTargetCount++; + } + if (isActionLookupValue) { + ActionLookupValue alv = (ActionLookupValue) value; + int numActions = alv.getNumActions(); + actionCount += numActions; + if (isConfiguredTarget) { + actionCountNotIncludingAspects += numActions; + } + actionShards.add(alv); + return; + } + if (!(value instanceof NonRuleConfiguredTargetValue)) { + BugReport.sendBugReport( + new IllegalStateException( + String.format( + "Unexpected value type: %s %s %s", value.getClass(), keyForDebugging, value))); + return; + } + ConfiguredTarget configuredTarget = + ((NonRuleConfiguredTargetValue) value).getConfiguredTarget(); + if (configuredTarget instanceof InputFileConfiguredTarget) { + inputFileConfiguredTargetCount++; + } else if (configuredTarget instanceof OutputFileConfiguredTarget) { + outputFileConfiguredTargetCount++; + } else { + otherConfiguredTargetCount++; + } + } + + Sharder<ActionLookupValue> getActionShards() { + return actionShards; + } + + BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics.Builder getMetrics() { + return BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics.newBuilder() + .setActionLookupValueCount(configuredObjectCount) + .setActionLookupValueCountNotIncludingAspects(configuredTargetCount) + .setActionCount(actionCount) + .setActionCountNotIncludingAspects(actionCountNotIncludingAspects) + .setInputFileConfiguredTargetCount(inputFileConfiguredTargetCount) + .setOutputFileConfiguredTargetCount(outputFileConfiguredTargetCount) + .setOtherConfiguredTargetCount(otherConfiguredTargetCount); + } + } + private static void findActionsRecursively( - WalkableGraph walkableGraph, SkyKey key, Set<SkyKey> seen, List<ActionLookupValue> result) + WalkableGraph walkableGraph, SkyKey key, Set<SkyKey> seen, AnalysisTraversalResult result) throws InterruptedException { if (!(key instanceof ActionLookupKey) || !seen.add(key)) { - // The subgraph of dependencies of ActionLookupValues never has a non-ActionLookupValue - // depending on an ActionLookupValue. So we can skip any non-ActionLookupValues in the - // traversal as an optimization. + // The subgraph of dependencies of ActionLookupKeys never has a non-ActionLookupKey depending + // on an ActionLookupKey. So we can skip any non-ActionLookupKeys in the traversal as an + // optimization. return; } SkyValue value = walkableGraph.getValue(key); if (value == null) { return; // The value failed to evaluate. } - if (value instanceof ActionLookupValue) { - result.add((ActionLookupValue) value); - } + result.accumulate(value, (ActionLookupKey) key); for (SkyKey dep : walkableGraph.getDirectDeps(key)) { findActionsRecursively(walkableGraph, dep, seen, result); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/ActionGraphDump.java b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/ActionGraphDump.java index 05b46ba..3ec30db 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/ActionGraphDump.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/ActionGraphDump.java
@@ -36,6 +36,7 @@ import com.google.devtools.build.lib.packages.AspectDescriptor; import com.google.devtools.build.lib.query2.aquery.AqueryActionFilter; import com.google.devtools.build.lib.query2.aquery.AqueryUtils; +import com.google.devtools.build.lib.skyframe.RuleConfiguredTargetValue; import com.google.devtools.build.lib.util.Pair; import java.util.HashMap; import java.util.List; @@ -234,7 +235,7 @@ } } - public void dumpConfiguredTarget(ConfiguredTargetValue configuredTargetValue) + public void dumpConfiguredTarget(RuleConfiguredTargetValue configuredTargetValue) throws CommandLineExpansionException, InterruptedException { ConfiguredTarget configuredTarget = configuredTargetValue.getConfiguredTarget(); if (!includeInActionGraph(configuredTarget.getLabel().toString())) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java index f43bead..7dd45a1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java
@@ -36,6 +36,7 @@ import com.google.devtools.build.lib.packages.AspectDescriptor; import com.google.devtools.build.lib.query2.aquery.AqueryActionFilter; import com.google.devtools.build.lib.query2.aquery.AqueryUtils; +import com.google.devtools.build.lib.skyframe.RuleConfiguredTargetValue; import com.google.devtools.build.lib.util.Pair; import java.io.IOException; import java.util.HashMap; @@ -49,11 +50,8 @@ * proto format. */ public class ActionGraphDump { - private final ActionKeyContext actionKeyContext = new ActionKeyContext(); private final Set<String> actionGraphTargets; - - private final KnownRuleClassStrings knownRuleClassStrings; private final KnownArtifacts knownArtifacts; private final KnownConfigurations knownConfigurations; private final KnownNestedSets knownNestedSets; @@ -96,7 +94,7 @@ this.includeParamFiles = includeParamFiles; this.aqueryOutputHandler = aqueryOutputHandler; - knownRuleClassStrings = new KnownRuleClassStrings(aqueryOutputHandler); + KnownRuleClassStrings knownRuleClassStrings = new KnownRuleClassStrings(aqueryOutputHandler); knownArtifacts = new KnownArtifacts(aqueryOutputHandler); knownConfigurations = new KnownConfigurations(aqueryOutputHandler); knownNestedSets = new KnownNestedSets(aqueryOutputHandler, knownArtifacts); @@ -245,7 +243,7 @@ } } - public void dumpConfiguredTarget(ConfiguredTargetValue configuredTargetValue) + public void dumpConfiguredTarget(RuleConfiguredTargetValue configuredTargetValue) throws CommandLineExpansionException, InterruptedException, IOException { ConfiguredTarget configuredTarget = configuredTargetValue.getConfiguredTarget(); if (!includeInActionGraph(configuredTarget.getLabel().toString())) {