Remove aspect configuration from AspectKey. Instead, store an appropriate SkyKey and perform the lookup in AspectFunction, in parallel with the lookup for the base configured target introduced in unknown commit.
PiperOrigin-RevId: 183399436
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index e861ab0..a5e1214 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -67,6 +67,7 @@
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.build.skyframe.ValueOrException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
@@ -220,30 +221,42 @@
new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier()));
}
+ boolean aspectHasConfiguration = key.getAspectConfigurationKey() != null;
- ConfiguredTargetValue configuredTargetValue;
+ ImmutableSet<SkyKey> keys =
+ aspectHasConfiguration
+ ? ImmutableSet.of(key.getBaseConfiguredTargetKey(), key.getAspectConfigurationKey())
+ : ImmutableSet.of(key.getBaseConfiguredTargetKey());
+
+ Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> baseAndAspectValues =
+ env.getValuesOrThrow(keys, ConfiguredValueCreationException.class);
+ if (env.valuesMissing()) {
+ return null;
+ }
+
+ ConfiguredTargetValue baseConfiguredTargetValue;
+ BuildConfiguration aspectConfiguration = null;
+
try {
- configuredTargetValue =
- (ConfiguredTargetValue)
- env.getValueOrThrow(
- key.getConfiguredTargetKey(), ConfiguredValueCreationException.class);
+ baseConfiguredTargetValue =
+ (ConfiguredTargetValue) baseAndAspectValues.get(key.getBaseConfiguredTargetKey()).get();
} catch (ConfiguredValueCreationException e) {
throw new AspectFunctionException(new AspectCreationException(e.getRootCauses()));
}
- if (configuredTargetValue == null) {
- // TODO(bazel-team): remove this check when top-level targets also use dynamic configurations.
- // Right now the key configuration may be dynamic while the original target's configuration
- // is static, resulting in a Skyframe cache miss even though the original target is, in fact,
- // precomputed.
- return null;
+
+ if (aspectHasConfiguration) {
+ try {
+ aspectConfiguration =
+ ((BuildConfigurationValue)
+ baseAndAspectValues.get(key.getAspectConfigurationKey()).get())
+ .getConfiguration();
+ } catch (ConfiguredValueCreationException e) {
+ throw new IllegalStateException("Unexpected exception from BuildConfigurationFunction when "
+ + "computing " + key.getAspectConfigurationKey(), e);
+ }
}
-
- if (configuredTargetValue.getConfiguredTarget() == null) {
- return null;
- }
-
- ConfiguredTarget associatedTarget = configuredTargetValue.getConfiguredTarget();
+ ConfiguredTarget associatedTarget = baseConfiguredTargetValue.getConfiguredTarget();
ConfiguredTargetAndTarget associatedConfiguredTargetAndTarget;
Package targetPkg =
@@ -258,14 +271,14 @@
throw new IllegalStateException("Name already verified", e);
}
- if (configuredTargetValue.getConfiguredTarget().getProvider(AliasProvider.class) != null) {
+ if (baseConfiguredTargetValue.getConfiguredTarget().getProvider(AliasProvider.class) != null) {
return createAliasAspect(
env,
view.getActionKeyContext(),
associatedConfiguredTargetAndTarget.getTarget(),
aspect,
key,
- configuredTargetValue.getConfiguredTarget());
+ baseConfiguredTargetValue.getConfiguredTarget());
}
@@ -315,7 +328,7 @@
// will be present this way.
TargetAndConfiguration originalTargetAndAspectConfiguration =
new TargetAndConfiguration(
- associatedConfiguredTargetAndTarget.getTarget(), key.getAspectConfiguration());
+ associatedConfiguredTargetAndTarget.getTarget(), aspectConfiguration);
ImmutableList<Aspect> aspectPath = aspectPathBuilder.build();
try {
// Get the configuration targets that trigger this rule's configurable attributes.
@@ -344,7 +357,7 @@
aspect.getDescriptor().getDescription(),
associatedConfiguredTargetAndTarget.getTarget().toString()),
requiredToolchains,
- key.getAspectConfiguration());
+ aspectConfiguration);
} catch (ToolchainContextException e) {
// TODO(katre): better error handling
throw new AspectCreationException(e.getMessage());
@@ -386,7 +399,7 @@
aspect,
aspectFactory,
associatedConfiguredTargetAndTarget,
- key.getAspectConfiguration(),
+ aspectConfiguration,
configConditions,
toolchainContext,
depValueMap,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index a79bf7d..e3b651f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -33,6 +33,8 @@
import com.google.devtools.build.lib.packages.AspectDescriptor;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.skyframe.BuildConfigurationValue.Key;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey.KeyAndHost;
import com.google.devtools.build.lib.syntax.SkylarkImport;
import com.google.devtools.build.skyframe.SkyFunctionName;
import java.util.List;
@@ -54,21 +56,21 @@
public static class AspectKey extends AspectValueKey {
private final Label label;
private final ImmutableList<AspectKey> baseKeys;
- private final BuildConfiguration aspectConfiguration;
- private final ConfiguredTargetKey configuredTargetKey;
+ private final BuildConfigurationValue.Key aspectConfigurationKey;
+ private final ConfiguredTargetKey baseConfiguredTargetKey;
private final AspectDescriptor aspectDescriptor;
private int hashCode;
private AspectKey(
Label label,
- ConfiguredTargetKey configuredTargetKey,
+ BuildConfigurationValue.Key aspectConfigurationKey,
+ ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
- AspectDescriptor aspectDescriptor,
- BuildConfiguration aspectConfiguration) {
+ AspectDescriptor aspectDescriptor) {
this.baseKeys = baseKeys;
this.label = label;
- this.configuredTargetKey = configuredTargetKey;
- this.aspectConfiguration = aspectConfiguration;
+ this.aspectConfigurationKey = aspectConfigurationKey;
+ this.baseConfiguredTargetKey = baseConfiguredTargetKey;
this.aspectDescriptor = aspectDescriptor;
}
@@ -112,8 +114,13 @@
}
}
+ protected boolean aspectConfigurationIsHost() {
+ return false;
+ }
+
/**
- * Returns the configuration to be used for the evaluation of the aspect itself.
+ * Returns the key of the configured target of the aspect; that is, the configuration in which
+ * the aspect will be evaluated.
*
* <p>In trimmed configuration mode, the aspect may require more fragments than the target on
* which it is being evaluated; in addition to configuration fragments required by the target
@@ -126,15 +133,15 @@
*
* <p>Because of these properties, this configuration is always a superset of the base target's
* configuration. In untrimmed configuration mode, this configuration will be equivalent to the
- * base target's configuration..
+ * base target's configuration.
*/
- public BuildConfiguration getAspectConfiguration() {
- return aspectConfiguration;
+ BuildConfigurationValue.Key getAspectConfigurationKey() {
+ return aspectConfigurationKey;
}
/** Returns the key for the base configured target for this aspect. */
- ConfiguredTargetKey getConfiguredTargetKey() {
- return configuredTargetKey;
+ ConfiguredTargetKey getBaseConfiguredTargetKey() {
+ return baseConfiguredTargetKey;
}
@Override
@@ -165,7 +172,7 @@
private int computeHashCode() {
return Objects.hashCode(
- label, baseKeys, aspectConfiguration, configuredTargetKey, aspectDescriptor);
+ label, baseKeys, aspectConfigurationKey, baseConfiguredTargetKey, aspectDescriptor);
}
@Override
@@ -181,8 +188,8 @@
AspectKey that = (AspectKey) other;
return Objects.equal(label, that.label)
&& Objects.equal(baseKeys, that.baseKeys)
- && Objects.equal(aspectConfiguration, that.aspectConfiguration)
- && Objects.equal(configuredTargetKey, that.configuredTargetKey)
+ && Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey)
+ && Objects.equal(baseConfiguredTargetKey, that.baseConfiguredTargetKey)
&& Objects.equal(aspectDescriptor, that.aspectDescriptor);
}
@@ -195,13 +202,12 @@
baseKeys.isEmpty()
? ""
: String.format(" (over %s)", baseKeys.toString());
- return String.format("%s with aspect %s%s%s",
+ return String.format(
+ "%s with aspect %s%s%s",
label.toString(),
aspectDescriptor.getAspectClass().getName(),
- (aspectConfiguration != null && aspectConfiguration.isHostConfiguration())
- ? "(host) " : "",
- baseKeysString
- );
+ (aspectConfigurationKey != null && aspectConfigurationIsHost()) ? "(host) " : "",
+ baseKeysString);
}
@Override
@@ -210,9 +216,9 @@
+ "#"
+ aspectDescriptor.getAspectClass().getName()
+ " "
- + (aspectConfiguration == null ? "null" : aspectConfiguration.checksum())
+ + aspectConfigurationKey
+ " "
- + configuredTargetKey
+ + baseConfiguredTargetKey
+ " "
+ aspectDescriptor.getParameters();
}
@@ -227,11 +233,30 @@
label,
ConfiguredTargetKey.of(
label,
- configuredTargetKey.getConfigurationKey(),
- configuredTargetKey.isHostConfiguration()),
+ baseConfiguredTargetKey.getConfigurationKey(),
+ baseConfiguredTargetKey.isHostConfiguration()),
newBaseKeys.build(),
aspectDescriptor,
- aspectConfiguration);
+ aspectConfigurationKey,
+ aspectConfigurationIsHost());
+ }
+ }
+
+ /** An {@link AspectKey} for an aspect in the host configuration. */
+ private static class HostAspectKey extends AspectKey {
+
+ private HostAspectKey(
+ Label label,
+ Key aspectConfigurationKey,
+ ConfiguredTargetKey baseConfiguredTargetKey,
+ ImmutableList<AspectKey> baseKeys,
+ AspectDescriptor aspectDescriptor) {
+ super(label, aspectConfigurationKey, baseConfiguredTargetKey, baseKeys, aspectDescriptor);
+ }
+
+ @Override
+ protected boolean aspectConfigurationIsHost() {
+ return true;
}
}
@@ -241,22 +266,21 @@
public static class SkylarkAspectLoadingKey extends AspectValueKey {
private final Label targetLabel;
- private final BuildConfiguration aspectConfiguration;
- private final ConfiguredTargetKey configuredTargetKey;
+ private final BuildConfigurationValue.Key aspectConfigurationKey;
+ private final ConfiguredTargetKey baseConfiguredTargetKey;
private final SkylarkImport skylarkImport;
private final String skylarkValueName;
private int hashCode;
private SkylarkAspectLoadingKey(
Label targetLabel,
- BuildConfiguration aspectConfiguration,
- ConfiguredTargetKey configuredTargetKey,
+ BuildConfigurationValue.Key aspectConfigurationKey,
+ ConfiguredTargetKey baseConfiguredTargetKey,
SkylarkImport skylarkImport,
String skylarkFunctionName) {
this.targetLabel = targetLabel;
- this.aspectConfiguration = aspectConfiguration;
- this.configuredTargetKey = configuredTargetKey;
-
+ this.aspectConfigurationKey = aspectConfigurationKey;
+ this.baseConfiguredTargetKey = baseConfiguredTargetKey;
this.skylarkImport = skylarkImport;
this.skylarkValueName = skylarkFunctionName;
}
@@ -274,6 +298,10 @@
return skylarkImport;
}
+ protected boolean isAspectConfigurationHost() {
+ return false;
+ }
+
@Override
public String getDescription() {
// Skylark aspects are referred to on command line with <file>%<value ame>
@@ -309,7 +337,11 @@
private int computeHashCode() {
return Objects.hashCode(
- targetLabel, aspectConfiguration, configuredTargetKey, skylarkImport, skylarkValueName);
+ targetLabel,
+ aspectConfigurationKey,
+ baseConfiguredTargetKey,
+ skylarkImport,
+ skylarkValueName);
}
@Override
@@ -323,8 +355,8 @@
}
SkylarkAspectLoadingKey that = (SkylarkAspectLoadingKey) o;
return Objects.equal(targetLabel, that.targetLabel)
- && Objects.equal(aspectConfiguration, that.aspectConfiguration)
- && Objects.equal(configuredTargetKey, that.configuredTargetKey)
+ && Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey)
+ && Objects.equal(baseConfiguredTargetKey, that.baseConfiguredTargetKey)
&& Objects.equal(skylarkImport, that.skylarkImport)
&& Objects.equal(skylarkValueName, that.skylarkValueName);
}
@@ -332,10 +364,34 @@
AspectKey toAspectKey(AspectClass aspectClass) {
return createAspectKey(
targetLabel,
- configuredTargetKey,
+ baseConfiguredTargetKey,
ImmutableList.of(),
new AspectDescriptor(aspectClass, AspectParameters.EMPTY),
- aspectConfiguration);
+ aspectConfigurationKey,
+ isAspectConfigurationHost());
+ }
+ }
+
+ /** A {@link SkylarkAspectLoadingKey} for an aspect in the host configuration. */
+ private static class HostSkylarkAspectLoadingKey extends SkylarkAspectLoadingKey {
+
+ private HostSkylarkAspectLoadingKey(
+ Label targetLabel,
+ Key aspectConfigurationKey,
+ ConfiguredTargetKey baseConfiguredTargetKey,
+ SkylarkImport skylarkImport,
+ String skylarkFunctionName) {
+ super(
+ targetLabel,
+ aspectConfigurationKey,
+ baseConfiguredTargetKey,
+ skylarkImport,
+ skylarkFunctionName);
+ }
+
+ @Override
+ protected boolean isAspectConfigurationHost() {
+ return true;
}
}
@@ -432,12 +488,14 @@
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor,
BuildConfiguration aspectConfiguration) {
+ KeyAndHost aspectKeyAndHost = ConfiguredTargetKey.keyFromConfiguration(aspectConfiguration);
return createAspectKey(
label,
ConfiguredTargetKey.of(label, baseConfiguration),
baseKeys,
aspectDescriptor,
- aspectConfiguration);
+ aspectKeyAndHost.key,
+ aspectKeyAndHost.isHost);
}
private static final Interner<AspectKey> aspectKeyInterner = BlazeInterners.newWeakInterner();
@@ -447,12 +505,14 @@
BuildConfiguration baseConfiguration,
AspectDescriptor aspectDescriptor,
BuildConfiguration aspectConfiguration) {
+ KeyAndHost aspectKeyAndHost = ConfiguredTargetKey.keyFromConfiguration(aspectConfiguration);
return createAspectKey(
label,
ConfiguredTargetKey.of(label, baseConfiguration),
ImmutableList.of(),
aspectDescriptor,
- aspectConfiguration);
+ aspectKeyAndHost.key,
+ aspectKeyAndHost.isHost);
}
private static AspectKey createAspectKey(
@@ -460,10 +520,14 @@
ConfiguredTargetKey configuredTargetKey,
ImmutableList<AspectKey> aspectKeys,
AspectDescriptor aspectDescriptor,
- BuildConfiguration aspectConfiguration) {
+ BuildConfigurationValue.Key aspectConfigurationKey,
+ boolean aspectConfigurationIsHost) {
return aspectKeyInterner.intern(
- new AspectKey(
- label, configuredTargetKey, aspectKeys, aspectDescriptor, aspectConfiguration));
+ aspectConfigurationIsHost
+ ? new HostAspectKey(
+ label, aspectConfigurationKey, configuredTargetKey, aspectKeys, aspectDescriptor)
+ : new AspectKey(
+ label, aspectConfigurationKey, configuredTargetKey, aspectKeys, aspectDescriptor));
}
private static final Interner<SkylarkAspectLoadingKey> skylarkAspectKeyInterner =
@@ -475,12 +539,22 @@
BuildConfiguration targetConfiguration,
SkylarkImport skylarkImport,
String skylarkExportName) {
- return skylarkAspectKeyInterner.intern(
- new SkylarkAspectLoadingKey(
- targetLabel,
- aspectConfiguration,
- ConfiguredTargetKey.of(targetLabel, targetConfiguration),
- skylarkImport,
- skylarkExportName));
+ KeyAndHost keyAndHost = ConfiguredTargetKey.keyFromConfiguration(aspectConfiguration);
+ SkylarkAspectLoadingKey key =
+ keyAndHost.isHost
+ ? new HostSkylarkAspectLoadingKey(
+ targetLabel,
+ keyAndHost.key,
+ ConfiguredTargetKey.of(targetLabel, targetConfiguration),
+ skylarkImport,
+ skylarkExportName)
+ : new SkylarkAspectLoadingKey(
+ targetLabel,
+ keyAndHost.key,
+ ConfiguredTargetKey.of(targetLabel, targetConfiguration),
+ skylarkImport,
+ skylarkExportName);
+
+ return skylarkAspectKeyInterner.intern(key);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
index 5a56202..c2353b8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java
@@ -185,7 +185,7 @@
private static final KeyAndHost NULL_INSTANCE = new KeyAndHost(null, false);
@Nullable public final BuildConfigurationValue.Key key;
- private final boolean isHost;
+ final boolean isHost;
private KeyAndHost(@Nullable BuildConfigurationValue.Key key, boolean isHost) {
this.key = key;
