Cache BUILD file AST parsing results instead of preprocessing results (the former uses the latter). This way we parse BUILD files exactly once.

This is part of a series of changes with the net result being that we open, read, and parse each BUILD file exactly once.

--
MOS_MIGRATED_REVID=105528075

4 files changed, 97 insertions, 53 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 59060c7154..7fcb29d5a6 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
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.GlobCache.BadGlobException;
 import com.google.devtools.build.lib.packages.License.DistributionType;
+import com.google.devtools.build.lib.packages.Preprocessor.AstAfterPreprocessing;
 import com.google.devtools.build.lib.syntax.AssignmentStatement;
 import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
@@ -996,15 +997,43 @@ public final class PackageFactory {
       CachingPackageLocator locator,
       RuleVisibility defaultVisibility,
       Globber globber) throws InterruptedException {
-    StoredEventHandler localReporter = new StoredEventHandler();
+    StoredEventHandler localReporterForParsing = new StoredEventHandler();
     // Run the lexer and parser with a local reporter, so that errors from other threads do not
-    // show up below. Merge the local and global reporters afterwards.
+    // show up below.
+    BuildFileAST buildFileAST = parseBuildFile(packageId, preprocessingResult.result,
+        preludeStatements, localReporterForParsing);
+    AstAfterPreprocessing astAfterPreprocessing =
+        new AstAfterPreprocessing(preprocessingResult, buildFileAST, localReporterForParsing);
+    return createPackageFromPreprocessingAst(
+        externalPkg,
+        packageId,
+        buildFile,
+        astAfterPreprocessing,
+        imports,
+        skylarkFileDependencies,
+        defaultVisibility,
+        globber);
+  }
+
+  public static BuildFileAST parseBuildFile(PackageIdentifier packageId, ParserInputSource in,
+      List<Statement> preludeStatements, EventHandler eventHandler) {
     // Logged messages are used as a testability hook tracing the parsing progress
     LOG.fine("Starting to parse " + packageId);
     BuildFileAST buildFileAST = BuildFileAST.parseBuildFile(
-        preprocessingResult.result, preludeStatements, localReporter, false);
+        in, preludeStatements, eventHandler, false);
     LOG.fine("Finished parsing of " + packageId);
+    return buildFileAST;
+  }
 
+  public Package.LegacyBuilder createPackageFromPreprocessingAst(
+      Package externalPkg,
+      PackageIdentifier packageId,
+      Path buildFile,
+      Preprocessor.AstAfterPreprocessing astAfterPreprocessing,
+      Map<PathFragment, Extension> imports,
+      ImmutableList<Label> skylarkFileDependencies,
+      RuleVisibility defaultVisibility,
+      Globber globber) throws InterruptedException {
     MakeEnvironment.Builder makeEnv = new MakeEnvironment.Builder();
     if (platformSetRegexps != null) {
       makeEnv.setPlatformSetRegexps(platformSetRegexps);
@@ -1012,17 +1041,17 @@ public final class PackageFactory {
     try {
       // At this point the package is guaranteed to exist.  It may have parse or
       // evaluation errors, resulting in a diminished number of rules.
-      prefetchGlobs(packageId, buildFileAST, preprocessingResult.preprocessed,
+      prefetchGlobs(packageId, astAfterPreprocessing.ast, astAfterPreprocessing.preprocessed,
           buildFile, globber, defaultVisibility, makeEnv);
       return evaluateBuildFile(
           externalPkg,
           packageId,
-          buildFileAST,
+          astAfterPreprocessing.ast,
           buildFile,
           globber,
-          Iterables.concat(preprocessingEvents, localReporter.getEvents()),
+          astAfterPreprocessing.allEvents,
           defaultVisibility,
-          preprocessingResult.containsErrors,
+          astAfterPreprocessing.containsPreprocessingErrors,
           makeEnv,
           imports,
           skylarkFileDependencies);
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 843fd9efd1..57a2d848df 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
@@ -14,8 +14,11 @@
 package com.google.devtools.build.lib.packages;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.PackageFactory.Globber;
+import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -160,4 +163,23 @@ public interface Preprocessor {
       Environment.Frame globals,
       Set<String> ruleNames)
     throws IOException, InterruptedException;
+
+  /** The result of parsing a preprocessed BUILD file. */
+  static class AstAfterPreprocessing {
+    public final boolean preprocessed;
+    public final boolean containsPreprocessingErrors;
+    public final BuildFileAST ast;
+    public final boolean containsAstParsingErrors;
+    public final Iterable<Event> allEvents;
+
+    public AstAfterPreprocessing(Result preprocessingResult, BuildFileAST ast,
+        StoredEventHandler astParsingEventHandler) {
+      this.ast = ast;
+      this.preprocessed = preprocessingResult.preprocessed;
+      this.containsPreprocessingErrors = preprocessingResult.containsErrors;
+      this.containsAstParsingErrors = astParsingEventHandler.hasErrors();
+      this.allEvents = Iterables.concat(
+          preprocessingResult.events, astParsingEventHandler.getEvents());
+    }
+  }
 }
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 f46cf2b13d..6ba2775e5a 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
@@ -38,7 +38,7 @@ import com.google.devtools.build.lib.packages.Package.LegacyBuilder;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.PackageFactory.Globber;
 import com.google.devtools.build.lib.packages.Preprocessor;
-import com.google.devtools.build.lib.packages.Preprocessor.Result;
+import com.google.devtools.build.lib.packages.Preprocessor.AstAfterPreprocessing;
 import com.google.devtools.build.lib.packages.RuleVisibility;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.profiler.Profiler;
@@ -84,7 +84,7 @@ public class PackageFunction implements SkyFunction {
   private final PackageFactory packageFactory;
   private final CachingPackageLocator packageLocator;
   private final Cache<PackageIdentifier, Package.LegacyBuilder> packageFunctionCache;
-  private final Cache<PackageIdentifier, Preprocessor.Result> preprocessCache;
+  private final Cache<PackageIdentifier, Preprocessor.AstAfterPreprocessing> astCache;
   private final AtomicBoolean showLoadingProgress;
   private final AtomicInteger numPackagesLoaded;
   private final Profiler profiler = Profiler.instance();
@@ -101,7 +101,7 @@ public class PackageFunction implements SkyFunction {
       CachingPackageLocator pkgLocator,
       AtomicBoolean showLoadingProgress,
       Cache<PackageIdentifier, LegacyBuilder> packageFunctionCache,
-      Cache<PackageIdentifier, Result> preprocessCache,
+      Cache<PackageIdentifier, AstAfterPreprocessing> astCache,
       AtomicInteger numPackagesLoaded,
       @Nullable SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining) {
     this.skylarkImportLookupFunctionForInlining = skylarkImportLookupFunctionForInlining;
@@ -113,7 +113,7 @@ public class PackageFunction implements SkyFunction {
     this.packageLocator = pkgLocator;
     this.showLoadingProgress = showLoadingProgress;
     this.packageFunctionCache = packageFunctionCache;
-    this.preprocessCache = preprocessCache;
+    this.astCache = astCache;
     this.numPackagesLoaded = numPackagesLoaded;
   }
 
@@ -515,32 +515,24 @@ public class PackageFunction implements SkyFunction {
     return new PackageValue(pkg);
   }
 
-  // TODO(bazel-team): this should take the AST so we don't parse the file twice.
   @Nullable
   private SkylarkImportResult discoverSkylarkImports(
       Path buildFilePath,
       PathFragment buildFileFragment,
       PackageIdentifier packageId,
-      Environment env,
-      ParserInputSource inputSource,
-      List<Statement> preludeStatements)
+      AstAfterPreprocessing astAfterPreprocessing,
+      Environment env)
       throws PackageFunctionException, InterruptedException {
-    StoredEventHandler eventHandler = new StoredEventHandler();
-    BuildFileAST buildFileAST =
-        BuildFileAST.parseBuildFile(
-            inputSource,
-            preludeStatements,
-            eventHandler,
-            /* parse python */ false);
     SkylarkImportResult importResult;
-    if (eventHandler.hasErrors()) {
+    if (astAfterPreprocessing.containsAstParsingErrors) {
       importResult =
           new SkylarkImportResult(
               ImmutableMap.<PathFragment, Extension>of(),
               ImmutableList.<Label>of());
     } else {
       importResult =
-          fetchImportsFromBuildFile(buildFilePath, buildFileFragment, packageId, buildFileAST, env);
+          fetchImportsFromBuildFile(buildFilePath, buildFileFragment, packageId,
+              astAfterPreprocessing.ast, env);
     }
 
     return importResult;
@@ -868,17 +860,18 @@ public class PackageFunction implements SkyFunction {
       try {
         Globber globber = packageFactory.createLegacyGlobber(buildFilePath.getParentDirectory(),
             packageId, packageLocator);
-        Preprocessor.Result preprocessingResult = preprocessCache.getIfPresent(packageId);
-        if (preprocessingResult == null) {
+        AstAfterPreprocessing astAfterPreprocessing = astCache.getIfPresent(packageId);
+        if (astAfterPreprocessing == null) {
           if (showLoadingProgress.get()) {
             env.getListener().handle(Event.progress("Loading package: " + packageId));
           }
-          // Even though we only open and read the file on a cache miss, note that the BUILD is
-          // still parsed two times. Also, the preprocessor may suboptimally open and read it again
-          // anyway.
-          ParserInputSource inputSource;
+          Preprocessor.Result preprocessingResult;
           if (replacementContents == null) {
             long buildFileSize = Preconditions.checkNotNull(buildFileValue, packageId).getSize();
+            // Even though we only open and read the file on a cache miss, note that the BUILD is
+            // still parsed two times. Also, the preprocessor may suboptimally open and read it
+            // again anyway.
+            ParserInputSource inputSource;
             try {
               inputSource = ParserInputSource.create(buildFilePath, buildFileSize);
             } catch (IOException e) {
@@ -904,31 +897,32 @@ public class PackageFunction implements SkyFunction {
                 ParserInputSource.create(replacementContents, buildFilePath.asFragment());
             preprocessingResult = Preprocessor.Result.noPreprocessing(replacementSource);
           }
-          preprocessCache.put(packageId, preprocessingResult);
+          StoredEventHandler astParsingEventHandler = new StoredEventHandler();
+          BuildFileAST ast = PackageFactory.parseBuildFile(packageId, preprocessingResult.result,
+              preludeStatements, astParsingEventHandler);
+          astAfterPreprocessing = new AstAfterPreprocessing(preprocessingResult, ast,
+              astParsingEventHandler);
+          astCache.put(packageId, astAfterPreprocessing);
         }
-
         SkylarkImportResult importResult;
         try {
           importResult = discoverSkylarkImports(
-                  buildFilePath,
-                  buildFileFragment,
-                  packageId,
-                  env,
-                  preprocessingResult.result,
-                  preludeStatements);
+              buildFilePath,
+              buildFileFragment,
+              packageId,
+              astAfterPreprocessing,
+              env);
         } catch (PackageFunctionException | InterruptedException e) {
-          preprocessCache.invalidate(packageId);
+          astCache.invalidate(packageId);
           throw e;
         }
         if (importResult == null) {
           return null;
         }
-        preprocessCache.invalidate(packageId);
-
-        pkgBuilder = packageFactory.createPackageFromPreprocessingResult(externalPkg, packageId,
-            buildFilePath, preprocessingResult, preprocessingResult.events, preludeStatements,
-            importResult.importMap, importResult.fileDependencies, packageLocator,
-            defaultVisibility, globber);
+        astCache.invalidate(packageId);
+        pkgBuilder = packageFactory.createPackageFromPreprocessingAst(externalPkg, packageId,
+            buildFilePath, astAfterPreprocessing, importResult.importMap,
+            importResult.fileDependencies, defaultVisibility, globber);
         numPackagesLoaded.incrementAndGet();
         packageFunctionCache.put(packageId, pkgBuilder);
       } finally {
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 a75e6c53a3..0b5c5cb707 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
@@ -83,7 +83,7 @@ import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Package.LegacyBuilder;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.Preprocessor;
-import com.google.devtools.build.lib.packages.Preprocessor.Result;
+import com.google.devtools.build.lib.packages.Preprocessor.AstAfterPreprocessing;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.RuleVisibility;
 import com.google.devtools.build.lib.packages.Target;
@@ -181,8 +181,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   // [skyframe-loading]
   private final Cache<PackageIdentifier, Package.LegacyBuilder> packageFunctionCache =
       newPkgFunctionCache();
-  private final Cache<PackageIdentifier, Preprocessor.Result> preprocessCache =
-      newPreprocessCache();
+  private final Cache<PackageIdentifier, AstAfterPreprocessing> astCache = newAstCache();
 
   private final AtomicInteger numPackagesLoaded = new AtomicInteger(0);
 
@@ -343,7 +342,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
             packageManager,
             showLoadingProgress,
             packageFunctionCache,
-            preprocessCache,
+            astCache,
             numPackagesLoaded,
             ruleClassProvider));
     map.put(SkyFunctions.PACKAGE_ERROR, new PackageErrorFunction());
@@ -392,7 +391,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       PackageManager packageManager,
       AtomicBoolean showLoadingProgress,
       Cache<PackageIdentifier, LegacyBuilder> packageFunctionCache,
-      Cache<PackageIdentifier, Result> preprocessCache,
+      Cache<PackageIdentifier, AstAfterPreprocessing> astCache,
       AtomicInteger numPackagesLoaded,
       RuleClassProvider ruleClassProvider) {
     return new PackageFunction(
@@ -400,7 +399,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         packageManager,
         showLoadingProgress,
         packageFunctionCache,
-        preprocessCache,
+        astCache,
         numPackagesLoaded,
         null);
   }
@@ -636,7 +635,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     return CacheBuilder.newBuilder().build();
   }
 
-  protected Cache<PackageIdentifier, Preprocessor.Result> newPreprocessCache() {
+  protected Cache<PackageIdentifier, Preprocessor.AstAfterPreprocessing> newAstCache() {
     return CacheBuilder.newBuilder().build();
   }
 
@@ -878,7 +877,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
     // If the PackageFunction was interrupted, there may be stale entries here.
     packageFunctionCache.invalidateAll();
-    preprocessCache.invalidateAll();
+    astCache.invalidateAll();
     numPackagesLoaded.set(0);
 
     // Reset the stateful SkyframeCycleReporter, which contains cycles from last run.