Roll forward of https://github.com/bazelbuild/bazel/commit/943c83aa58731c4f9561d79c458f254427a8f24c: Command line aspect-on-aspect
Supports aspect-on-aspect for command line aspects. Command line aspects specified via `--aspects` option will support a top-level aspect requiring aspect providers via `required_aspect_providers` to get their values from other top-level aspects advertising it that come before it in the `--aspects` list.
NEW:
- Store only the list of AspectValue in TopLevelAspectsValue and not a map from AspectKey to AspectValue
- Introduce `BuildTopLevelAspectsDetailsFunction` sky function to be responsible for computing the dependency between the top-level aspects based on `required_aspect_providers` and `provides` or based on `requires` attribute. Since the dependency between the aspects will be the same for all top level targets this function will only compute it once.
Automated rollback of commit b27fd22f1bd1e29ec2475a3935c9004cc14713bf.
*** Reason for rollback ***
Roll forward, optimize the solution for top-level aspect-on-aspect by storing only a list of AspectValues in TopLevelAspectsValue and introducing `BuildTopLevelAspectsDetailsFunction` to compute the dependency between the top-level aspects only once for all top-level targets.
*** Original change description ***
Automated rollback of commit 943c83aa58731c4f9561d79c458f254427a8f24c.
*** Reason for rollback ***
This CL causes memory regression for single top-level aspect described in b/193314770
*** Original change description ***
Command line aspect-on-aspect
This CL supports aspect-on-aspect for command line aspects. Command line aspects specified via `--aspects` option will support a top-level aspect requiring aspect providers via `required_aspect_providers` to get their values from other top-level aspects advertising it...
***
PiperOrigin-RevId: 387052900
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 04c15bd..f97e08d 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
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.base.MoreObjects;
import com.google.common.base.Objects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
@@ -30,29 +29,15 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import javax.annotation.Nullable;
-/** A base class for keys that have AspectValue as a Sky value. */
-public abstract class AspectValueKey implements ActionLookupKey {
+/** A wrapper class for sky keys needed to compute sky values for aspects. */
+public final class AspectValueKey {
+
+ private AspectValueKey() {}
private static final Interner<AspectKey> aspectKeyInterner = BlazeInterners.newWeakInterner();
- private static final Interner<StarlarkAspectLoadingKey> starlarkAspectKeyInterner =
+ private static final Interner<TopLevelAspectsKey> topLevelAspectsKeyInterner =
BlazeInterners.newWeakInterner();
- /**
- * Gets the name of the aspect that would be returned by the corresponding value's {@code
- * aspectValue.getAspect().getAspectClass().getName()}, if the value could be produced.
- *
- * <p>Only needed for reporting errors in BEP when the key's AspectValue fails evaluation.
- */
- public abstract String getAspectName();
-
- public abstract String getDescription();
-
- @Nullable
- abstract BuildConfigurationValue.Key getAspectConfigurationKey();
-
- /** Returns the key for the base configured target for this aspect. */
- public abstract ConfiguredTargetKey getBaseConfiguredTargetKey();
-
public static AspectKey createAspectKey(
Label label,
@Nullable BuildConfiguration baseConfiguration,
@@ -87,33 +72,48 @@
aspectConfiguration == null ? null : BuildConfigurationValue.key(aspectConfiguration));
}
- public static StarlarkAspectLoadingKey createStarlarkAspectKey(
+ public static TopLevelAspectsKey createTopLevelAspectsKey(
+ ImmutableList<AspectClass> topLevelAspectsClasses,
Label targetLabel,
- @Nullable BuildConfiguration aspectConfiguration,
- @Nullable BuildConfiguration targetConfiguration,
- Label starlarkFileLabel,
- String starlarkExportName) {
- return StarlarkAspectLoadingKey.createInternal(
+ @Nullable BuildConfiguration configuration) {
+ return TopLevelAspectsKey.createInternal(
+ topLevelAspectsClasses,
targetLabel,
- aspectConfiguration == null ? null : BuildConfigurationValue.key(aspectConfiguration),
ConfiguredTargetKey.builder()
.setLabel(targetLabel)
- .setConfiguration(targetConfiguration)
- .build(),
- starlarkFileLabel,
- starlarkExportName);
+ .setConfiguration(configuration)
+ .build());
+ }
+
+ /** Common superclass for {@link AspectKey} and {@link TopLevelAspectsKey}. */
+ public abstract static class AspectBaseKey implements ActionLookupKey {
+ private final ConfiguredTargetKey baseConfiguredTargetKey;
+ private final int hashCode;
+
+ private AspectBaseKey(ConfiguredTargetKey baseConfiguredTargetKey, int hashCode) {
+ this.baseConfiguredTargetKey = baseConfiguredTargetKey;
+ this.hashCode = hashCode;
+ }
+
+ /** Returns the key for the base configured target for this aspect. */
+ public final ConfiguredTargetKey getBaseConfiguredTargetKey() {
+ return baseConfiguredTargetKey;
+ }
+
+ @Override
+ public final int hashCode() {
+ return hashCode;
+ }
}
// Specific subtypes of aspect keys.
/** Represents an aspect applied to a particular target. */
@AutoCodec
- public static final class AspectKey extends AspectValueKey {
- private final ConfiguredTargetKey baseConfiguredTargetKey;
+ public static final class AspectKey extends AspectBaseKey {
private final ImmutableList<AspectKey> baseKeys;
@Nullable private final BuildConfigurationValue.Key aspectConfigurationKey;
private final AspectDescriptor aspectDescriptor;
- private final int hashCode;
private AspectKey(
ConfiguredTargetKey baseConfiguredTargetKey,
@@ -121,11 +121,10 @@
AspectDescriptor aspectDescriptor,
@Nullable BuildConfigurationValue.Key aspectConfigurationKey,
int hashCode) {
+ super(baseConfiguredTargetKey, hashCode);
this.baseKeys = baseKeys;
this.aspectConfigurationKey = aspectConfigurationKey;
- this.baseConfiguredTargetKey = baseConfiguredTargetKey;
this.aspectDescriptor = aspectDescriptor;
- this.hashCode = hashCode;
}
@AutoCodec.VisibleForSerialization
@@ -150,14 +149,19 @@
return SkyFunctions.ASPECT;
}
- @Override
+ /**
+ * Gets the name of the aspect that would be returned by the corresponding value's {@code
+ * aspectValue.getAspect().getAspectClass().getName()}, if the value could be produced.
+ *
+ * <p>Only needed for reporting errors in BEP when the key's AspectValue fails evaluation.
+ */
public String getAspectName() {
return aspectDescriptor.getDescription();
}
@Override
public Label getLabel() {
- return baseConfiguredTargetKey.getLabel();
+ return getBaseConfiguredTargetKey().getLabel();
}
public AspectClass getAspectClass() {
@@ -187,11 +191,9 @@
return baseKeys;
}
- @Override
public String getDescription() {
if (baseKeys.isEmpty()) {
- return String.format("%s of %s",
- aspectDescriptor.getAspectClass().getName(), getLabel());
+ return String.format("%s of %s", aspectDescriptor.getAspectClass().getName(), getLabel());
} else {
return String.format(
"%s on top of %s", aspectDescriptor.getAspectClass().getName(), baseKeys);
@@ -216,22 +218,10 @@
* base target's configuration.
*/
@Nullable
- @Override
BuildConfigurationValue.Key getAspectConfigurationKey() {
return aspectConfigurationKey;
}
- /** Returns the key for the base configured target for this aspect. */
- @Override
- public ConfiguredTargetKey getBaseConfiguredTargetKey() {
- return baseConfiguredTargetKey;
- }
-
- @Override
- public int hashCode() {
- return hashCode;
- }
-
@Override
public boolean equals(Object other) {
if (this == other) {
@@ -241,10 +231,10 @@
return false;
}
AspectKey that = (AspectKey) other;
- return hashCode == that.hashCode
+ return hashCode() == that.hashCode()
&& Objects.equal(baseKeys, that.baseKeys)
&& Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey)
- && Objects.equal(baseConfiguredTargetKey, that.baseConfiguredTargetKey)
+ && Objects.equal(getBaseConfiguredTargetKey(), that.getBaseConfiguredTargetKey())
&& Objects.equal(aspectDescriptor, that.aspectDescriptor);
}
@@ -267,7 +257,7 @@
+ " "
+ aspectConfigurationKey
+ " "
- + baseConfiguredTargetKey
+ + getBaseConfiguredTargetKey()
+ " "
+ aspectDescriptor.getParameters();
}
@@ -281,7 +271,7 @@
return createAspectKey(
ConfiguredTargetKey.builder()
.setLabel(label)
- .setConfigurationKey(baseConfiguredTargetKey.getConfigurationKey())
+ .setConfigurationKey(getBaseConfiguredTargetKey().getConfigurationKey())
.build(),
newBaseKeys.build(),
aspectDescriptor,
@@ -289,70 +279,43 @@
}
}
- /** The key for a Starlark aspect. */
+ /** The key for top level aspects specified by --aspects option on a top level target. */
@AutoCodec
- public static final class StarlarkAspectLoadingKey extends AspectValueKey {
+ public static final class TopLevelAspectsKey extends AspectBaseKey {
+ private final ImmutableList<AspectClass> topLevelAspectsClasses;
private final Label targetLabel;
- private final BuildConfigurationValue.Key aspectConfigurationKey;
- private final ConfiguredTargetKey baseConfiguredTargetKey;
- private final Label starlarkFileLabel;
- private final String starlarkValueName;
- private final int hashCode;
@AutoCodec.Instantiator
@AutoCodec.VisibleForSerialization
- static StarlarkAspectLoadingKey createInternal(
+ static TopLevelAspectsKey createInternal(
+ ImmutableList<AspectClass> topLevelAspectsClasses,
Label targetLabel,
- BuildConfigurationValue.Key aspectConfigurationKey,
- ConfiguredTargetKey baseConfiguredTargetKey,
- Label starlarkFileLabel,
- String starlarkValueName) {
- return starlarkAspectKeyInterner.intern(
- new StarlarkAspectLoadingKey(
+ ConfiguredTargetKey baseConfiguredTargetKey) {
+ return topLevelAspectsKeyInterner.intern(
+ new TopLevelAspectsKey(
+ topLevelAspectsClasses,
targetLabel,
- aspectConfigurationKey,
baseConfiguredTargetKey,
- starlarkFileLabel,
- starlarkValueName,
- Objects.hashCode(
- targetLabel,
- aspectConfigurationKey,
- baseConfiguredTargetKey,
- starlarkFileLabel,
- starlarkValueName)));
+ Objects.hashCode(topLevelAspectsClasses, targetLabel, baseConfiguredTargetKey)));
}
- private StarlarkAspectLoadingKey(
+ private TopLevelAspectsKey(
+ ImmutableList<AspectClass> topLevelAspectsClasses,
Label targetLabel,
- BuildConfigurationValue.Key aspectConfigurationKey,
ConfiguredTargetKey baseConfiguredTargetKey,
- Label starlarkFileLabel,
- String starlarkValueName,
int hashCode) {
+ super(baseConfiguredTargetKey, hashCode);
+ this.topLevelAspectsClasses = topLevelAspectsClasses;
this.targetLabel = targetLabel;
- this.aspectConfigurationKey = aspectConfigurationKey;
- this.baseConfiguredTargetKey = baseConfiguredTargetKey;
- this.starlarkFileLabel = starlarkFileLabel;
- this.starlarkValueName = starlarkValueName;
- this.hashCode = hashCode;
}
@Override
public SkyFunctionName functionName() {
- return SkyFunctions.LOAD_STARLARK_ASPECT;
+ return SkyFunctions.TOP_LEVEL_ASPECTS;
}
- String getStarlarkValueName() {
- return starlarkValueName;
- }
-
- Label getStarlarkFileLabel() {
- return starlarkFileLabel;
- }
-
- @Override
- public String getAspectName() {
- return String.format("%s%%%s", starlarkFileLabel, starlarkValueName);
+ ImmutableList<AspectClass> getTopLevelAspectsClasses() {
+ return topLevelAspectsClasses;
}
@Override
@@ -360,27 +323,8 @@
return targetLabel;
}
- @Override
- public String getDescription() {
- // Starlark aspects are referred to on command line with <file>%<value ame>
- return String.format("%s%%%s of %s", starlarkFileLabel, starlarkValueName, targetLabel);
- }
-
- @Nullable
- @Override
- BuildConfigurationValue.Key getAspectConfigurationKey() {
- return aspectConfigurationKey;
- }
-
- /** Returns the key for the base configured target for this aspect. */
- @Override
- public ConfiguredTargetKey getBaseConfiguredTargetKey() {
- return baseConfiguredTargetKey;
- }
-
- @Override
- public int hashCode() {
- return hashCode;
+ String getDescription() {
+ return topLevelAspectsClasses + " on " + getLabel();
}
@Override
@@ -388,35 +332,14 @@
if (o == this) {
return true;
}
- if (!(o instanceof StarlarkAspectLoadingKey)) {
+ if (!(o instanceof TopLevelAspectsKey)) {
return false;
}
- StarlarkAspectLoadingKey that = (StarlarkAspectLoadingKey) o;
- return hashCode == that.hashCode
+ TopLevelAspectsKey that = (TopLevelAspectsKey) o;
+ return hashCode() == that.hashCode()
&& Objects.equal(targetLabel, that.targetLabel)
- && Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey)
- && Objects.equal(baseConfiguredTargetKey, that.baseConfiguredTargetKey)
- && Objects.equal(starlarkFileLabel, that.starlarkFileLabel)
- && Objects.equal(starlarkValueName, that.starlarkValueName);
- }
-
- @Override
- public String toString() {
- return MoreObjects.toStringHelper(this)
- .add("targetLabel", targetLabel)
- .add("aspectConfigurationKey", aspectConfigurationKey)
- .add("baseConfiguredTargetKey", baseConfiguredTargetKey)
- .add("starlarkFileLabel", starlarkFileLabel)
- .add("starlarkValueName", starlarkValueName)
- .toString();
- }
-
- AspectKey toAspectKey(AspectClass aspectClass) {
- return AspectKey.createAspectKey(
- baseConfiguredTargetKey,
- ImmutableList.of(),
- new AspectDescriptor(aspectClass, AspectParameters.EMPTY),
- aspectConfigurationKey);
+ && Objects.equal(getBaseConfiguredTargetKey(), that.getBaseConfiguredTargetKey())
+ && Objects.equal(topLevelAspectsClasses, that.topLevelAspectsClasses);
}
}
}
