Batch SkylarkImportLookupValue calls instead of doing them serially. Also throw errors more eagerly in SkylarkImportLookupFunction -- don't try to request deps if the ast is in error.

--
MOS_MIGRATED_REVID=103607939

2 files changed, 100 insertions, 59 deletions

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 5e461fb113..9676aa7c35 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
@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -554,6 +555,25 @@ public class PackageFunction implements SkyFunction {
     return importResult;
   }
 
+  static SkyKey getImportKey(
+      Map.Entry<Location, PathFragment> entry,
+      PathFragment preludePath,
+      PathFragment buildFileFragment,
+      PackageIdentifier packageId)
+      throws ASTLookupInputException {
+    PathFragment importFile = entry.getValue();
+    // HACK: The prelude sometimes contains load() statements, which need to be resolved
+    // relative to the prelude file. However, we don't have a good way to tell "this should come
+    // from the main repository" in a load() statement, and we don't have a good way to tell if
+    // a load() statement comes from the prelude, since we just prepend those statements before
+    // the actual BUILD file. So we use this evil .endsWith() statement to figure it out.
+    RepositoryName repository =
+        entry.getKey().getPath().endsWith(preludePath)
+            ? PackageIdentifier.DEFAULT_REPOSITORY_NAME
+            : packageId.getRepository();
+    return SkylarkImportLookupValue.key(repository, buildFileFragment, importFile);
+  }
+
   /**
    * Fetch the skylark loads for this BUILD file. If any of them haven't been computed yet,
    * returns null.
@@ -567,44 +587,63 @@ public class PackageFunction implements SkyFunction {
       Environment env)
       throws PackageFunctionException {
     ImmutableMap<Location, PathFragment> imports = buildFileAST.getImports();
-    Map<PathFragment, Extension> importMap = new HashMap<>();
+    Map<PathFragment, Extension> importMap = Maps.newHashMapWithExpectedSize(imports.size());
     ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
-    try {
-      for (Map.Entry<Location, PathFragment> entry : imports.entrySet()) {
-        PathFragment importFile = entry.getValue();
-        // HACK: The prelude sometimes contains load() statements, which need to be resolved
-        // relative to the prelude file. However, we don't have a good way to tell "this should come
-        // from the main repository" in a load() statement, and we don't have a good way to tell if
-        // a load() statement comes from the prelude, since we just prepend those statements before
-        // the actual BUILD file. So we use this evil .endsWith() statement to figure it out.
-        RepositoryName repository =
-            entry.getKey().getPath().endsWith(preludePath)
-                ? PackageIdentifier.DEFAULT_REPOSITORY_NAME : packageId.getRepository();
-        SkyKey importsLookupKey = SkylarkImportLookupValue.key(
-            repository, buildFileFragment, importFile);
-        SkylarkImportLookupValue importLookupValue = (SkylarkImportLookupValue)
-            env.getValueOrThrow(importsLookupKey, SkylarkImportFailedException.class,
-                InconsistentFilesystemException.class, ASTLookupInputException.class,
+    Map<SkyKey, PathFragment> skylarkImports = Maps.newHashMapWithExpectedSize(imports.size());
+    for (Map.Entry<Location, PathFragment> entry : imports.entrySet()) {
+      try {
+        skylarkImports.put(
+            getImportKey(entry, preludePath, buildFileFragment, packageId), entry.getValue());
+      } catch (ASTLookupInputException e) {
+        // The load syntax is bad in the BUILD file so BuildFileContainsErrorsException is OK.
+        throw new PackageFunctionException(
+            new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
+      }
+    }
+    Map<
+            SkyKey,
+            ValueOrException4<
+                SkylarkImportFailedException, InconsistentFilesystemException,
+                ASTLookupInputException, BuildFileNotFoundException>>
+        skylarkImportMap =
+            env.getValuesOrThrow(
+                skylarkImports.keySet(),
+                SkylarkImportFailedException.class,
+                InconsistentFilesystemException.class,
+                ASTLookupInputException.class,
                 BuildFileNotFoundException.class);
-        if (importLookupValue != null) {
-          importMap.put(importFile, importLookupValue.getEnvironmentExtension());
-          fileDependencies.add(importLookupValue.getDependency());
-        }
+    for (Map.Entry<
+            SkyKey,
+            ValueOrException4<
+                SkylarkImportFailedException, InconsistentFilesystemException,
+                ASTLookupInputException, BuildFileNotFoundException>>
+        entry : skylarkImportMap.entrySet()) {
+      SkylarkImportLookupValue importLookupValue;
+      try {
+        importLookupValue = (SkylarkImportLookupValue) entry.getValue().get();
+      } catch (SkylarkImportFailedException e) {
+        env.getListener().handle(Event.error(Location.fromFile(buildFilePath), e.getMessage()));
+        throw new PackageFunctionException(
+            new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
+      } catch (InconsistentFilesystemException e) {
+        throw new PackageFunctionException(
+            new InternalInconsistentFilesystemException(packageId, e), Transience.PERSISTENT);
+      } catch (ASTLookupInputException e) {
+        // The load syntax is bad in the BUILD file so BuildFileContainsErrorsException is OK.
+        throw new PackageFunctionException(
+            new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
+      } catch (BuildFileNotFoundException e) {
+        throw new PackageFunctionException(e, Transience.PERSISTENT);
+      }
+      if (importLookupValue == null) {
+        Preconditions.checkState(env.valuesMissing(), entry);
+      } else {
+        importMap.put(
+            skylarkImports.get(entry.getKey()), importLookupValue.getEnvironmentExtension());
+        fileDependencies.add(importLookupValue.getDependency());
       }
-    } catch (SkylarkImportFailedException e) {
-      env.getListener().handle(Event.error(Location.fromFile(buildFilePath), e.getMessage()));
-      throw new PackageFunctionException(
-          new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
-    } catch (InconsistentFilesystemException e) {
-      throw new PackageFunctionException(
-          new InternalInconsistentFilesystemException(packageId, e), Transience.PERSISTENT);
-    } catch (ASTLookupInputException e) {
-      // The load syntax is bad in the BUILD file so BuildFileContainsErrorsException is OK.
-      throw new PackageFunctionException(
-          new BuildFileContainsErrorsException(packageId, e.getMessage()), Transience.PERSISTENT);
-    } catch (BuildFileNotFoundException e) {
-      throw new PackageFunctionException(e, Transience.PERSISTENT);
     }
+
     if (env.valuesMissing()) {
       // There are unavailable Skylark dependencies.
       return null;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index 29318f359c..de09f75d0a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -13,7 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -36,7 +38,6 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
-import java.util.HashMap;
 import java.util.Map;
 
 /**
@@ -77,42 +78,43 @@ public class SkylarkImportLookupFunction implements SkyFunction {
       throw new SkylarkImportLookupFunctionException(SkylarkImportFailedException.noFile(file));
     }
 
-    Map<PathFragment, Extension> importMap = new HashMap<>();
-    ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
     BuildFileAST ast = astLookupValue.getAST();
-    // TODO(bazel-team): Refactor this code and PackageFunction to reduce code duplications.
+    if (ast.containsErrors()) {
+      throw new SkylarkImportLookupFunctionException(
+          SkylarkImportFailedException.skylarkErrors(file));
+    }
+
+    Map<Location, PathFragment> astImports = ast.getImports();
+    Map<PathFragment, Extension> importMap = Maps.newHashMapWithExpectedSize(astImports.size());
+    ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
+    Map<SkyKey, PathFragment> skylarkImports = Maps.newHashMapWithExpectedSize(astImports.size());
     for (Map.Entry<Location, PathFragment> entry : ast.getImports().entrySet()) {
       try {
-        PathFragment importFile = entry.getValue();
-        // HACK: The prelude sometimes contains load() statements, which need to be resolved
-        // relative to the prelude file. However, we don't have a good way to tell "this should come
-        // from the main repository" in a load() statement, and we don't have a good way to tell if
-        // a load() statement comes from the prelude, since we just prepend those statements before
-        // the actual BUILD file. So we use this evil .endsWith() statement to figure it out.
-        RepositoryName repository =
-            entry.getKey().getPath().endsWith(ruleClassProvider.getPreludePath())
-                ? PackageIdentifier.DEFAULT_REPOSITORY_NAME : arg.getRepository();
-        SkyKey importsLookupKey = SkylarkImportLookupValue.key(repository, file, importFile);
-        SkylarkImportLookupValue importsLookupValue;
-        importsLookupValue = (SkylarkImportLookupValue) env.getValueOrThrow(
-            importsLookupKey, ASTLookupInputException.class);
-        if (importsLookupValue != null) {
-          importMap.put(importFile, importsLookupValue.getEnvironmentExtension());
-          fileDependencies.add(importsLookupValue.getDependency());
-        }
+        skylarkImports.put(
+            PackageFunction.getImportKey(entry, ruleClassProvider.getPreludePath(), file, arg),
+            entry.getValue());
       } catch (ASTLookupInputException e) {
         throw new SkylarkImportLookupFunctionException(e, Transience.PERSISTENT);
       }
     }
-    Label label = pathFragmentToLabel(arg.getRepository(), file, env);
+    Map<SkyKey, SkyValue> skylarkImportMap = env.getValues(skylarkImports.keySet());
+
     if (env.valuesMissing()) {
       // This means some imports are unavailable.
       return null;
     }
+    for (Map.Entry<SkyKey, SkyValue> entry : skylarkImportMap.entrySet()) {
+      SkylarkImportLookupValue importLookupValue = (SkylarkImportLookupValue) entry.getValue();
+      importMap.put(
+          skylarkImports.get(entry.getKey()), importLookupValue.getEnvironmentExtension());
+      fileDependencies.add(importLookupValue.getDependency());
+    }
 
-    if (ast.containsErrors()) {
-      throw new SkylarkImportLookupFunctionException(SkylarkImportFailedException.skylarkErrors(
-          file));
+    Label label = pathFragmentToLabel(arg.getRepository(), file, env);
+
+    if (label == null) {
+      Preconditions.checkState(env.valuesMissing(), "label null but no missing for %s", file);
+      return null;
     }
 
     // Skylark UserDefinedFunction-s in that file will share this function definition Environment,