From b795e6bd2a2371a4a9f5e1b2cd618fdd7eceb30a Mon Sep 17 00:00:00 2001 From: Nathan Harmata Date: Thu, 4 Feb 2016 01:10:19 +0000 Subject: 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 --- .../build/lib/skyframe/GlobDescriptor.java | 25 ++++++++++++++++----- .../devtools/build/lib/skyframe/GlobFunction.java | 26 +++++++--------------- .../devtools/build/lib/skyframe/GlobValue.java | 16 ++++++------- .../build/lib/skyframe/PackageFunction.java | 8 ++++--- 4 files changed, 40 insertions(+), 35 deletions(-) (limited to 'src/main/java/com/google/devtools/build') 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("", - packageId, subdir, pattern, excludeDirs); + return String.format( + "", + packageId, packageRoot, subdir, pattern, excludeDirs); } /** @@ -69,6 +74,13 @@ public final class GlobDescriptor implements Serializable { return packageId; } + /** + * Returns the package root of {@code getPackageId()}. + */ + public Path getPackageRoot() { + return packageRoot; + } + /** * Returns the subdirectory of the package under consideration. */ @@ -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 { *

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> globPatterns, Map 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, -- cgit v1.2.3