From 1cf8e3b204974868904ff45737e122de95a25e82 Mon Sep 17 00:00:00 2001 From: Nathan Harmata Date: Sat, 17 Oct 2015 00:19:05 +0000 Subject: Avoid some Skyframe restarts. -- MOS_MIGRATED_REVID=105648425 --- .../build/lib/skyframe/PackageFunction.java | 15 ++++---- .../RecursiveDirectoryTraversalFunction.java | 44 +++++++++++++++------- 2 files changed, 39 insertions(+), 20 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 50d5c8a403..26e35b1388 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 @@ -473,6 +473,12 @@ public class PackageFunction implements SkyFunction { } legacyPkgBuilder.buildPartial(); try { + // Since the Skyframe dependencies we request below in + // markDependenciesAndPropagateInconsistentFilesystemExceptions are requested independently of + // the ones requested here in + // handleLabelsCrossingSubpackagesAndPropagateInconsistentFilesystemExceptions, we don't + // bother checking for missing values and instead piggyback on the env.missingValues() call + // for the former. This avoids a Skyframe restart. handleLabelsCrossingSubpackagesAndPropagateInconsistentFilesystemExceptions( packageLookupValue.getRoot(), packageId, legacyPkgBuilder, env); } catch (InternalInconsistentFilesystemException e) { @@ -480,14 +486,8 @@ public class PackageFunction implements SkyFunction { throw new PackageFunctionException(e, e.isTransient() ? Transience.TRANSIENT : Transience.PERSISTENT); } - if (env.valuesMissing()) { - // The package we just loaded will be in the {@code packageFunctionCache} next when this - // SkyFunction is called again. - return null; - } Collection> globPatterns = legacyPkgBuilder.getGlobPatterns(); Map subincludes = legacyPkgBuilder.getSubincludes(); - Event.replayEventsOn(env.getListener(), legacyPkgBuilder.getEvents()); boolean packageShouldBeConsideredInError; try { packageShouldBeConsideredInError = @@ -498,11 +498,12 @@ public class PackageFunction implements SkyFunction { throw new PackageFunctionException(e, e.isTransient() ? Transience.TRANSIENT : Transience.PERSISTENT); } - if (env.valuesMissing()) { return null; } + Event.replayEventsOn(env.getListener(), legacyPkgBuilder.getEvents()); + if (packageShouldBeConsideredInError) { legacyPkgBuilder.setContainsErrors(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java index b33f78c3d9..f55de62252 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java @@ -13,6 +13,8 @@ // 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.ImmutableSet; import com.google.common.collect.Lists; import com.google.devtools.build.lib.cmdline.PackageIdentifier; @@ -31,6 +33,7 @@ import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.ValueOrException4; import java.io.IOException; import java.util.List; @@ -132,16 +135,32 @@ abstract class RecursiveDirectoryTraversalFunction PackageIdentifier packageId = PackageIdentifier.create( recursivePkgKey.getRepository(), rootRelativePath); + SkyKey pkgLookupKey = PackageLookupValue.key(packageId); + SkyKey dirListingKey = DirectoryListingValue.key(rootedPath); + Map> pkgLookupAndDirectoryListingDeps = env.getValuesOrThrow( + ImmutableList.of(pkgLookupKey, dirListingKey), + NoSuchPackageException.class, + InconsistentFilesystemException.class, + FileSymlinkException.class, + IOException.class); + if (env.valuesMissing()) { + return null; + } PackageLookupValue pkgLookupValue; try { - pkgLookupValue = (PackageLookupValue) env.getValueOrThrow(PackageLookupValue.key(packageId), - NoSuchPackageException.class, InconsistentFilesystemException.class); + pkgLookupValue = (PackageLookupValue) Preconditions.checkNotNull( + pkgLookupAndDirectoryListingDeps.get(pkgLookupKey).get(), "%s %s", recursivePkgKey, + pkgLookupKey); } catch (NoSuchPackageException | InconsistentFilesystemException e) { return reportErrorAndReturn("Failed to load package", e, rootRelativePath, env.getListener()); - } - if (pkgLookupValue == null) { - return null; + } catch (IOException | FileSymlinkException e) { + throw new IllegalStateException(e); } TVisitor visitor = getInitialVisitor(); @@ -187,11 +206,11 @@ abstract class RecursiveDirectoryTraversalFunction // 'foo/bar' under 'rootA/workspace'. } - DirectoryListingValue dirValue; + DirectoryListingValue dirListingValue; try { - dirValue = (DirectoryListingValue) env.getValueOrThrow(DirectoryListingValue.key(rootedPath), - InconsistentFilesystemException.class, IOException.class, - FileSymlinkException.class); + dirListingValue = (DirectoryListingValue) Preconditions.checkNotNull( + pkgLookupAndDirectoryListingDeps.get(dirListingKey).get(), "%s %s", recursivePkgKey, + dirListingKey); } catch (InconsistentFilesystemException | IOException e) { return reportErrorAndReturn("Failed to list directory contents", e, rootRelativePath, env.getListener()); @@ -201,13 +220,12 @@ abstract class RecursiveDirectoryTraversalFunction // be able to avoid throwing there but throw here. throw new IllegalStateException("Symlink cycle found after not being found for \"" + rootedPath + "\""); - } - if (dirValue == null) { - return null; + } catch (NoSuchPackageException e) { + throw new IllegalStateException(e); } List childDeps = Lists.newArrayList(); - for (Dirent dirent : dirValue.getDirents()) { + for (Dirent dirent : dirListingValue.getDirents()) { if (dirent.getType() != Type.DIRECTORY && dirent.getType() != Type.SYMLINK) { // Non-directories can never host packages. Symlinks to non-directories are weeded out at // the next level of recursion when we check if its FileValue is a directory. This is slower -- cgit v1.2.3