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
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 ec59544..1687a81 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.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 @@
@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 @@
* 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 @@
@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 @@
}
/**
+ * 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 @@
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 5520c23..50b7c29 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.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 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 @@
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 @@
}
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 @@
// 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 @@
} 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 @@
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 127db0c..5e877bd 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.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 @@
* @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 @@
throw new InvalidGlobPatternException(pattern, error);
}
- return internalKey(packageId, subdir, pattern, excludeDirs);
+ return internalKey(packageId, packageRoot, subdir, pattern, excludeDirs);
}
/**
@@ -100,10 +100,10 @@
* <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 @@
*/
@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 d77ef79..03b0f5f 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 @@
Collection<Pair<String, Boolean>> globPatterns,
Map<Label, Path> subincludes,
PackageIdentifier packageIdentifier,
+ Path packageRoot,
boolean containsErrors)
throws InternalInconsistentFilesystemException {
boolean packageShouldBeInError = containsErrors;
@@ -312,8 +313,8 @@
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 @@
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,
