Introduce a new SkyValue that merely contains the workspace name. The workspace name is needed for package loading, and so splitting out this computation into a separate skyframe node that can be change-pruned gives us better incrementality; previously we'd need to reload all packages on a WORKSPACE file change.

N.B.
(i) This CL doesn't solve all the other performance issues with //external in Bazel/Blaze since it's still inefficiently used for resolving labels like @foo//bar:baz.
(ii) This CL doesn't address the wasteful invalidation + change pruning of all the packages.

--
PiperOrigin-RevId: 146925369
MOS_MIGRATED_REVID=146925369

7 files changed, 169 insertions, 17 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index a7482bbb60..45fa5cab50 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -491,8 +491,6 @@ public class Package {
 
   /**
    * Returns this package's workspace name.
-   *
-   * <p>Package-private to encourage callers to get their workspace name from a rule, not a package.
    */
   public String getWorkspaceName() {
     return workspaceName;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 311987377e..1dc58df511 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -1289,7 +1289,7 @@ public final class PackageFactory {
    */
   // Used outside of bazel!
   public Package.Builder createPackageFromPreprocessingResult(
-      Package externalPkg,
+      String workspaceName,
       PackageIdentifier packageId,
       Path buildFile,
       Preprocessor.Result preprocessingResult,
@@ -1306,7 +1306,7 @@ public final class PackageFactory {
     AstAfterPreprocessing astAfterPreprocessing = new AstAfterPreprocessing(preprocessingResult,
         buildFileAST, localReporterForParsing);
     return createPackageFromPreprocessingAst(
-        externalPkg,
+        workspaceName,
         packageId,
         buildFile,
         astAfterPreprocessing,
@@ -1326,7 +1326,7 @@ public final class PackageFactory {
   }
 
   public Package.Builder createPackageFromPreprocessingAst(
-      Package externalPkg,
+      String workspaceName,
       PackageIdentifier packageId,
       Path buildFile,
       Preprocessor.AstAfterPreprocessing astAfterPreprocessing,
@@ -1344,7 +1344,7 @@ public final class PackageFactory {
       prefetchGlobs(packageId, astAfterPreprocessing.ast, astAfterPreprocessing.preprocessed,
           buildFile, globber, defaultVisibility, makeEnv);
       return evaluateBuildFile(
-          externalPkg,
+          workspaceName,
           packageId,
           astAfterPreprocessing.ast,
           buildFile,
@@ -1420,7 +1420,7 @@ public final class PackageFactory {
 
     Package result =
         createPackageFromPreprocessingResult(
-                externalPkg,
+                externalPkg.getWorkspaceName(),
                 packageId,
                 buildFile,
                 preprocessingResult,
@@ -1661,7 +1661,7 @@ public final class PackageFactory {
    */
   @VisibleForTesting // used by PackageFactoryApparatus
   public Package.Builder evaluateBuildFile(
-      Package externalPkg,
+      String workspaceName,
       PackageIdentifier packageId,
       BuildFileAST buildFileAST,
       Path buildFilePath,
@@ -1694,7 +1694,7 @@ public final class PackageFactory {
           // set default_visibility once, be reseting the PackageBuilder.defaultVisibilitySet flag.
           .setDefaultVisibilitySet(false)
           .setSkylarkFileDependencies(skylarkFileDependencies)
-          .setWorkspaceName(externalPkg.getWorkspaceName());
+          .setWorkspaceName(workspaceName);
 
       Event.replayEventsOn(eventHandler, pastEvents);
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index e0116fceb4..7b3b1c6e96 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -448,13 +448,13 @@ public class PackageFunction implements SkyFunction {
     if (packageId.equals(Label.EXTERNAL_PACKAGE_IDENTIFIER)) {
       return getExternalPackage(env, packageLookupValue.getRoot());
     }
-    SkyKey externalPackageKey = PackageValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER);
-    PackageValue externalPackage = (PackageValue) env.getValue(externalPackageKey);
-    if (externalPackage == null) {
+    WorkspaceNameValue workspaceNameValue =
+        (WorkspaceNameValue) env.getValue(WorkspaceNameValue.key());
+    if (workspaceNameValue == null) {
       return null;
     }
-    Package externalPkg = externalPackage.getPackage();
-    if (externalPkg.containsErrors()) {
+    String workspaceName = workspaceNameValue.maybeGetName();
+    if (workspaceName == null) {
       throw new PackageFunctionException(
           new BuildFileContainsErrorsException(Label.EXTERNAL_PACKAGE_IDENTIFIER),
           Transience.PERSISTENT);
@@ -502,7 +502,7 @@ public class PackageFunction implements SkyFunction {
             ? astLookupValue.getAST().getStatements() : ImmutableList.<Statement>of();
     CacheEntryWithGlobDeps<Package.Builder> packageBuilderAndGlobDeps =
         loadPackage(
-            externalPkg,
+            workspaceName,
             replacementContents,
             packageId,
             buildFilePath,
@@ -1107,7 +1107,7 @@ public class PackageFunction implements SkyFunction {
    */
   @Nullable
   private CacheEntryWithGlobDeps<Package.Builder> loadPackage(
-      Package externalPkg,
+      String workspaceName,
       @Nullable String replacementContents,
       PackageIdentifier packageId,
       Path buildFilePath,
@@ -1208,7 +1208,7 @@ public class PackageFunction implements SkyFunction {
         SkyframeHybridGlobber skyframeGlobber = new SkyframeHybridGlobber(packageId, packageRoot,
             env, legacyGlobber);
         Package.Builder pkgBuilder = packageFactory.createPackageFromPreprocessingAst(
-            externalPkg, packageId, buildFilePath, astAfterPreprocessing, importResult.importMap,
+            workspaceName, packageId, buildFilePath, astAfterPreprocessing, importResult.importMap,
             importResult.fileDependencies, defaultVisibility, skyframeGlobber);
         Set<SkyKey> globDepsRequested = ImmutableSet.<SkyKey>builder()
             .addAll(globDepsRequestedDuringPreprocessing)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
index 476e313a8a..05c78873b6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -97,6 +97,7 @@ public final class SkyFunctions {
   public static final SkyFunctionName BUILD_INFO_COLLECTION =
       SkyFunctionName.create("BUILD_INFO_COLLECTION");
   public static final SkyFunctionName BUILD_INFO = SkyFunctionName.create("BUILD_INFO");
+  public static final SkyFunctionName WORKSPACE_NAME = SkyFunctionName.create("WORKSPACE_NAME");
   public static final SkyFunctionName WORKSPACE_FILE = SkyFunctionName.create("WORKSPACE_FILE");
   public static final SkyFunctionName COVERAGE_REPORT = SkyFunctionName.create("COVERAGE_REPORT");
   public static final SkyFunctionName REPOSITORY = SkyFunctionName.create("REPOSITORY");
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 25c087de7a..4b7f98c5d5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -417,6 +417,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         configurationFactory, ruleClassProvider));
     map.put(SkyFunctions.CONFIGURATION_FRAGMENT, new ConfigurationFragmentFunction(
         configurationFragments, ruleClassProvider));
+    map.put(SkyFunctions.WORKSPACE_NAME, new WorkspaceNameFunction());
     map.put(SkyFunctions.WORKSPACE_AST, new WorkspaceASTFunction(ruleClassProvider));
     map.put(
         SkyFunctions.WORKSPACE_FILE,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceNameFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceNameFunction.java
new file mode 100644
index 0000000000..f6dc151dfe
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceNameFunction.java
@@ -0,0 +1,50 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.skyframe.SkyFunction;
+import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
+import javax.annotation.Nullable;
+
+/**
+ * {@link SkyFunction} for {@link WorkspaceNameValue}s.
+ *
+ * <p>All transitive errors (e.g. a symlink cycle encountered when consuming the WORKSPACE file)
+ * result in a {@link NoSuchPackageException}.
+ */
+public class WorkspaceNameFunction implements SkyFunction {
+  @Override
+  @Nullable
+  public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+    SkyKey externalPackageKey = PackageValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER);
+    PackageValue externalPackageValue = (PackageValue) env.getValue(externalPackageKey);
+    if (externalPackageValue == null) {
+      return null;
+    }
+    Package externalPackage = externalPackageValue.getPackage();
+    return externalPackage.containsErrors()
+        ? WorkspaceNameValue.withError()
+        : WorkspaceNameValue.withName(externalPackage.getWorkspaceName());
+  }
+
+  @Override
+  @Nullable
+  public String extractTag(SkyKey skyKey) {
+    return null;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceNameValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceNameValue.java
new file mode 100644
index 0000000000..9bfbfed3fc
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceNameValue.java
@@ -0,0 +1,102 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.devtools.build.lib.util.Preconditions;
+import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
+import java.util.Objects;
+import javax.annotation.Nullable;
+
+/**
+ * A value that solely represents the 'name' of a Bazel workspace, as defined in the WORKSPACE file.
+ *
+ * <p>This is a separate value with trivial change pruning so as to not necessitate a dependency
+ * from every {@link PackageValue} to the //external {@link PackageValue}, since such a
+ * hypothetical design would necessiate reloading all packages whenever there's a benign change to
+ * the WORKSPACE file.
+ */
+public class WorkspaceNameValue implements SkyValue {
+  private static final SkyKey KEY =
+      SkyKey.create(SkyFunctions.WORKSPACE_NAME, DummyArgument.INSTANCE);
+  private static final WorkspaceNameValue ERROR = new WorkspaceNameValue(null);
+
+  @Nullable
+  private final String workspaceName;
+
+  private WorkspaceNameValue(@Nullable String workspaceName) {
+    this.workspaceName = workspaceName;
+  }
+
+  /**
+   * Returns the name of the workspace, or {@code null} if there was an error in the WORKSPACE file.
+   */
+  @Nullable
+  public String maybeGetName() {
+    return workspaceName;
+  }
+
+  /** Returns the (singleton) {@link SkyKey} for {@link WorkspaceNameValue}s. */
+  public static SkyKey key() {
+    return KEY;
+  }
+
+  /** Returns a {@link WorkspaceNameValue} for a workspace whose WORKSPACE file is in error. */
+  public static WorkspaceNameValue withError() {
+    return WorkspaceNameValue.ERROR;
+  }
+
+  /** Returns a {@link WorkspaceNameValue} for a workspace with the given name. */
+  public static WorkspaceNameValue withName(String workspaceName) {
+    return new WorkspaceNameValue(Preconditions.checkNotNull(workspaceName));
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (!(obj instanceof WorkspaceNameValue)) {
+      return false;
+    }
+    WorkspaceNameValue other = (WorkspaceNameValue) obj;
+    return Objects.equals(workspaceName, other.workspaceName);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(workspaceName);
+  }
+
+  @Override
+  public String toString() {
+    return String.format("WorkspaceNameValue[name=%s]", workspaceName);
+  }
+
+  /** Singleton class used as the {@link SkyKey#argument} for {@link WorkspaceNameValue#key}. */
+  public static final class DummyArgument {
+    public static final int HASHCODE = DummyArgument.class.getCanonicalName().hashCode();
+    public static final DummyArgument INSTANCE = new DummyArgument();
+
+    private DummyArgument() {
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      return obj instanceof DummyArgument;
+    }
+
+    @Override
+    public int hashCode() {
+      return HASHCODE;
+    }
+  }
+}