Try to clean up some of the excess baggage Aspects are carting around, mainly from a code cleanliness perspective, but with some minor memory benefits:
1. Use AspectKey instead of AspectValue wherever possible at the top-level, since keys are what callers should be using, not values.
2. Remove the freestanding label field from AspectKey. It is always equal to its ConfiguredTargetKey field's label. Having two fields with redundant data is confusing.
3. Remove the freestanding label field from AspectValue. Same reason: it was always the AspectKey's label.
4. Remove AspectDescriptor from ConfiguredAspect: it comes from the AspectKey, so just use the key.
This is also good because I think maximizing correspondence between ConfiguredAspect and ConfiguredTarget is a goal. Having an AspectValue muddies the waters. Note that, for historical reasons, ConfiguredTarget has much more data in it than we'd want a ConfiguredAspect to have (ideally, both would just have actions and providers).
In a follow-up, I may wrap AspectValue together with an AspectKey in TopLevelSkylarkAspectFunction#compute's return value. I think that, plus some tolerable casting, would remove the only remaining real need for AspectValue to have an AspectKey inside it.
PiperOrigin-RevId: 307877866
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
index 901c8fc..dafb679 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -217,8 +217,7 @@
update(defaultFlags().with(Flag.KEEP_GOING), "//conflict:x", "//conflict:_objs/x/foo.o");
// We want to force a "dropConfiguredTargetsNow" operation, which won't inform the
// invalidation receiver about the dropped configured targets.
- skyframeExecutor.clearAnalysisCache(
- ImmutableList.<ConfiguredTarget>of(), ImmutableSet.<AspectValue>of());
+ skyframeExecutor.clearAnalysisCache(ImmutableList.of(), ImmutableSet.of());
assertContainsEvent("file 'conflict/_objs/x/foo.o' " + CONFLICT_MSG);
eventCollector.clear();
scratch.overwriteFile(
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
index 51e5225..60164a6 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -432,7 +432,7 @@
String toolsRepository)
throws InterruptedException, ActionConflictException {
Object lateBoundPrereq = ruleContext.getPrerequisite(":late", TARGET);
- return new ConfiguredAspect.Builder(this, parameters, ruleContext)
+ return new ConfiguredAspect.Builder(ruleContext)
.addProvider(
AspectInfo.class,
new AspectInfo(
@@ -505,7 +505,7 @@
String toolsRepository)
throws InterruptedException, ActionConflictException {
ruleContext.registerAction(new NullAction(ruleContext.createOutputArtifact()));
- return new ConfiguredAspect.Builder(this, parameters, ruleContext).build();
+ return new ConfiguredAspect.Builder(ruleContext).build();
}
}
}
@@ -832,9 +832,9 @@
AnalysisResult analysisResult = update(new EventBus(), defaultFlags(),
ImmutableList.of(aspectApplyingToFiles.getName()),
"//a:x_deploy.jar");
- AspectValue aspect = Iterables.getOnlyElement(analysisResult.getAspects());
+ ConfiguredAspect aspect = Iterables.getOnlyElement(analysisResult.getAspectsMap().values());
AspectApplyingToFiles.Provider provider =
- aspect.getConfiguredAspect().getProvider(AspectApplyingToFiles.Provider.class);
+ aspect.getProvider(AspectApplyingToFiles.Provider.class);
assertThat(provider.getLabel())
.isEqualTo(Label.parseAbsoluteUnchecked("//a:x_deploy.jar"));
}
@@ -853,9 +853,8 @@
AnalysisResult analysisResult = update(new EventBus(), defaultFlags(),
ImmutableList.of(aspectApplyingToFiles.getName()),
"//a:x.java");
- AspectValue aspect = Iterables.getOnlyElement(analysisResult.getAspects());
- assertThat(aspect.getConfiguredAspect().getProvider(AspectApplyingToFiles.Provider.class))
- .isNull();
+ ConfiguredAspect aspect = Iterables.getOnlyElement(analysisResult.getAspectsMap().values());
+ assertThat(aspect.getProvider(AspectApplyingToFiles.Provider.class)).isNull();
}
@Test
@@ -871,9 +870,9 @@
defaultFlags(),
ImmutableList.of(aspectApplyingToFiles.getName(), aspectApplyingToFiles.getName()),
"//a:x_deploy.jar");
- AspectValue aspect = Iterables.getOnlyElement(analysisResult.getAspects());
+ ConfiguredAspect aspect = Iterables.getOnlyElement(analysisResult.getAspectsMap().values());
AspectApplyingToFiles.Provider provider =
- aspect.getConfiguredAspect().getProvider(AspectApplyingToFiles.Provider.class);
+ aspect.getProvider(AspectApplyingToFiles.Provider.class);
assertThat(provider.getLabel()).isEqualTo(Label.parseAbsoluteUnchecked("//a:x_deploy.jar"));
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 274c558..209ad60 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -404,7 +404,8 @@
reporter,
eventBus);
if (discardAnalysisCache) {
- buildView.clearAnalysisCache(analysisResult.getTargetsToBuild(), analysisResult.getAspects());
+ buildView.clearAnalysisCache(
+ analysisResult.getTargetsToBuild(), analysisResult.getAspectsMap().keySet());
}
masterConfig = analysisResult.getConfigurationCollection();
return analysisResult;
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index cdc67d4..972d757 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -28,7 +28,6 @@
import com.google.devtools.build.lib.analysis.AnalysisOptions;
import com.google.devtools.build.lib.analysis.AnalysisResult;
import com.google.devtools.build.lib.analysis.AnalysisUtils;
-import com.google.devtools.build.lib.analysis.AspectValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.BuildView;
import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
@@ -77,6 +76,7 @@
import com.google.devtools.build.lib.packages.RawAttributeMapper;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
@@ -613,8 +613,9 @@
}
/** Clears the analysis cache as in --discard_analysis_cache. */
- public void clearAnalysisCache(
- Collection<ConfiguredTarget> topLevelTargets, Collection<AspectValue> topLevelAspects) {
+ @VisibleForTesting
+ void clearAnalysisCache(
+ Collection<ConfiguredTarget> topLevelTargets, ImmutableSet<AspectKey> topLevelAspects) {
skyframeBuildView.clearAnalysisCache(topLevelTargets, topLevelAspects);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
index e4f25da..3177a5e 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
@@ -239,7 +239,7 @@
String information = parameters.isEmpty()
? ""
: " data " + Iterables.getFirst(parameters.getAttribute("baz"), null);
- return new ConfiguredAspect.Builder(this, parameters, ruleContext)
+ return new ConfiguredAspect.Builder(ruleContext)
.addProvider(
new AspectInfo(
collectAspectData("aspect " + ruleContext.getLabel() + information, ruleContext)))
@@ -286,9 +286,7 @@
AspectParameters parameters,
String toolsRepository)
throws ActionConflictException {
- return new ConfiguredAspect.Builder(this, parameters, ruleContext)
- .addProvider(new FooProvider())
- .build();
+ return new ConfiguredAspect.Builder(ruleContext).addProvider(new FooProvider()).build();
}
}
@@ -309,9 +307,7 @@
AspectParameters parameters,
String toolsRepository)
throws ActionConflictException {
- return new ConfiguredAspect.Builder(this, parameters, ruleContext)
- .addProvider(new BarProvider())
- .build();
+ return new ConfiguredAspect.Builder(ruleContext).addProvider(new BarProvider()).build();
}
}
@@ -463,7 +459,7 @@
information.append(dep.getLabel());
}
information.append("]");
- return new ConfiguredAspect.Builder(this, parameters, ruleContext)
+ return new ConfiguredAspect.Builder(ruleContext)
.addProvider(new AspectInfo(collectAspectData(information.toString(), ruleContext)))
.build();
}
@@ -502,7 +498,7 @@
String toolsRepository)
throws ActionConflictException {
ruleContext.ruleWarning("Aspect warning on " + ctadBase.getTarget().getLabel());
- return new ConfiguredAspect.Builder(this, parameters, ruleContext).build();
+ return new ConfiguredAspect.Builder(ruleContext).build();
}
@Override
@@ -563,7 +559,7 @@
AspectParameters parameters,
String toolsRepository)
throws InterruptedException, ActionConflictException {
- return new ConfiguredAspect.Builder(this, parameters, context).build();
+ return new ConfiguredAspect.Builder(context).build();
}
}
public static final FalseAdvertisementAspect FALSE_ADVERTISEMENT_ASPECT
@@ -812,7 +808,7 @@
AspectParameters parameters,
String toolsRepository)
throws InterruptedException, ActionConflictException {
- return ConfiguredAspect.builder(this, parameters, context)
+ return ConfiguredAspect.builder(context)
.addProvider(Provider.class, new Provider(ctadBase.getConfiguredTarget().getLabel()))
.build();
}