Skip to content

Commit

Permalink
Fix counting created actions to not over count aspect actions on alia…
Browse files Browse the repository at this point in the history
…s targets

if an aspect is applied on an alias, the resultant `AspectValue` holds the `ConfiguredAspect` of the aspect on the alias's actual target. This results in counting the actions created by the aspect multiple times, one time per target in the alias chain. This CL adds a new class `AspectValueForAlias` to distinguish this case and updates the action counting logic to ignore actions for this `SkyValue`.

PiperOrigin-RevId: 678288884
Change-Id: I9428eac955d2026682e5fb93e2ec62d108bb2fd7
  • Loading branch information
mai93 authored and copybara-github committed Sep 24, 2024
1 parent c1734aa commit e69dd3c
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,17 @@ public static AspectValue create(
: new AspectValueWithTransitivePackages(key, aspect, configuredAspect, transitivePackages);
}

public static AspectValue createForAlias(
AspectKey key,
Aspect aspect,
ConfiguredAspect configuredAspect,
@Nullable NestedSet<Package> transitivePackages) {
return transitivePackages == null
? new AspectValueForAlias(aspect, configuredAspect)
: new AspectValueWithTransitivePackagesForAlias(
key, aspect, configuredAspect, transitivePackages);
}

// 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;
Expand Down Expand Up @@ -101,7 +112,7 @@ public String toString() {
return getStringHelper().toString();
}

private static final class AspectValueWithTransitivePackages extends AspectValue {
private static class AspectValueWithTransitivePackages extends AspectValue {
@Nullable private transient NestedSet<Package> transitivePackages; // Null after clear().
@Nullable private AspectKey key;

Expand Down Expand Up @@ -137,4 +148,26 @@ protected ToStringHelper getStringHelper() {
return super.getStringHelper().add("key", key).add("transitivePackages", transitivePackages);
}
}

private static final class AspectValueForAlias extends AspectValue {
private AspectValueForAlias(Aspect aspect, ConfiguredAspect configuredAspect) {
super(aspect, configuredAspect);
}
}

private static final class AspectValueWithTransitivePackagesForAlias
extends AspectValueWithTransitivePackages {
private AspectValueWithTransitivePackagesForAlias(
AspectKey key,
Aspect aspect,
ConfiguredAspect configuredAspect,
NestedSet<Package> transitivePackages) {
super(key, aspect, configuredAspect, transitivePackages);
}
}

public static boolean isForAliasTarget(AspectValue aspectValue) {
return aspectValue instanceof AspectValueForAlias
|| aspectValue instanceof AspectValueWithTransitivePackagesForAlias;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ private AspectValue createAliasAspect(
.addTransitive(real.getTransitivePackages())
.build()
: null;
return AspectValue.create(
return AspectValue.createForAlias(
originalKey, aspect, ConfiguredAspect.forAlias(real), transitivePackages);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import com.google.devtools.build.lib.analysis.AnalysisFailureEvent;
import com.google.devtools.build.lib.analysis.AnalysisOperationWatcher;
import com.google.devtools.build.lib.analysis.AnalysisPhaseCompleteEvent;
import com.google.devtools.build.lib.analysis.AspectValue;
import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
Expand Down Expand Up @@ -1506,6 +1507,12 @@ public void evaluated(
}
configuredObjectCount.incrementAndGet();
if (newValue instanceof ActionLookupValue alv) {
if (alv instanceof AspectValue) {
if (AspectValue.isForAliasTarget((AspectValue) alv)) {
// Created actions will be counted from {@link AspectValue} on the original target.
return;
}
}
// During multithreaded operation, this is only set to true, so no concurrency issues.
someActionLookupValueEvaluated = true;
ImmutableList<ActionAnalysisMetadata> actions = alv.getActions();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,54 @@ def _impl(ctx):
.isEqualTo(buildMetrics.getActionSummary().getActionsExecuted());
}

@Test
public void testAspectActionsCreatedNotOverCountedForAlias() throws Exception {
write(
"pkg/BUILD",
"""
load(":defs.bzl", "my_rule")
my_rule(name = "foo", dep = "dep_alias_alias")
alias(name = "dep_alias_alias", actual = ":dep_alias")
alias(name = "dep_alias", actual = ":dep")
my_rule(name = "dep")
""");
write(
"pkg/defs.bzl",
"""
def _aspect_impl(target, ctx):
f = ctx.actions.declare_file(target.label.name + ".out")
ctx.actions.run_shell(
outputs = [f],
command = "touch $@",
mnemonic = "MyAspectAction",
)
return [OutputGroupInfo(my_out = depset([f]))]
my_aspect = aspect(implementation = _aspect_impl)
def _impl(ctx):
pass
my_rule = rule(
implementation = _impl,
attrs = {
"dep": attr.label(aspects = [my_aspect]),
},
)
""");
buildTarget("//pkg:foo");

BuildMetrics buildMetrics = buildMetricsEventListener.event.getBuildMetrics();
List<ActionData> actionData = buildMetrics.getActionSummary().getActionDataList();
ImmutableList<ActionData> aspectActions =
actionData.stream()
.filter(a -> a.getMnemonic().equals("MyAspectAction"))
.collect(toImmutableList());
assertThat(aspectActions).hasSize(1);
ActionData aspectAction = aspectActions.get(0);
assertThat(aspectAction.getActionsCreated()).isEqualTo(1);
}

@Test
public void buildGraphAndArtifactMetrics() throws Exception {
write(
Expand Down

0 comments on commit e69dd3c

Please sign in to comment.