Have GlobFunction make use of the assumption that the glob's package exists by having it not declare a dep on the PackageLookupValue for the package. This optimization means that a BUILD file edit doesn't (necessarily) invalidate all the globs in the package; the PackageLookupValue node would get change-pruned but we still pay the very small cost of invalidating unnecessarily.

Also slightly improve variable naming in GlobFunctionTest.

--
MOS_MIGRATED_REVID=113799936

4 files changed, 40 insertions, 35 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java
index ec595447a9..1687a819fa 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java
@@ -17,6 +17,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.StringCanonicalizer;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.io.Serializable;
@@ -32,6 +33,7 @@ import java.util.Objects;
 @ThreadSafe
 public final class GlobDescriptor implements Serializable {
   final PackageIdentifier packageId;
+  final Path packageRoot;
   final PathFragment subdir;
   final String pattern;
   final boolean excludeDirs;
@@ -40,15 +42,17 @@ public final class GlobDescriptor implements Serializable {
    * Constructs a GlobDescriptor.
    *
    * @param packageId the name of the owner package (must be an existing package)
+   * @param packageRoot the package root of {@code packageId}
    * @param subdir the subdirectory being looked at (must exist and must be a directory. It's
    *               assumed that there are no other packages between {@code packageName} and
    *               {@code subdir}.
    * @param pattern a valid glob pattern
    * @param excludeDirs true if directories should be excluded from results
    */
-  public GlobDescriptor(PackageIdentifier packageId, PathFragment subdir, String pattern,
-      boolean excludeDirs) {
+  public GlobDescriptor(PackageIdentifier packageId, Path packageRoot, PathFragment subdir,
+      String pattern, boolean excludeDirs) {
     this.packageId = Preconditions.checkNotNull(packageId);
+    this.packageRoot = Preconditions.checkNotNull(packageRoot);
     this.subdir = Preconditions.checkNotNull(subdir);
     this.pattern = Preconditions.checkNotNull(StringCanonicalizer.intern(pattern));
     this.excludeDirs = excludeDirs;
@@ -56,8 +60,9 @@ public final class GlobDescriptor implements Serializable {
 
   @Override
   public String toString() {
-    return String.format("<GlobDescriptor packageName=%s subdir=%s pattern=%s excludeDirs=%s>",
-        packageId, subdir, pattern, excludeDirs);
+    return String.format(
+        "<GlobDescriptor packageName=%s packageRoot=%s subdir=%s pattern=%s excludeDirs=%s>",
+        packageId, packageRoot, subdir, pattern, excludeDirs);
   }
 
   /**
@@ -70,6 +75,13 @@ public final class GlobDescriptor implements Serializable {
   }
 
   /**
+   * Returns the package root of {@code getPackageId()}.
+   */
+  public Path getPackageRoot() {
+    return packageRoot;
+  }
+
+  /**
    * Returns the subdirectory of the package under consideration.
    */
   public PathFragment getSubdir() {
@@ -107,7 +119,8 @@ public final class GlobDescriptor implements Serializable {
       return false;
     }
     GlobDescriptor other = (GlobDescriptor) obj;
-    return packageId.equals(other.packageId) && subdir.equals(other.subdir)
-        && pattern.equals(other.pattern) && excludeDirs == other.excludeDirs;
+    return packageId.equals(other.packageId) && packageRoot.equals(other.packageRoot)
+        && subdir.equals(other.subdir) && pattern.equals(other.pattern)
+        && excludeDirs == other.excludeDirs;
   }
 }
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
index 5520c23621..50b7c294c7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
@@ -18,7 +18,6 @@ import com.google.common.cache.CacheBuilder;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.Dirent.Type;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -54,16 +53,8 @@ public final class GlobFunction implements SkyFunction {
   public SkyValue compute(SkyKey skyKey, Environment env) throws GlobFunctionException {
     GlobDescriptor glob = (GlobDescriptor) skyKey.argument();
 
-    PackageLookupValue globPkgLookupValue = (PackageLookupValue)
-        env.getValue(PackageLookupValue.key(glob.getPackageId()));
-    if (globPkgLookupValue == null) {
-      return null;
-    }
-    Preconditions.checkState(globPkgLookupValue.packageExists(), "%s isn't an existing package",
-        glob.getPackageId());
-    // Note that this implies that the package's BUILD file exists which implies that the
-    // package's directory exists.
-
+    // Note that the glob's package is assumed to exist which implies that the package's BUILD file
+    // exists which implies that the package's directory exists.
     PathFragment globSubdir = glob.getSubdir();
     if (!globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) {
       PackageLookupValue globSubdirPkgLookupValue = (PackageLookupValue) env.getValue(
@@ -103,8 +94,8 @@ public final class GlobFunction implements SkyFunction {
           matches.add(globSubdir);
         }
       } else {
-        SkyKey globKey = GlobValue.internalKey(
-            glob.getPackageId(), globSubdir, patternTail, glob.excludeDirs());
+        SkyKey globKey = GlobValue.internalKey(glob.getPackageId(), glob.getPackageRoot(),
+            globSubdir, patternTail, glob.excludeDirs());
         GlobValue globValue = (GlobValue) env.getValue(globKey);
         if (globValue == null) {
           return null;
@@ -114,8 +105,7 @@ public final class GlobFunction implements SkyFunction {
     }
 
     PathFragment dirPathFragment = glob.getPackageId().getPackageFragment().getRelative(globSubdir);
-    RootedPath dirRootedPath = RootedPath.toRootedPath(globPkgLookupValue.getRoot(),
-        dirPathFragment);
+    RootedPath dirRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(), dirPathFragment);
     if (alwaysUseDirListing || containsGlobs(patternHead)) {
       // Pattern contains globs, so a directory listing is required.
       //
@@ -145,7 +135,7 @@ public final class GlobFunction implements SkyFunction {
           // For symlinks, look up the corresponding FileValue. This ensures that if the symlink
           // changes and "switches types" (say, from a file to a directory), this value will be
           // invalidated.
-          RootedPath symlinkRootedPath = RootedPath.toRootedPath(globPkgLookupValue.getRoot(),
+          RootedPath symlinkRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(),
               dirPathFragment.getRelative(fileName));
           FileValue symlinkFileValue = (FileValue) env.getValue(FileValue.key(symlinkRootedPath));
           if (symlinkFileValue == null) {
@@ -166,7 +156,7 @@ public final class GlobFunction implements SkyFunction {
     } else {
       // Pattern does not contain globs, so a direct stat is enough.
       String fileName = patternHead;
-      RootedPath fileRootedPath = RootedPath.toRootedPath(globPkgLookupValue.getRoot(),
+      RootedPath fileRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(),
           dirPathFragment.getRelative(fileName));
       FileValue fileValue = (FileValue) env.getValue(FileValue.key(fileRootedPath));
       if (fileValue == null) {
@@ -212,7 +202,7 @@ public final class GlobFunction implements SkyFunction {
       Environment env) {
     if (isDirectory && subdirPattern != null) {
       // This is a directory, and the pattern covers files under that directory.
-      SkyKey subdirGlobKey = GlobValue.internalKey(glob.getPackageId(),
+      SkyKey subdirGlobKey = GlobValue.internalKey(glob.getPackageId(), glob.getPackageRoot(),
           glob.getSubdir().getRelative(fileName), subdirPattern, glob.excludeDirs());
       GlobValue subdirGlob = (GlobValue) env.getValue(subdirGlobKey);
       if (subdirGlob == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java
index 127db0c4bf..5e877bd078 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.util.Preconditions;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.UnixGlob;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -79,9 +80,8 @@ public final class GlobValue implements SkyValue {
    * @throws InvalidGlobPatternException if the pattern is not valid.
    */
   @ThreadSafe
-  public static SkyKey key(PackageIdentifier packageId, String pattern, boolean excludeDirs,
-      PathFragment subdir)
-      throws InvalidGlobPatternException {
+  public static SkyKey key(PackageIdentifier packageId, Path packageRoot, String pattern,
+      boolean excludeDirs, PathFragment subdir) throws InvalidGlobPatternException {
     if (pattern.indexOf('?') != -1) {
       throw new InvalidGlobPatternException(pattern, "wildcard ? forbidden");
     }
@@ -91,7 +91,7 @@ public final class GlobValue implements SkyValue {
       throw new InvalidGlobPatternException(pattern, error);
     }
 
-    return internalKey(packageId, subdir, pattern, excludeDirs);
+    return internalKey(packageId, packageRoot, subdir, pattern, excludeDirs);
   }
 
   /**
@@ -100,10 +100,10 @@ public final class GlobValue implements SkyValue {
    * <p>Do not use outside {@code GlobFunction}.
    */
   @ThreadSafe
-  static SkyKey internalKey(PackageIdentifier packageId, PathFragment subdir, String pattern,
-      boolean excludeDirs) {
+  static SkyKey internalKey(PackageIdentifier packageId, Path packageRoot, PathFragment subdir,
+      String pattern, boolean excludeDirs) {
     return new SkyKey(SkyFunctions.GLOB,
-        new GlobDescriptor(packageId, subdir, pattern, excludeDirs));
+        new GlobDescriptor(packageId, packageRoot, subdir, pattern, excludeDirs));
   }
 
   /**
@@ -113,7 +113,7 @@ public final class GlobValue implements SkyValue {
    */
   @ThreadSafe
   static SkyKey internalKey(GlobDescriptor glob, String subdirName) {
-    return internalKey(glob.packageId, glob.subdir.getRelative(subdirName),
+    return internalKey(glob.packageId, glob.packageRoot, glob.subdir.getRelative(subdirName),
         glob.pattern, glob.excludeDirs);
   }
 
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 d77ef7907a..03b0f5f5c4 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
@@ -232,6 +232,7 @@ public class PackageFunction implements SkyFunction {
       Collection<Pair<String, Boolean>> globPatterns,
       Map<Label, Path> subincludes,
       PackageIdentifier packageIdentifier,
+      Path packageRoot,
       boolean containsErrors)
       throws InternalInconsistentFilesystemException {
     boolean packageShouldBeInError = containsErrors;
@@ -312,8 +313,8 @@ public class PackageFunction implements SkyFunction {
       boolean excludeDirs = globPattern.getSecond();
       SkyKey globSkyKey;
       try {
-        globSkyKey =
-            GlobValue.key(packageIdentifier, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
+        globSkyKey = GlobValue.key(packageIdentifier, packageRoot, pattern, excludeDirs,
+            PathFragment.EMPTY_FRAGMENT);
       } catch (InvalidGlobPatternException e) {
         // Globs that make it to pkg.getGlobPatterns() should already be filtered for errors.
         throw new IllegalStateException(e);
@@ -483,7 +484,8 @@ public class PackageFunction implements SkyFunction {
     try {
       packageShouldBeConsideredInError =
           markDependenciesAndPropagateInconsistentFilesystemExceptions(
-              env, globPatterns, subincludes, packageId, legacyPkgBuilder.containsErrors());
+              env, globPatterns, subincludes, packageId, packageLookupValue.getRoot(),
+              legacyPkgBuilder.containsErrors());
     } catch (InternalInconsistentFilesystemException e) {
       packageFunctionCache.invalidate(packageId);
       throw new PackageFunctionException(e,