Convert ActionLookupKey implementations to directly implement SkyKey,…

… removing the layer of indirection of getting SkyKey out of ActionLookupKey, which uses more memory for no reason.

PiperOrigin-RevId: 181658615

src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java

@@ -23,8 +23,6 @@
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.skyframe.LegacySkyKey;
-import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.ArrayList;
@@ -151,11 +149,6 @@ public String toString() {
     return getStringHelper().toString();
   }
 
-  @VisibleForTesting
-  public static SkyKey key(ActionLookupKey ownerKey) {
-    return ownerKey.getSkyKey();
-  }
-
   public int getNumActions() {
     return actions.size();
   }
@@ -190,43 +183,14 @@ public void actionEvaluated(int actionIndex, Action action) {
   }
 
   /**
-   * ArtifactOwner is not a SkyKey, but we wish to convert any ArtifactOwner into a SkyKey as simply
-   * as possible. To that end, all subclasses of ActionLookupValue "own" artifacts with
-   * ArtifactOwners that are subclasses of ActionLookupKey. This allows callers to easily find the
-   * value key, while remaining agnostic to what ActionLookupValues actually exist.
-   *
-   * <p>The methods of this class should only be called by {@link ActionLookupValue#key}.
+   * All subclasses of ActionLookupValue "own" artifacts with {@link ArtifactOwner}s that are
+   * subclasses of ActionLookupKey. This allows callers to easily find the value key, while
+   * remaining agnostic to what ActionLookupValues actually exist.
    */
-  public abstract static class ActionLookupKey implements ArtifactOwner {
+  public abstract static class ActionLookupKey implements ArtifactOwner, SkyKey {
     @Override
     public Label getLabel() {
       return null;
     }
-
-    /**
-     * Subclasses must override this to specify their specific value type, unless they override
-     * {@link #getSkyKey}, in which case they are free not to implement this method.
-     */
-    protected abstract SkyFunctionName getType();
-
-    protected SkyKey getSkyKeyInternal() {
-      return LegacySkyKey.create(getType(), this);
-    }
-
-    /**
-     * Prefer {@link ActionLookupValue#key} to calling this method directly.
-     *
-     * <p>Subclasses may override {@link #getSkyKeyInternal} if the {@link SkyKey} argument should
-     * not be this {@link ActionLookupKey} itself.
-     */
-    public final SkyKey getSkyKey() {
-      SkyKey result = getSkyKeyInternal();
-      Preconditions.checkState(
-          result.argument() instanceof ActionLookupKey,
-          "Not ActionLookupKey for %s: %s",
-          this,
-          result);
-      return result;
-    }
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java

@@ -156,7 +156,7 @@ private static AspectValue.AspectKey buildAspectKey(AspectCollection.AspectDeps
         dependentAspects.build(),
         aspectDeps.getAspect(),
         dep.getAspectConfiguration(aspectDeps.getAspect()));
-    result.put(aspectKey.getAspectDescriptor(), aspectKey.getSkyKey());
+    result.put(aspectKey.getAspectDescriptor(), aspectKey);
     return aspectKey;
   }
 

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

@@ -490,13 +490,15 @@ public AnalysisResult update(
       eventBus.post(new TargetConfiguredEvent(target, byLabel.get(target.getLabel())));
     }
 
-    List<ConfiguredTargetKey> topLevelCtKeys = Lists.transform(topLevelTargetsWithConfigs,
-        new Function<TargetAndConfiguration, ConfiguredTargetKey>() {
-          @Override
-          public ConfiguredTargetKey apply(TargetAndConfiguration node) {
-            return new ConfiguredTargetKey(node.getLabel(), node.getConfiguration());
-          }
-        });
+    List<ConfiguredTargetKey> topLevelCtKeys =
+        Lists.transform(
+            topLevelTargetsWithConfigs,
+            new Function<TargetAndConfiguration, ConfiguredTargetKey>() {
+              @Override
+              public ConfiguredTargetKey apply(TargetAndConfiguration node) {
+                return ConfiguredTargetKey.of(node.getLabel(), node.getConfiguration());
+              }
+            });
 
     Multimap<Pair<Label, String>, BuildConfiguration> aspectConfigurations =
         ArrayListMultimap.create();
@@ -646,13 +648,14 @@ private AnalysisResult createResult(
     Iterables.addAll(artifactsToBuild, baselineCoverageArtifacts);
     if (coverageReportActionFactory != null) {
       CoverageReportActionsWrapper actionsWrapper;
-      actionsWrapper = coverageReportActionFactory.createCoverageReportActionsWrapper(
-          eventHandler,
-          directories,
-          allTargetsToTest,
-          baselineCoverageArtifacts,
-          getArtifactFactory(),
-          CoverageReportValue.ARTIFACT_OWNER);
+      actionsWrapper =
+          coverageReportActionFactory.createCoverageReportActionsWrapper(
+              eventHandler,
+              directories,
+              allTargetsToTest,
+              baselineCoverageArtifacts,
+              getArtifactFactory(),
+              CoverageReportValue.COVERAGE_REPORT_KEY);
       if (actionsWrapper != null) {
         ImmutableList<ActionAnalysisMetadata> actions = actionsWrapper.getActions();
         skyframeExecutor.injectCoverageReportData(actions);
@@ -673,7 +676,7 @@ private AnalysisResult createResult(
           public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) {
             ArtifactOwner artifactOwner = artifact.getArtifactOwner();
             if (artifactOwner instanceof ActionLookupValue.ActionLookupKey) {
-              SkyKey key = ActionLookupValue.key((ActionLookupValue.ActionLookupKey) artifactOwner);
+              SkyKey key = (ActionLookupValue.ActionLookupKey) artifactOwner;
               ActionLookupValue val;
               try {
                 val = (ActionLookupValue) graph.getValue(key);
@@ -1081,7 +1084,7 @@ public RuleContext getRuleContextForTesting(
         new CachingAnalysisEnvironment(
             getArtifactFactory(),
             skyframeExecutor.getActionKeyContext(),
-            new ConfiguredTargetKey(target.getLabel(), targetConfig),
+            ConfiguredTargetKey.of(target.getLabel(), targetConfig),
             /*isSystemEnv=*/ false,
             targetConfig.extendedSanityChecks(),
             eventHandler,

src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java

@@ -297,14 +297,14 @@ public SkylarkSemantics getSkylarkSemantics() throws InterruptedException {
 
   @Override
   public Artifact getStableWorkspaceStatusArtifact() throws InterruptedException {
-    return ((WorkspaceStatusValue) skyframeEnv.getValue(WorkspaceStatusValue.SKY_KEY))
-            .getStableArtifact();
+    return ((WorkspaceStatusValue) skyframeEnv.getValue(WorkspaceStatusValue.BUILD_INFO_KEY))
+        .getStableArtifact();
   }
 
   @Override
   public Artifact getVolatileWorkspaceStatusArtifact() throws InterruptedException {
-    return ((WorkspaceStatusValue) skyframeEnv.getValue(WorkspaceStatusValue.SKY_KEY))
-            .getVolatileArtifact();
+    return ((WorkspaceStatusValue) skyframeEnv.getValue(WorkspaceStatusValue.BUILD_INFO_KEY))
+        .getVolatileArtifact();
   }
 
   // See SkyframeBuildView#getWorkspaceStatusValues for the code that this method is attempting to
@@ -327,8 +327,7 @@ public ImmutableList<Artifact> getBuildInfo(
       throws InterruptedException {
     boolean stamp = AnalysisUtils.isStampingEnabled(ruleContext, config);
     BuildInfoCollectionValue collectionValue =
-        (BuildInfoCollectionValue) skyframeEnv.getValue(BuildInfoCollectionValue.key(
-            new BuildInfoCollectionValue.BuildInfoKeyAndConfig(key, config)));
+        (BuildInfoCollectionValue) skyframeEnv.getValue(BuildInfoCollectionValue.key(key, config));
     if (collectionValue == null) {
       throw collectDebugInfoAndCrash(key, config);
     }

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

@@ -166,8 +166,10 @@ private Artifact getOutputArtifact(AnalysisEnvironment analysisEnvironment, Outp
     Root root = rule.hasBinaryOutput()
         ? configuration.getBinDirectory(rule.getRepository())
         : configuration.getGenfilesDirectory(rule.getRepository());
-    ArtifactOwner owner = new ConfiguredTargetKey(rule.getLabel(),
-        getArtifactOwnerConfiguration(analysisEnvironment.getSkyframeEnv(), configuration));
+    ArtifactOwner owner =
+        ConfiguredTargetKey.of(
+            rule.getLabel(),
+            getArtifactOwnerConfiguration(analysisEnvironment.getSkyframeEnv(), configuration));
     if (analysisEnvironment.getSkyframeEnv().valuesMissing()) {
       return null;
     }
@@ -271,11 +273,13 @@ public final ConfiguredTarget createConfiguredTarget(
       }
     } else if (target instanceof InputFile) {
       InputFile inputFile = (InputFile) target;
-      Artifact artifact = artifactFactory.getSourceArtifact(
-          inputFile.getExecPath(),
-          Root.asSourceRoot(inputFile.getPackage().getSourceRoot(),
-              inputFile.getPackage().getPackageIdentifier().getRepository().isMain()),
-          new ConfiguredTargetKey(target.getLabel(), config));
+      Artifact artifact =
+          artifactFactory.getSourceArtifact(
+              inputFile.getExecPath(),
+              Root.asSourceRoot(
+                  inputFile.getPackage().getSourceRoot(),
+                  inputFile.getPackage().getPackageIdentifier().getRepository().isMain()),
+              ConfiguredTargetKey.of(target.getLabel(), config));
 
       return new InputFileConfiguredTarget(targetContext, inputFile, artifact);
     } else if (target instanceof PackageGroup) {

src/main/java/com/google/devtools/build/lib/analysis/TargetAndConfiguration.java

@@ -13,13 +13,11 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
-import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
 import java.util.Objects;
 import javax.annotation.Nullable;
 
@@ -49,22 +47,6 @@ public String getName() {
         + (configuration == null ? "null" : configuration.checksum());
   }
 
-  public static final Function<TargetAndConfiguration, String> NAME_FUNCTION =
-      new Function<TargetAndConfiguration, String>() {
-        @Override
-        public String apply(TargetAndConfiguration node) {
-          return node.getName();
-        }
-      };
-
-  public static final Function<TargetAndConfiguration, ConfiguredTargetKey>
-      TO_LABEL_AND_CONFIGURATION = new Function<TargetAndConfiguration, ConfiguredTargetKey>() {
-        @Override
-        public ConfiguredTargetKey apply(TargetAndConfiguration input) {
-          return new ConfiguredTargetKey(input.getLabel(), input.getConfiguration());
-        }
-      };
-
   @Override
   public boolean equals(Object that) {
     if (this == that) {

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java

@@ -474,7 +474,7 @@ public Iterable<Artifact> discoverInputsStage2(SkyFunction.Environment env)
     }
     Map<Artifact, SkyKey> skyKeys = new HashMap<>();
     for (Artifact artifact : this.usedModules) {
-      skyKeys.put(artifact, ActionLookupValue.key((ActionLookupKey) artifact.getArtifactOwner()));
+      skyKeys.put(artifact, (ActionLookupKey) artifact.getArtifactOwner());
     }
     Map<SkyKey, SkyValue> skyValues = env.getValues(skyKeys.values());
     Set<Artifact> additionalModules = Sets.newLinkedHashSet();

src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java

@@ -15,42 +15,37 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Interner;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
 import com.google.devtools.build.lib.analysis.actions.ActionTemplate;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.skyframe.LegacySkyKey;
+import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.skyframe.SkyFunctionName;
-import com.google.devtools.build.skyframe.SkyKey;
 
 /**
  * Value that stores expanded actions from ActionTemplate.
  */
 public final class ActionTemplateExpansionValue extends ActionLookupValue {
-
   ActionTemplateExpansionValue(
       ActionKeyContext actionKeyContext,
       Iterable<Action> expandedActions,
       boolean removeActionsAfterEvaluation) {
     super(actionKeyContext, ImmutableList.copyOf(expandedActions), removeActionsAfterEvaluation);
   }
 
-  static SkyKey key(ActionTemplate<?> actionTemplate) {
-    return LegacySkyKey.create(
-        SkyFunctions.ACTION_TEMPLATE_EXPANSION, createActionTemplateExpansionKey(actionTemplate));
-  }
+  private static final Interner<ActionTemplateExpansionKey> interner =
+      BlazeInterners.newWeakInterner();
 
-  static ActionTemplateExpansionKey createActionTemplateExpansionKey(
-      ActionTemplate<?> actionTemplate) {
-    return new ActionTemplateExpansionKey(actionTemplate);
+  static ActionTemplateExpansionKey key(ActionTemplate<?> actionTemplate) {
+    return interner.intern(new ActionTemplateExpansionKey(actionTemplate));
   }
 
-
   static final class ActionTemplateExpansionKey extends ActionLookupKey {
     private final ActionTemplate<?> actionTemplate;
 
-    ActionTemplateExpansionKey(ActionTemplate<?> actionTemplate) {
+    private ActionTemplateExpansionKey(ActionTemplate<?> actionTemplate) {
       Preconditions.checkNotNull(
           actionTemplate,
           "Passed in action template cannot be null: %s",
@@ -59,11 +54,10 @@ static final class ActionTemplateExpansionKey extends ActionLookupKey {
     }
 
     @Override
-    protected SkyFunctionName getType() {
+    public SkyFunctionName functionName() {
       return SkyFunctions.ACTION_TEMPLATE_EXPANSION;
     }
 
-
     @Override
     public Label getLabel() {
       return actionTemplate.getOwner().getLabel();

src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java

@@ -315,7 +315,7 @@ static SkyKey getActionLookupKey(Artifact artifact) {
     ArtifactOwner artifactOwner = artifact.getArtifactOwner();
 
     Preconditions.checkState(artifactOwner instanceof ActionLookupKey, "", artifact, artifactOwner);
-    return ActionLookupValue.key((ActionLookupKey) artifactOwner);
+    return (ActionLookupKey) artifactOwner;
   }
 
   @Nullable
@@ -326,7 +326,7 @@ private static ActionLookupValue getActionLookupValue(
     if (value == null) {
       ArtifactOwner artifactOwner = artifact.getArtifactOwner();
       Preconditions.checkState(
-          artifactOwner == CoverageReportValue.ARTIFACT_OWNER,
+          artifactOwner == CoverageReportValue.COVERAGE_REPORT_KEY,
           "Not-yet-present artifact owner: %s (%s %s)",
           artifactOwner,
           artifact,

src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java

@@ -20,7 +20,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
-import com.google.devtools.build.lib.actions.ActionLookupValue;
 import com.google.devtools.build.lib.analysis.AliasProvider;
 import com.google.devtools.build.lib.analysis.AspectResolver;
 import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
@@ -408,8 +407,7 @@ private ConfiguredTarget getBaseTarget(ConfiguredTarget target,
       throws DuplicateException {
     ArrayList<ConfiguredAspect> aspectValues = new ArrayList<>();
     for (AspectKey aspectKey : aspectKeys) {
-      SkyKey skyAspectKey = aspectKey.getSkyKey();
-      AspectValue aspectValue = (AspectValue) values.get(skyAspectKey);
+      AspectValue aspectValue = (AspectValue) values.get(aspectKey);
       ConfiguredAspect configuredAspect = aspectValue.getConfiguredAspect();
       aspectValues.add(configuredAspect);
     }
@@ -438,14 +436,13 @@ private void buildSkyKeys(AspectKey key, HashMap<AspectDescriptor, SkyKey> resul
       return;
     }
     ImmutableList<AspectKey> baseKeys = key.getBaseKeys();
-    SkyKey skyKey = key.getSkyKey();
-    result.put(key.getAspectDescriptor(), skyKey);
+    result.put(key.getAspectDescriptor(), key);
     for (AspectKey baseKey : baseKeys) {
       buildSkyKeys(baseKey, result, aspectPathBuilder);
     }
     // Post-order list of aspect SkyKeys gives the order of propagating aspects:
     // the aspect comes after all aspects it transitively sees.
-    aspectPathBuilder.add(skyKey);
+    aspectPathBuilder.add(key);
   }
 
   private SkyValue createAliasAspect(
@@ -462,7 +459,7 @@ private SkyValue createAliasAspect(
     // the real configured target.
     Label aliasLabel = aliasChain.size() > 1 ? aliasChain.get(1) : configuredTarget.getLabel();
 
-    SkyKey depKey = ActionLookupValue.key(originalKey.withLabel(aliasLabel));
+    SkyKey depKey = originalKey.withLabel(aliasLabel);
 
     // Compute the AspectValue of the target the alias refers to (which can itself be either an
     // alias or a real target)