When getting package roots for exec paths, don't start with the file, which is guaranteed not to be a package. Instead, start with its parent. This will be faster and take less memory.

--
MOS_MIGRATED_REVID=107725767

7 files changed, 22 insertions, 13 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
index f1f8f6efe5..d248d5a357 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
@@ -382,7 +382,7 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
         unresolvedPaths.add(execPath);
       }
     }
-    Map<PathFragment, Root> sourceRoots = resolver.findPackageRoots(unresolvedPaths);
+    Map<PathFragment, Root> sourceRoots = resolver.findPackageRootsForFiles(unresolvedPaths);
     // We are missing some dependencies. We need to rerun this method later.
     if (sourceRoots == null) {
       return null;
@@ -470,7 +470,8 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
       }
       return result;
     } else {
-      Map<PathFragment, Root> sourceRoots = resolver.findPackageRoots(Lists.newArrayList(execPath));
+      Map<PathFragment, Root> sourceRoots = resolver.findPackageRootsForFiles(
+          Lists.newArrayList(execPath));
       if (sourceRoots == null || sourceRoots.get(execPath) == null) {
         return null;
       }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java
index 5995b4e7da..d93b7b9f33 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java
@@ -16,7 +16,7 @@ package com.google.devtools.build.lib.actions;
 
 /**
  * Exception signaling an error occurred determining package roots. See
- * {@link PackageRootResolver#findPackageRoots(Iterable)} for further details.
+ * {@link PackageRootResolver#findPackageRootsForFiles(Iterable)} for further details.
  */
 public class PackageRootResolutionException extends Exception {
 
diff --git a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
index d7b00875b0..6f058a5728 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
@@ -29,13 +29,14 @@ public interface PackageRootResolver {
    * Returns mapping from execPath to Root. Root will be null if the path has no containing
    * package.
    *
-   * @param execPaths the paths to find {@link Root}s for
+   * @param execPaths the paths to find {@link Root}s for. The search for a containing package will
+   *    start with the path's parent directory, since the path is assumed to be a file.
    * @return mappings from {@code execPath} to {@link Root}, or null if for some reason we
    *    cannot determine the result at this time (such as when used within a SkyFunction)
    * @throws PackageRootResolutionException if unable to determine package roots or lack thereof,
    *    typically caused by exceptions encountered while attempting to locate BUILD files
    */
   @Nullable
-  Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
+  Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> execPaths)
       throws PackageRootResolutionException;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
index 06ea7bd056..8fed1a7993 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
@@ -38,8 +38,8 @@ public final class SkyframePackageRootResolver implements PackageRootResolver {
   }
 
   @Override
-  public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
+  public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> execPaths)
       throws PackageRootResolutionException {
     return executor.getArtifactRoots(eventHandler, execPaths);
   }
-}
\ No newline at end of file
+}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 8ed3b21bbf..92d6448255 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -1836,7 +1836,9 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
       Root sysrootRoot;
       try {
         sysrootRoot = Iterables.getOnlyElement(
-          resolver.findPackageRoots(ImmutableList.of(getSysroot())).entrySet()).getValue();
+          resolver.findPackageRootsForFiles(
+              // See doc of findPackageRootsForFiles for why we need a getChild here.
+              ImmutableList.of(getSysroot().getChild("dummy_child"))).entrySet()).getValue();
       } catch (PackageRootResolutionException prre) {
         throw new ViewCreationFailedException("Failed to determine sysroot", prre);
       }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 6e1b6c3e08..76bb36e18d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -259,15 +259,18 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     }
 
     @Override
-    public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
+    public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> execPaths)
         throws PackageRootResolutionException {
       Preconditions.checkState(keysRequested.isEmpty(),
           "resolver should only be called once: %s %s", keysRequested, execPaths);
       // Create SkyKeys list based on execPaths.
       Map<PathFragment, SkyKey> depKeys = new HashMap<>();
       for (PathFragment path : execPaths) {
+        PathFragment parent = Preconditions.checkNotNull(
+            path.getParentDirectory(), "Must pass in files, not root directory");
+        Preconditions.checkArgument(!parent.isAbsolute(), path);
         SkyKey depKey =
-            ContainingPackageLookupValue.key(PackageIdentifier.createInDefaultRepo(path));
+            ContainingPackageLookupValue.key(PackageIdentifier.createInDefaultRepo(parent));
         depKeys.put(path, depKey);
         keysRequested.add(depKey);
       }
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 4b31a6673b..9c1dce2132 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
@@ -699,9 +699,11 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       Iterable<PathFragment> execPaths) throws PackageRootResolutionException {
     final List<SkyKey> packageKeys = new ArrayList<>();
     for (PathFragment execPath : execPaths) {
-      Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
+      PathFragment parent = Preconditions.checkNotNull(
+          execPath.getParentDirectory(), "Must pass in files, not root directory");
+      Preconditions.checkArgument(!parent.isAbsolute(), execPath);
       packageKeys.add(ContainingPackageLookupValue.key(
-          PackageIdentifier.createInDefaultRepo(execPath)));
+          PackageIdentifier.createInDefaultRepo(parent)));
     }
 
     EvaluationResult<ContainingPackageLookupValue> result;
@@ -727,7 +729,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     Map<PathFragment, Root> roots = new HashMap<>();
     for (PathFragment execPath : execPaths) {
       ContainingPackageLookupValue value = result.get(ContainingPackageLookupValue.key(
-          PackageIdentifier.createInDefaultRepo(execPath)));
+          PackageIdentifier.createInDefaultRepo(execPath.getParentDirectory())));
       if (value.hasContainingPackage()) {
         roots.put(execPath, Root.asSourceRoot(value.getContainingPackageRoot()));
       } else {