Remove PackageValue.Key wrapper around PackageIdentifier in favor…

… of implementing `SkyKey` directly.

This saves a tiny bit of memory but not much since there aren't that many `PACKAGE` nodes. Perhaps more importantly, it saves the cost of constructing a `PackageValue.Key`, which includes interning. This happens even more frequently with pooled `Label` interning, since we need to look up the corresponding `PACKAGE` node in the graph.

PiperOrigin-RevId: 527294657
Change-Id: I297046fbb67c6b9bd956e726bf403caf2f037b09

src/main/java/com/google/devtools/build/lib/analysis/producers/ConfiguredTargetAndDataProducer.java

@@ -124,7 +124,7 @@ private StateMachine fetchConfigurationAndPackage(Tasks tasks, ExtendedEventHand
       // example, suppose that a configured target A has two children B and C. If B is dirty, it
       // causes A's re-evaluation, which causes this fetch to be performed for C. However, C has not
       // been evaluated this build.
-      tasks.lookUp(PackageValue.key(packageId), (Consumer<SkyValue>) this);
+      tasks.lookUp(packageId, (Consumer<SkyValue>) this);
     }
 
     return this::constructResult;

src/main/java/com/google/devtools/build/lib/analysis/producers/PlatformInfoProducer.java

@@ -70,7 +70,7 @@ public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
     //
     // In distributed analysis, these packages will be duplicated across shards.
     tasks.lookUp(
-        PackageValue.key(platformKey.getLabel().getPackageIdentifier()),
+        platformKey.getLabel().getPackageIdentifier(),
         NoSuchPackageException.class,
         (StateMachine.ValueOrExceptionSink<NoSuchPackageException>) this);
     return this::lookupPlatform;

src/main/java/com/google/devtools/build/lib/cmdline/BUILD

@@ -41,14 +41,17 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
         "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
         "//src/main/java/com/google/devtools/build/lib/supplier",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
+        "//src/main/java/com/google/devtools/build/lib/util:hash_codes",
         "//src/main/java/com/google/devtools/build/lib/util:string",
         "//src/main/java/com/google/devtools/build/lib/vfs:ospathpolicy",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/main/java/com/google/devtools/build/skyframe:cpu_heavy_skykey",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
         "//src/main/java/net/starlark/java/annot",
         "//src/main/java/net/starlark/java/eval",

src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java

@@ -16,21 +16,26 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ComparisonChain;
-import com.google.common.collect.Interner;
-import com.google.devtools.build.lib.concurrent.BlazeInterners;
+import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.util.HashCodes;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.util.Objects;
+import com.google.devtools.build.skyframe.CPUHeavySkyKey;
+import com.google.devtools.build.skyframe.SkyFunctionName;
+import com.google.devtools.build.skyframe.SkyKey;
 import javax.annotation.concurrent.Immutable;
 
 /**
  * Uniquely identifies a package. Contains the (canonical) name of the repository this package lives
  * in, and the package's path fragment.
+ *
+ * <p>Used as a {@link SkyKey} to request a {@link
+ * com.google.devtools.build.lib.skyframe.PackageValue}.
  */
 @AutoCodec
 @Immutable
-public final class PackageIdentifier implements Comparable<PackageIdentifier> {
-  private static final Interner<PackageIdentifier> INTERNER = BlazeInterners.newWeakInterner();
+public final class PackageIdentifier implements CPUHeavySkyKey, Comparable<PackageIdentifier> {
+  private static final SkyKeyInterner<PackageIdentifier> interner = SkyKey.newInterner();
 
   public static PackageIdentifier create(String repository, PathFragment pkgName)
       throws LabelSyntaxException {
@@ -39,8 +44,7 @@ public static PackageIdentifier create(String repository, PathFragment pkgName)
 
   @AutoCodec.Instantiator
   public static PackageIdentifier create(RepositoryName repository, PathFragment pkgName) {
-    // Note: We rely on these being (weakly) interned to fast-path Label#equals.
-    return INTERNER.intern(new PackageIdentifier(repository, pkgName));
+    return interner.intern(new PackageIdentifier(repository, pkgName));
   }
 
   /** Creates {@code PackageIdentifier} from a known-valid string. */
@@ -112,7 +116,7 @@ public static PackageIdentifier discoverFromExecPath(
   private PackageIdentifier(RepositoryName repository, PathFragment pkgName) {
     this.repository = Preconditions.checkNotNull(repository);
     this.pkgName = Preconditions.checkNotNull(pkgName);
-    this.hashCode = Objects.hash(repository, pkgName);
+    this.hashCode = HashCodes.hashObjects(repository, pkgName);
   }
 
   public static PackageIdentifier parse(String input) throws LabelSyntaxException {
@@ -174,16 +178,16 @@ public PathFragment getRunfilesPath() {
    */
   // TODO(bazel-team): Maybe rename to "getDefaultForm"?
   public String getCanonicalForm() {
-    return repository.getCanonicalForm() + "//" + getPackageFragment();
+    return repository.getCanonicalForm() + "//" + pkgName;
   }
 
   /**
    * Returns an absolutely unambiguous canonical form for this package in label form. Parsing this
    * string in any environment, even when subject to repository mapping, should identify the same
    * package.
    */
-  public String getUnambiguousCanonicalForm() {
-    return String.format("@%s//%s", getRepository().getNameWithAt(), getPackageFragment());
+  String getUnambiguousCanonicalForm() {
+    return String.format("@%s//%s", repository.getNameWithAt(), pkgName);
   }
 
   /**
@@ -204,8 +208,17 @@ public String getUnambiguousCanonicalForm() {
    *           from the main module
    */
   public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
-    return String.format(
-        "%s//%s", getRepository().getDisplayForm(mainRepositoryMapping), getPackageFragment());
+    return String.format("%s//%s", repository.getDisplayForm(mainRepositoryMapping), pkgName);
+  }
+
+  @Override
+  public SkyFunctionName functionName() {
+    return SkyFunctions.PACKAGE;
+  }
+
+  @Override
+  public SkyKeyInterner<?> getSkyKeyInterner() {
+    return interner;
   }
 
   /**
@@ -219,7 +232,7 @@ public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
   @Override
   public String toString() {
     if (repository.isMain()) {
-      return getPackageFragment().getPathString();
+      return pkgName.getPathString();
     }
     return getCanonicalForm();
   }

src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java

@@ -187,8 +187,7 @@ private void checkSettings(Set<Setting> settings) throws QueryException {
   @Override
   public Target getTarget(Label label) throws TargetNotFoundException, InterruptedException {
     try {
-      return ((PackageValue)
-              walkableGraphSupplier.get().getValue(PackageValue.key(label.getPackageIdentifier())))
+      return ((PackageValue) walkableGraphSupplier.get().getValue(label.getPackageIdentifier()))
           .getPackage()
           .getTarget(label.getName());
     } catch (NoSuchTargetException e) {

src/main/java/com/google/devtools/build/lib/query2/RBuildFilesVisitor.java

@@ -38,7 +38,6 @@
 import com.google.devtools.build.lib.skyframe.DirectoryListingStateValue;
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue;
-import com.google.devtools.build.lib.skyframe.PackageValue;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.WorkspaceNameValue;
 import com.google.devtools.build.lib.vfs.FileStateKey;
@@ -81,8 +80,7 @@ public class RBuildFilesVisitor extends ParallelQueryVisitor<SkyKey, PackageIden
           SkyFunctions.PREPARE_DEPS_OF_PATTERN,
           SkyFunctions.PREPARE_DEPS_OF_PATTERNS);
 
-  private static final SkyKey EXTERNAL_PACKAGE_KEY =
-      PackageValue.key(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER);
+  private static final SkyKey EXTERNAL_PACKAGE_KEY = LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER;
   private final SkyQueryEnvironment env;
   private final QueryExpressionContext<Target> context;
   private final Uniquifier<SkyKey> visitUniquifier;

src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java

@@ -289,11 +289,11 @@ protected void beforeEvaluateQuery(QueryExpression expr)
       executor =
           MoreExecutors.listeningDecorator(
               new ThreadPoolExecutor(
-                  /*corePoolSize=*/ queryEvaluationParallelismLevel,
-                  /*maximumPoolSize=*/ queryEvaluationParallelismLevel,
-                  /*keepAliveTime=*/ 1,
-                  /*unit=*/ TimeUnit.SECONDS,
-                  /*workQueue=*/ new BlockingStack<Runnable>(),
+                  /* corePoolSize= */ queryEvaluationParallelismLevel,
+                  /* maximumPoolSize= */ queryEvaluationParallelismLevel,
+                  /* keepAliveTime= */ 1,
+                  /* unit= */ TimeUnit.SECONDS,
+                  /* workQueue= */ new BlockingStack<>(),
                   new ThreadFactoryBuilder().setNameFormat("QueryEnvironment %d").build()));
     }
     resolver = makeNewTargetPatternResolver();
@@ -331,7 +331,7 @@ boolean hasDependencyFilter() {
     return dependencyFilter != DependencyFilter.ALL_DEPS;
   }
 
-  private void checkEvaluationResult(
+  private static void checkEvaluationResult(
       ImmutableList<String> universeScopeList,
       Set<SkyKey> roots,
       SkyKey universeKey,
@@ -380,15 +380,15 @@ public final QueryExpression transformParsedQuery(QueryExpression queryExpressio
   protected QueryExpressionMapper<Void> getQueryExpressionMapper() {
     Optional<ImmutableList<String>> constantUniverseScopeListMaybe =
         universeScope.getConstantValueMaybe();
-    if (!constantUniverseScopeListMaybe.isPresent()) {
+    if (constantUniverseScopeListMaybe.isEmpty()) {
       return QueryExpressionMapper.identity();
     }
     ImmutableList<String> constantUniverseScopeList = constantUniverseScopeListMaybe.get();
     if (constantUniverseScopeList.size() != 1) {
       return QueryExpressionMapper.identity();
     }
     String universeScopePatternString = Iterables.getOnlyElement(constantUniverseScopeList);
-    TargetPattern absoluteUniverseScopePattern = null;
+    TargetPattern absoluteUniverseScopePattern;
     try {
       absoluteUniverseScopePattern =
           mainRepoTargetParser.parse(mainRepoTargetParser.absolutize(universeScopePatternString));
@@ -474,8 +474,8 @@ public QueryEvalResult evaluateQuery(
     return evaluateQueryInternal(expr, batchCallback);
   }
 
-  Map<SkyKey, Collection<Target>> targetifyValues(Map<SkyKey, ? extends Iterable<SkyKey>> input)
-      throws InterruptedException {
+  private Map<SkyKey, Collection<Target>> targetifyValues(
+      Map<SkyKey, ? extends Iterable<SkyKey>> input) throws InterruptedException {
     return targetifyValues(
         input, makePackageKeyToTargetKeyMap(ImmutableSet.copyOf(Iterables.concat(input.values()))));
   }
@@ -513,7 +513,7 @@ protected Map<SkyKey, Iterable<SkyKey>> getReverseDepLabelsOfLabels(
     return graph.getReverseDeps(labels);
   }
 
-  private Set<Label> getAllowedDeps(Rule rule) throws InterruptedException {
+  private Set<Label> getAllowedDeps(Rule rule) {
     Set<Label> allowedLabels = new HashSet<>(rule.getTransitions(dependencyFilter).values());
     if (visibilityDepsAreAllowed) {
       // Rule#getTransitions only visits the labels of attribute values, so that means it doesn't
@@ -526,8 +526,7 @@ private Set<Label> getAllowedDeps(Rule rule) throws InterruptedException {
     return allowedLabels;
   }
 
-  private Collection<Target> filterFwdDeps(Target target, Collection<Target> rawFwdDeps)
-      throws InterruptedException {
+  private Collection<Target> filterFwdDeps(Target target, Collection<Target> rawFwdDeps) {
     if (!(target instanceof Rule)) {
       return rawFwdDeps;
     }
@@ -651,8 +650,7 @@ Collection<Target> filterRawReverseDepsOfTransitiveTraversalKeys(
     return processRawReverseDeps(targetifyValues(rawReverseDeps, packageKeyToTargetKeyMap));
   }
 
-  private Collection<Target> processRawReverseDeps(Map<SkyKey, Collection<Target>> rawReverseDeps)
-      throws InterruptedException {
+  private Set<Target> processRawReverseDeps(Map<SkyKey, Collection<Target>> rawReverseDeps) {
     Set<Target> result = CompactHashSet.create();
     CompactHashSet<Target> visited =
         CompactHashSet.createWithExpectedSize(totalSizeOfCollections(rawReverseDeps.values()));
@@ -1001,14 +999,10 @@ public VisitTaskStatusCallback getVisitTaskStatusCallback() {
     return VisitTaskStatusCallback.NULL_INSTANCE;
   }
 
-  private Target getLoadTarget(Label label, Package pkg) {
+  private static Target getLoadTarget(Label label, Package pkg) {
     return new FakeLoadTarget(label, pkg);
   }
 
-  int getQueryEvaluationParallelismLevel() {
-    return queryEvaluationParallelismLevel;
-  }
-
   @ThreadSafe
   @Override
   public TargetAccessor<Target> getAccessor() {
@@ -1024,23 +1018,23 @@ public RepositoryMapping getMainRepoMapping() {
   @ThreadSafe
   private Package getPackage(PackageIdentifier packageIdentifier)
       throws InterruptedException, QueryException, NoSuchPackageException {
-    SkyKey packageKey = PackageValue.key(packageIdentifier);
-    PackageValue packageValue = (PackageValue) graph.getValue(packageKey);
+    PackageValue packageValue = (PackageValue) graph.getValue(packageIdentifier);
     if (packageValue != null) {
       Package pkg = packageValue.getPackage();
       if (pkg.containsErrors()) {
         throw new BuildFileContainsErrorsException(packageIdentifier);
       }
       return pkg;
     } else {
-      NoSuchPackageException exception = (NoSuchPackageException) graph.getException(packageKey);
+      NoSuchPackageException exception =
+          (NoSuchPackageException) graph.getException(packageIdentifier);
       if (exception != null) {
         throw exception;
       }
-      if (graph.isCycle(packageKey)) {
+      if (graph.isCycle(packageIdentifier)) {
         throw new NoSuchPackageException(packageIdentifier, "Package depends on a cycle");
       } else {
-        throw new QueryException(packageKey + " does not exist in graph", Query.Code.CYCLE);
+        throw new QueryException(packageIdentifier + " does not exist in graph", Query.Code.CYCLE);
       }
     }
   }
@@ -1093,9 +1087,8 @@ public Map<Label, Target> getTargets(Iterable<Label> labels) throws InterruptedE
   @ThreadSafe
   public Map<PackageIdentifier, Package> bulkGetPackages(Iterable<PackageIdentifier> pkgIds)
       throws InterruptedException {
-    Set<SkyKey> pkgKeys = ImmutableSet.copyOf(PackageValue.keys(pkgIds));
     ImmutableMap.Builder<PackageIdentifier, Package> pkgResults = ImmutableMap.builder();
-    Map<SkyKey, SkyValue> packages = graph.getSuccessfulValues(pkgKeys);
+    Map<SkyKey, SkyValue> packages = graph.getSuccessfulValues(pkgIds);
     for (Map.Entry<SkyKey, SkyValue> pkgEntry : packages.entrySet()) {
       PackageIdentifier pkgId = (PackageIdentifier) pkgEntry.getKey().argument();
       PackageValue pkgValue = (PackageValue) pkgEntry.getValue();
@@ -1247,7 +1240,7 @@ public ExtendedEventHandler getEventHandler() {
   public static final Function<SkyKey, Label> SKYKEY_TO_LABEL =
       skyKey -> IS_LABEL.test(skyKey) ? (Label) skyKey.argument() : null;
 
-  static final Function<SkyKey, PackageIdentifier> PACKAGE_SKYKEY_TO_PACKAGE_IDENTIFIER =
+  private static final Function<SkyKey, PackageIdentifier> PACKAGE_SKYKEY_TO_PACKAGE_IDENTIFIER =
       skyKey -> (PackageIdentifier) skyKey.argument();
 
   public static Multimap<SkyKey, SkyKey> makePackageKeyToTargetKeyMap(Iterable<SkyKey> keys) {
@@ -1257,7 +1250,7 @@ public static Multimap<SkyKey, SkyKey> makePackageKeyToTargetKeyMap(Iterable<Sky
       if (label == null) {
         continue;
       }
-      packageKeyToTargetKeyMap.put(PackageValue.key(label.getPackageIdentifier()), key);
+      packageKeyToTargetKeyMap.put(label.getPackageIdentifier(), key);
     }
     return packageKeyToTargetKeyMap;
   }
@@ -1270,7 +1263,7 @@ public static Set<PackageIdentifier> getPkgIdsNeededForTargetification(
   }
 
   @ThreadSafe
-  public Map<SkyKey, Target> getTargetKeyToTargetMapForPackageKeyToTargetKeyMap(
+  Map<SkyKey, Target> getTargetKeyToTargetMapForPackageKeyToTargetKeyMap(
       Multimap<SkyKey, SkyKey> packageKeyToTargetKeyMap) throws InterruptedException {
     ImmutableMap.Builder<SkyKey, Target> resultBuilder = ImmutableMap.builder();
     getTargetsForPackageKeyToTargetKeyMapHelper(packageKeyToTargetKeyMap, resultBuilder::put);
@@ -1330,7 +1323,7 @@ protected Iterable<Target> getBuildFileTargetsForPackageKeys(
     packageSemaphore.acquireAll(pkgIds);
     try {
       return Iterables.transform(
-          graph.getSuccessfulValues(PackageValue.keys(pkgIds)).values(),
+          graph.getSuccessfulValues(pkgIds).values(),
           skyValue -> ((PackageValue) skyValue).getPackage().getBuildFile());
     } finally {
       packageSemaphore.releaseAll(pkgIds);

src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java

@@ -468,7 +468,7 @@ public Map<String, Collection<Target>> preloadTargetPatterns(
             for (Label label :
                 Iterables.concat(
                     resolvedLabels.getTargets(), resolvedLabels.getFilteredTargets())) {
-              packageKeys.add(PackageValue.key(label.getPackageIdentifier()));
+              packageKeys.add(label.getPackageIdentifier());
             }
           }
         }

src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryTtvPackageProviderFactory.java

@@ -78,7 +78,7 @@ public GenQueryPackageProvider constructPackageMap(Environment env, ImmutableLis
       }
       validTargets.addTransitive(transNode.getTransitiveTargets());
       for (Label transitiveLabel : transNode.getTransitiveTargets().toList()) {
-        successfulPackageKeys.add(PackageValue.key(transitiveLabel.getPackageIdentifier()));
+        successfulPackageKeys.add(transitiveLabel.getPackageIdentifier());
       }
     }
 

src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintSetting.java

@@ -80,7 +80,7 @@ private void validateDefaultConstraintValue(
     // because it will cause a cycle.
     SkyFunction.Environment env = ruleContext.getAnalysisEnvironment().getSkyframeEnv();
     PackageValue packageNode =
-        (PackageValue) env.getValue(PackageValue.key(constraintSetting.getPackageIdentifier()));
+        (PackageValue) env.getValue(constraintSetting.getPackageIdentifier());
     Preconditions.checkNotNull(
         packageNode,
         "Package '%s' is the package for the current target, and so must have already been loaded.",