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/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
index 77ee261..bfd5266 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValueKey.java
@@ -51,7 +51,6 @@
BuildConfiguration aspectConfiguration) {
KeyAndHost aspectKeyAndHost = ConfiguredTargetKey.keyFromConfiguration(aspectConfiguration);
return AspectKey.createAspectKey(
- label,
ConfiguredTargetKey.of(label, baseConfiguration),
baseKeys,
aspectDescriptor,
@@ -66,7 +65,6 @@
BuildConfiguration aspectConfiguration) {
KeyAndHost aspectKeyAndHost = ConfiguredTargetKey.keyFromConfiguration(aspectConfiguration);
return AspectKey.createAspectKey(
- label,
ConfiguredTargetKey.of(label, baseConfiguration),
ImmutableList.of(),
aspectDescriptor,
@@ -104,7 +102,6 @@
/** A base class for a key representing an aspect applied to a particular target. */
@AutoCodec
public static class AspectKey extends AspectValueKey {
- private final Label label;
private final ImmutableList<AspectKey> baseKeys;
private final BuildConfigurationValue.Key aspectConfigurationKey;
private final ConfiguredTargetKey baseConfiguredTargetKey;
@@ -112,13 +109,11 @@
private int hashCode;
private AspectKey(
- Label label,
BuildConfigurationValue.Key aspectConfigurationKey,
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor) {
this.baseKeys = baseKeys;
- this.label = label;
this.aspectConfigurationKey = aspectConfigurationKey;
this.baseConfiguredTargetKey = baseConfiguredTargetKey;
this.aspectDescriptor = aspectDescriptor;
@@ -127,7 +122,6 @@
@AutoCodec.VisibleForSerialization
@AutoCodec.Instantiator
static AspectKey createAspectKey(
- Label label,
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor,
@@ -136,17 +130,9 @@
return aspectKeyInterner.intern(
aspectConfigurationIsHost
? new HostAspectKey(
- label,
- aspectConfigurationKey,
- baseConfiguredTargetKey,
- baseKeys,
- aspectDescriptor)
+ aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor)
: new AspectKey(
- label,
- aspectConfigurationKey,
- baseConfiguredTargetKey,
- baseKeys,
- aspectDescriptor));
+ aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor));
}
@Override
@@ -157,7 +143,7 @@
@Override
public Label getLabel() {
- return label;
+ return baseConfiguredTargetKey.getLabel();
}
public AspectClass getAspectClass() {
@@ -249,7 +235,7 @@
private int computeHashCode() {
return Objects.hashCode(
- label, baseKeys, aspectConfigurationKey, baseConfiguredTargetKey, aspectDescriptor);
+ baseKeys, aspectConfigurationKey, baseConfiguredTargetKey, aspectDescriptor);
}
@Override
@@ -263,15 +249,14 @@
}
AspectKey that = (AspectKey) other;
- return Objects.equal(label, that.label)
- && Objects.equal(baseKeys, that.baseKeys)
+ return Objects.equal(baseKeys, that.baseKeys)
&& Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey)
&& Objects.equal(baseConfiguredTargetKey, that.baseConfiguredTargetKey)
&& Objects.equal(aspectDescriptor, that.aspectDescriptor);
}
public String prettyPrint() {
- if (label == null) {
+ if (getLabel() == null) {
return "null";
}
@@ -281,7 +266,7 @@
: String.format(" (over %s)", baseKeys.toString());
return String.format(
"%s with aspect %s%s%s",
- label.toString(),
+ getLabel().toString(),
aspectDescriptor.getAspectClass().getName(),
(aspectConfigurationKey != null && aspectConfigurationIsHost()) ? "(host) " : "",
baseKeysString);
@@ -289,7 +274,7 @@
@Override
public String toString() {
- return (baseKeys == null ? label : baseKeys.toString())
+ return (baseKeys == null ? getLabel() : baseKeys.toString())
+ "#"
+ aspectDescriptor
+ " "
@@ -308,7 +293,6 @@
}
return createAspectKey(
- label,
ConfiguredTargetKey.of(
label,
baseConfiguredTargetKey.getConfigurationKey(),
@@ -323,12 +307,11 @@
/** An {@link AspectKey} for an aspect in the host configuration. */
static class HostAspectKey extends AspectKey {
private HostAspectKey(
- Label label,
BuildConfigurationValue.Key aspectConfigurationKey,
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor) {
- super(label, aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor);
+ super(aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor);
}
@Override
@@ -453,7 +436,6 @@
AspectKey toAspectKey(AspectClass aspectClass) {
return AspectKey.createAspectKey(
- targetLabel,
baseConfiguredTargetKey,
ImmutableList.of(),
new AspectDescriptor(aspectClass, AspectParameters.EMPTY),
