Skip to content

Commit

Permalink
Pass only the top-level artifacts to ExecutorLifecycleListener and …
Browse files Browse the repository at this point in the history 
…make the computation more efficient.

Delete the now unused `ArtifactsToOwnerLabels`.

PiperOrigin-RevId: 372152937
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed May 5, 2021
1 parent 1a7b977 commit cd477f2
Show file tree
Hide file tree
Showing 22 changed files with 160 additions and 405 deletions.
13 changes: 7 additions & 6 deletions src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionGraph;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.PackageRoots;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
Expand All @@ -37,7 +38,7 @@ public final class AnalysisResult {
private final ImmutableSet<ConfiguredTarget> targetsToSkip;
@Nullable private final FailureDetail failureDetail;
private final ActionGraph actionGraph;
private final ArtifactsToOwnerLabels topLevelArtifactsToOwnerLabels;
private final ImmutableSet<Artifact> artifactsToBuild;
private final ImmutableSet<ConfiguredTarget> parallelTests;
private final ImmutableSet<ConfiguredTarget> exclusiveTests;
@Nullable private final TopLevelArtifactContext topLevelContext;
Expand All @@ -55,7 +56,7 @@ public final class AnalysisResult {
ImmutableSet<ConfiguredTarget> targetsToSkip,
@Nullable FailureDetail failureDetail,
ActionGraph actionGraph,
ArtifactsToOwnerLabels topLevelArtifactsToOwnerLabels,
ImmutableSet<Artifact> artifactsToBuild,
ImmutableSet<ConfiguredTarget> parallelTests,
ImmutableSet<ConfiguredTarget> exclusiveTests,
TopLevelArtifactContext topLevelContext,
Expand All @@ -70,7 +71,7 @@ public final class AnalysisResult {
this.targetsToSkip = targetsToSkip;
this.failureDetail = failureDetail;
this.actionGraph = actionGraph;
this.topLevelArtifactsToOwnerLabels = topLevelArtifactsToOwnerLabels;
this.artifactsToBuild = artifactsToBuild;
this.parallelTests = parallelTests;
this.exclusiveTests = exclusiveTests;
this.topLevelContext = topLevelContext;
Expand Down Expand Up @@ -120,8 +121,8 @@ public ImmutableSet<ConfiguredTarget> getTargetsToSkip() {
return targetsToSkip;
}

public ArtifactsToOwnerLabels getTopLevelArtifactsToOwnerLabels() {
return topLevelArtifactsToOwnerLabels;
public ImmutableSet<Artifact> getArtifactsToBuild() {
return artifactsToBuild;
}

public ImmutableSet<ConfiguredTarget> getExclusiveTests() {
Expand Down Expand Up @@ -178,7 +179,7 @@ public AnalysisResult withExclusiveTestsAsParallelTests() {
targetsToSkip,
failureDetail,
actionGraph,
topLevelArtifactsToOwnerLabels,
artifactsToBuild,
Sets.union(parallelTests, exclusiveTests).immutableCopy(),
/*exclusiveTests=*/ ImmutableSet.of(),
topLevelContext,
Expand Down
125 changes: 0 additions & 125 deletions src/main/java/com/google/devtools/build/lib/analysis/ArtifactsToOwnerLabels.java
View file

This file was deleted.

13 changes: 0 additions & 13 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ java_library(
":analysis_options",
":analysis_phase_complete_event",
":analysis_phase_started_event",
":artifacts_to_owner_labels",
":aspect_aware_attribute_mapper",
":aspect_collection",
":aspect_configured_event",
Expand Down Expand Up @@ -282,7 +281,6 @@ java_library(
":actions/symlink_action",
":actions/template_expansion_action",
":actions_provider",
":artifacts_to_owner_labels",
":aspect_aware_attribute_mapper",
":aspect_collection",
":build_setting_provider",
Expand Down Expand Up @@ -497,16 +495,6 @@ java_library(
],
)

java_library(
name = "artifacts_to_owner_labels",
srcs = ["ArtifactsToOwnerLabels.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//third_party:guava",
],
)

java_library(
name = "aspect_aware_attribute_mapper",
srcs = ["AspectAwareAttributeMapper.java"],
Expand Down Expand Up @@ -599,7 +587,6 @@ java_library(
":analysis_cluster",
":analysis_options",
":analysis_phase_started_event",
":artifacts_to_owner_labels",
":aspect_configured_event",
":blaze_directories",
":config/build_configuration",
Expand Down
80 changes: 36 additions & 44 deletions src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -470,8 +470,7 @@ private AnalysisResult createResult(
allTargetsToTest = filterTestsByTargets(configuredTargets, testsToRun);
}

ArtifactsToOwnerLabels.Builder artifactsToOwnerLabelsBuilder =
new ArtifactsToOwnerLabels.Builder();
ImmutableSet.Builder<Artifact> artifactsToBuild = ImmutableSet.builder();

// build-info and build-changelist.
Collection<Artifact> buildInfoArtifacts =
Expand All @@ -480,11 +479,11 @@ private AnalysisResult createResult(

// Extra actions
addExtraActionsIfRequested(
viewOptions, configuredTargets, aspects, artifactsToOwnerLabelsBuilder, eventHandler);
viewOptions, configuredTargets, aspects, artifactsToBuild, eventHandler);

// Coverage
NestedSet<Artifact> baselineCoverageArtifacts =
getBaselineCoverageArtifacts(configuredTargets, artifactsToOwnerLabelsBuilder);
getBaselineCoverageArtifacts(configuredTargets, artifactsToBuild);
if (coverageReportActionFactory != null) {
CoverageReportActionsWrapper actionsWrapper;
actionsWrapper =
Expand All @@ -501,23 +500,24 @@ private AnalysisResult createResult(
if (actionsWrapper != null) {
Actions.GeneratingActions actions = actionsWrapper.getActions();
skyframeExecutor.injectCoverageReportData(actions);
actionsWrapper.getCoverageOutputs().forEach(artifactsToOwnerLabelsBuilder::addArtifact);
actionsWrapper.getCoverageOutputs().forEach(artifactsToBuild::add);
}
}
// TODO(cparsons): If extra actions are ever removed, this filtering step can probably be
// removed as well: the only concern would be action conflicts involving coverage artifacts,
// which seems far-fetched.
if (skyframeAnalysisResult.hasActionConflicts()) {
ArtifactsToOwnerLabels tempOwners = artifactsToOwnerLabelsBuilder.build();
// We don't remove the (hopefully unnecessary) guard in SkyframeBuildView that enables/
// disables analysis, since no new targets should actually be analyzed.
Set<Artifact> artifacts = tempOwners.getArtifacts();
ImmutableSet<Artifact> artifacts = artifactsToBuild.build();
Predicate<Artifact> errorFreeArtifacts =
skyframeExecutor.filterActionConflictsForTopLevelArtifacts(eventHandler, artifacts);
artifactsToOwnerLabelsBuilder = tempOwners.toBuilder().filterArtifacts(errorFreeArtifacts);

artifactsToBuild = ImmutableSet.builder();
artifacts.stream().filter(errorFreeArtifacts).forEach(artifactsToBuild::add);
}
// Build-info artifacts are always conflict-free, and can't be checked easily.
buildInfoArtifacts.forEach(artifactsToOwnerLabelsBuilder::addArtifact);
buildInfoArtifacts.forEach(artifactsToBuild::add);

// Tests.
Pair<ImmutableSet<ConfiguredTarget>, ImmutableSet<ConfiguredTarget>> testsPair =
Expand All @@ -529,8 +529,8 @@ private AnalysisResult createResult(
FailureDetail failureDetail =
createFailureDetail(loadingResult, skyframeAnalysisResult, topLevelTargetsWithConfigs);

final WalkableGraph graph = skyframeAnalysisResult.getWalkableGraph();
final ActionGraph actionGraph =
WalkableGraph graph = skyframeAnalysisResult.getWalkableGraph();
ActionGraph actionGraph =
new ActionGraph() {
@Nullable
@Override
Expand Down Expand Up @@ -563,7 +563,7 @@ public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) {
ImmutableSet.copyOf(targetsToSkip),
failureDetail,
actionGraph,
artifactsToOwnerLabelsBuilder.build(),
artifactsToBuild.build(),
parallelTests,
exclusiveTests,
topLevelOptions,
Expand Down Expand Up @@ -623,16 +623,12 @@ private static FailureDetail createFailureDetail(String errorMessage, Analysis.C

private static NestedSet<Artifact> getBaselineCoverageArtifacts(
Collection<ConfiguredTarget> configuredTargets,
ArtifactsToOwnerLabels.Builder topLevelArtifactsToOwnerLabels) {
ImmutableSet.Builder<Artifact> artifactsToBuild) {
NestedSetBuilder<Artifact> baselineCoverageArtifacts = NestedSetBuilder.stableOrder();
for (ConfiguredTarget target : configuredTargets) {
InstrumentedFilesInfo provider = target.get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
if (provider != null) {
TopLevelArtifactHelper.addArtifactsWithOwnerLabel(
provider.getBaselineCoverageArtifacts(),
null,
target.getLabel(),
topLevelArtifactsToOwnerLabels);
artifactsToBuild.addAll(provider.getBaselineCoverageArtifacts().toList());
baselineCoverageArtifacts.addTransitive(provider.getBaselineCoverageArtifacts());
}
}
Expand All @@ -643,7 +639,7 @@ private void addExtraActionsIfRequested(
AnalysisOptions viewOptions,
Collection<ConfiguredTarget> configuredTargets,
ImmutableMap<AspectKey, ConfiguredAspect> aspects,
ArtifactsToOwnerLabels.Builder artifactsToTopLevelLabelsMap,
ImmutableSet.Builder<Artifact> artifactsToBuild,
ExtendedEventHandler eventHandler) {
RegexFilter filter = viewOptions.extraActionFilter;
for (ConfiguredTarget target : configuredTargets) {
Expand All @@ -663,24 +659,15 @@ private void addExtraActionsIfRequested(
for (Attribute attr : actualTarget.getAssociatedRule().getAttributes()) {
aspectClasses.addAll(attr.getAspectClasses());
}
TopLevelArtifactHelper.addArtifactsWithOwnerLabel(
provider.getExtraActionArtifacts(),
filter,
target.getLabel(),
artifactsToTopLevelLabelsMap);
addArtifactsToBuilder(
provider.getExtraActionArtifacts().toList(), artifactsToBuild, filter);
if (!aspectClasses.isEmpty()) {
TopLevelArtifactHelper.addArtifactsWithOwnerLabel(
filterTransitiveExtraActions(provider, aspectClasses),
filter,
target.getLabel(),
artifactsToTopLevelLabelsMap);
addArtifactsToBuilder(
filterTransitiveExtraActions(provider, aspectClasses), artifactsToBuild, filter);
}
} else {
TopLevelArtifactHelper.addArtifactsWithOwnerLabel(
provider.getTransitiveExtraActionArtifacts(),
filter,
target.getLabel(),
artifactsToTopLevelLabelsMap);
addArtifactsToBuilder(
provider.getTransitiveExtraActionArtifacts().toList(), artifactsToBuild, filter);
}
}
}
Expand All @@ -689,22 +676,27 @@ private void addExtraActionsIfRequested(
aspectEntry.getValue().getProvider(ExtraActionArtifactsProvider.class);
if (provider != null) {
if (viewOptions.extraActionTopLevelOnly) {
TopLevelArtifactHelper.addArtifactsWithOwnerLabel(
provider.getExtraActionArtifacts(),
filter,
aspectEntry.getKey().getLabel(),
artifactsToTopLevelLabelsMap);
addArtifactsToBuilder(
provider.getExtraActionArtifacts().toList(), artifactsToBuild, filter);
} else {
TopLevelArtifactHelper.addArtifactsWithOwnerLabel(
provider.getTransitiveExtraActionArtifacts(),
filter,
aspectEntry.getKey().getLabel(),
artifactsToTopLevelLabelsMap);
addArtifactsToBuilder(
provider.getTransitiveExtraActionArtifacts().toList(), artifactsToBuild, filter);
}
}
}
}

private static void addArtifactsToBuilder(
List<? extends Artifact> artifacts,
ImmutableSet.Builder<Artifact> builder,
RegexFilter filter) {
for (Artifact artifact : artifacts) {
if (filter.isIncluded(artifact.getOwnerLabel().toString())) {
builder.add(artifact);
}
}
}

/**
* Returns a list of artifacts from 'provider' that were registered by an aspect from
* 'aspectClasses'. All artifacts in 'provider' are considered - both direct and transitive.
Expand Down
Loading

0 comments on commit cd477f2

Please sign in to comment.