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

1 files changed, 34 insertions, 21 deletions

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 e861ab0d23..a5e12144f9 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.SkyFunction;
 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 @@ public final class AspectFunction implements SkyFunction {
           new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier()));
     }
 
+    boolean aspectHasConfiguration = key.getAspectConfigurationKey() != null;
+
+    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;
 
-    ConfiguredTargetValue configuredTargetValue;
     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 (configuredTargetValue.getConfiguredTarget() == null) {
-      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);
+      }
     }
 
-    ConfiguredTarget associatedTarget = configuredTargetValue.getConfiguredTarget();
+    ConfiguredTarget associatedTarget = baseConfiguredTargetValue.getConfiguredTarget();
 
     ConfiguredTargetAndTarget associatedConfiguredTargetAndTarget;
     Package targetPkg =
@@ -258,14 +271,14 @@ public final class AspectFunction implements SkyFunction {
       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 @@ public final class AspectFunction implements SkyFunction {
     // 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 @@ public final class AspectFunction implements SkyFunction {
                     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 @@ public final class AspectFunction implements SkyFunction {
           aspect,
           aspectFactory,
           associatedConfiguredTargetAndTarget,
-          key.getAspectConfiguration(),
+          aspectConfiguration,
           configConditions,
           toolchainContext,
           depValueMap,