Clear up some confusion about glob prefetching (the old comment was wrong). Also add some TODOs for potentially improving package loading performance.

This CL has no semantic impact.

--
MOS_MIGRATED_REVID=113301656

5 files changed, 33 insertions, 4 deletions

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 f8ad17c053..8fa921918b 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
@@ -1586,11 +1586,17 @@ public final class PackageFactory {
       boolean wasPreprocessed, Path buildFilePath, Globber globber,
       RuleVisibility defaultVisibility, MakeEnvironment.Builder pkgMakeEnv)
       throws InterruptedException {
-    if (wasPreprocessed) {
-      // No point in prefetching globs here: preprocessing implies eager evaluation
-      // of all globs.
+    if (wasPreprocessed && preprocessorFactory.considersGlobs()) {
+      // All the globs have either already been evaluated and they aren't in the ast anymore, or
+      // they are in the ast but the globber has been evaluating them lazily and so there is no
+      // point in prefetching them again.
       return;
     }
+    // TODO(bazel-team): It may be wasteful to evaluate the BUILD file here, only to throw away the
+    // result. It may be better to first scan the ast and see if there are even possibly any globs
+    // at all. Additionally, it's wasteful to execute Skylark code that cannot invoke globs. So one
+    // strategy would be to crawl the ast and tag statements whose execution cannot involve globs -
+    // these can be executed and their impact on the resulting package can be saved.
     try (Mutability mutability = Mutability.create("prefetchGlobs for %s", packageId)) {
       Environment pkgEnv = Environment.builder(mutability)
           .setGlobals(Environment.BUILD)
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
index 53e383454c..1bf5654348 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
@@ -68,6 +68,14 @@ public interface Preprocessor {
     boolean isStillValid();
 
     /**
+     * Returns whether all globs encountered during {@link Preprocessor#preprocess} will be passed
+     * along to the {@link Globber} given there (which then executes them asynchronously). If this
+     * is not the case, then e.g. prefetching globs during normal BUILD file evaluation may be
+     * profitable.
+     */
+    boolean considersGlobs();
+
+    /**
      * Returns a Preprocessor instance capable of preprocessing a BUILD file independently (e.g. it
      * ought to be fine to call {@link #getPreprocessor} for each BUILD file).
      */
@@ -87,6 +95,11 @@ public interface Preprocessor {
       }
 
       @Override
+      public boolean considersGlobs() {
+        return false;
+      }
+
+      @Override
       public Preprocessor getPreprocessor() {
         return null;
       }
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 0a69fdd50a..4d8596f27a 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
@@ -894,7 +894,7 @@ public class PackageFunction implements SkyFunction {
         // If the globber was used to evaluate globs during preprocessing, it's important that we
         // reuse that globber during BUILD file evaluation for two reasons: (i) correctness, since
         // Skyframe deps are added after the fact (ii) performance, in the case that globs were
-        // fetched lazily during preprocessing.
+        // fetched lazily during preprocessing. See Preprocessor.Factory#considersGlobs.
         Globber globber = astAfterPreprocessing.globber != null
             ? astAfterPreprocessing.globber
             : packageFactory.createLegacyGlobber(buildFilePath.getParentDirectory(),
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PreprocessorUtils.java b/src/test/java/com/google/devtools/build/lib/packages/util/PreprocessorUtils.java
index b4f674b41c..7bfb31ec2d 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PreprocessorUtils.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PreprocessorUtils.java
@@ -53,6 +53,11 @@ public class PreprocessorUtils {
       }
 
       @Override
+      public boolean considersGlobs() {
+        return false;
+      }
+
+      @Override
       @Nullable
       public Preprocessor getPreprocessor() {
         return preprocessor;
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/SubincludePreprocessor.java b/src/test/java/com/google/devtools/build/lib/packages/util/SubincludePreprocessor.java
index b192237d99..74e615f812 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/SubincludePreprocessor.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/SubincludePreprocessor.java
@@ -59,6 +59,11 @@ public class SubincludePreprocessor implements Preprocessor {
         }
 
         @Override
+        public boolean considersGlobs() {
+          return false;
+        }
+
+        @Override
         public Preprocessor getPreprocessor() {
           return new SubincludePreprocessor(fileSystem, loc);
         }