Replace `getValues` in `skyframe` directory with `getValuesAndExceptions/getOrderedValuesAndExceptions` to create less garbage and narrow the available interface of the returned result. PiperOrigin-RevId: 435647269
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java index 860b606..96c5d3e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java
@@ -43,6 +43,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.SkyframeIterableResult; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -145,7 +146,8 @@ addExtraActionsIfRequested( configuredTarget.getProvider(ExtraActionArtifactsProvider.class), artifactsToBuild); if (buildDriverKey.getTestType() == NOT_TEST) { - env.getValues( + declareDependenciesAndCheckValues( + env, Iterables.concat( artifactsToBuild.build(), Collections.singletonList( @@ -172,7 +174,8 @@ "Invalid test type, expect only parallel tests: %s", buildDriverKey); // Only run non-exclusive tests here. Exclusive tests need to be run sequentially later. - env.getValues( + declareDependenciesAndCheckValues( + env, Iterables.concat( artifactsToBuild.build(), Collections.singletonList( @@ -200,7 +203,23 @@ AspectCompletionKey.create( ((AspectValue) aspectValue).getKey(), topLevelArtifactContext)); } - env.getValues(Iterables.concat(artifactsToBuild.build(), aspectCompletionKeys)); + declareDependenciesAndCheckValues( + env, Iterables.concat(artifactsToBuild.build(), aspectCompletionKeys)); + } + + /** + * Declares dependencies and checks values for requested nodes in the graph. + * + * <p>Calls {@link SkyframeIterableResult} and iterates over the result. If any node is not done, + * or during iteration any value has exception, {@link SkyFunction.Environment#valuesMissing} will + * return true. + */ + private static void declareDependenciesAndCheckValues( + Environment env, Iterable<? extends SkyKey> skyKeys) throws InterruptedException { + SkyframeIterableResult result = env.getOrderedValuesAndExceptions(skyKeys); + while (result.hasNext()) { + result.next(); + } } private ImmutableMap<ActionAnalysisMetadata, ConflictException> checkActionConflicts(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CollectTargetsInPackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CollectTargetsInPackageFunction.java index e7f9a4e..324a172 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CollectTargetsInPackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CollectTargetsInPackageFunction.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.pkgcache.TargetPatternResolverUtil; +import com.google.devtools.build.skyframe.GraphTraversingHelper; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; @@ -50,14 +51,13 @@ Event.error( "package contains errors: " + packageId.getPackageFragment().getPathString())); } - env.getValues( - Iterables.transform( - TargetPatternResolverUtil.resolvePackageTargets(pkg, argument.getFilteringPolicy()), - TO_TRANSITIVE_TRAVERSAL_KEY)); - if (env.valuesMissing()) { - return null; - } - return CollectTargetsInPackageValue.INSTANCE; + return GraphTraversingHelper.declareDependenciesAndCheckIfValuesMissing( + env, + Iterables.transform( + TargetPatternResolverUtil.resolvePackageTargets(pkg, argument.getFilteringPolicy()), + TO_TRANSITIVE_TRAVERSAL_KEY)) + ? null + : CollectTargetsInPackageValue.INSTANCE; } private static final Function<Target, SkyKey> TO_TRANSITIVE_TRAVERSAL_KEY =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CollectTestSuitesInPackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CollectTestSuitesInPackageFunction.java index 85671af..41a78ad 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CollectTestSuitesInPackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CollectTestSuitesInPackageFunction.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.TargetUtils; +import com.google.devtools.build.skyframe.GraphTraversingHelper; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; @@ -67,11 +68,9 @@ CollectTestSuitesInPackageValue.key(label.getPackageIdentifier())); } collectTestSuiteInPkgDeps.remove(skyKey); - env.getValues(collectTestSuiteInPkgDeps); - - if (env.valuesMissing()) { - return null; - } - return CollectTestSuitesInPackageValue.INSTANCE; + return GraphTraversingHelper.declareDependenciesAndCheckIfValuesMissing( + env, collectTestSuiteInPkgDeps) + ? null + : CollectTestSuitesInPackageValue.INSTANCE; } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java index 00fef49..7171920 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java
@@ -24,8 +24,7 @@ import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyValue; -import java.util.Map; +import com.google.devtools.build.skyframe.SkyframeIterableResult; import javax.annotation.Nullable; /** @@ -93,16 +92,15 @@ } else { packageAndMaybeConfiguration = ImmutableSet.of(packageKey, configurationKeyMaybe); } - Map<SkyKey, SkyValue> packageAndMaybeConfigurationValues = - env.getValues(packageAndMaybeConfiguration); + SkyframeIterableResult packageAndMaybeConfigurationValues = + env.getOrderedValuesAndExceptions(packageAndMaybeConfiguration); // Don't test env.valuesMissing(), because values may already be missing from the caller. - PackageValue packageValue = (PackageValue) packageAndMaybeConfigurationValues.get(packageKey); + PackageValue packageValue = (PackageValue) packageAndMaybeConfigurationValues.next(); if (packageValue == null) { return null; } if (configurationKeyMaybe != null) { - configuration = - (BuildConfigurationValue) packageAndMaybeConfigurationValues.get(configurationKeyMaybe); + configuration = (BuildConfigurationValue) packageAndMaybeConfigurationValues.next(); if (configuration == null) { return null; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index a6fc2141..ac56de2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -479,12 +479,15 @@ } else { packageAndMaybeConfiguration = ImmutableSet.of(packageKey, configurationKeyMaybe); } - Map<SkyKey, SkyValue> packageAndMaybeConfigurationValues = - env.getValues(packageAndMaybeConfiguration); + SkyframeLookupResult packageAndMaybeConfigurationValues = + env.getValuesAndExceptions(packageAndMaybeConfiguration); if (env.valuesMissing()) { return null; } PackageValue packageValue = (PackageValue) packageAndMaybeConfigurationValues.get(packageKey); + if (packageValue == null) { + return null; + } Package pkg = packageValue.getPackage(); if (configurationKeyMaybe != null) { configuration = @@ -991,7 +994,7 @@ Map<SkyKey, ConfiguredTargetAndData> result = Maps.newHashMapWithExpectedSize(deps.size()); Set<SkyKey> aliasPackagesToFetch = new HashSet<>(); List<Dependency> aliasDepsToRedo = new ArrayList<>(); - Map<SkyKey, SkyValue> aliasPackageValues = null; + SkyframeLookupResult aliasPackageValues = null; Collection<Dependency> depsToProcess = deps; for (int i = 0; i < 2; i++) { for (Dependency dep : depsToProcess) { @@ -1073,7 +1076,7 @@ if (aliasDepsToRedo.isEmpty()) { break; } - aliasPackageValues = env.getValues(aliasPackagesToFetch); + aliasPackageValues = env.getValuesAndExceptions(aliasPackagesToFetch); depsToProcess = aliasDepsToRedo; }
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 2885689..01bcb9f 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
@@ -35,7 +35,9 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.SkyframeIterableResult; import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.regex.Pattern; import javax.annotation.Nullable; @@ -168,17 +170,15 @@ globSubdir, patternTail, globberOperation); - Map<SkyKey, SkyValue> listingAndRecursiveGlobMap = - env.getValues( + SkyframeIterableResult listingAndRecursiveGlobResult = + env.getOrderedValuesAndExceptions( ImmutableList.of(keyForRecursiveGlobInCurrentDirectory, directoryListingKey)); if (env.valuesMissing()) { return null; } - GlobValue globValue = - (GlobValue) listingAndRecursiveGlobMap.get(keyForRecursiveGlobInCurrentDirectory); + GlobValue globValue = (GlobValue) listingAndRecursiveGlobResult.next(); matches.addTransitive(globValue.getMatches()); - listingValue = - (DirectoryListingValue) listingAndRecursiveGlobMap.get(directoryListingKey); + listingValue = (DirectoryListingValue) listingAndRecursiveGlobResult.next(); } } @@ -233,8 +233,9 @@ } } - Map<SkyKey, SkyValue> subdirAndSymlinksResult = - env.getValues(Sets.union(subdirMap.keySet(), symlinkFileMap.keySet())); + Set<SkyKey> subdirAndSymlinksKeys = Sets.union(subdirMap.keySet(), symlinkFileMap.keySet()); + SkyframeIterableResult subdirAndSymlinksResult = + env.getOrderedValuesAndExceptions(subdirAndSymlinksKeys); if (env.valuesMissing()) { return null; } @@ -242,14 +243,17 @@ // Second pass: process the symlinks and subdirectories from the first pass, and maybe // collect further SkyKeys if fully resolved symlink targets are themselves directories. // Also process any known directories. - for (Map.Entry<SkyKey, SkyValue> lookedUpKeyAndValue : subdirAndSymlinksResult.entrySet()) { - if (symlinkFileMap.containsKey(lookedUpKeyAndValue.getKey())) { - FileValue symlinkFileValue = (FileValue) lookedUpKeyAndValue.getValue(); + for (SkyKey subdirAndSymlinksKey : subdirAndSymlinksKeys) { + if (symlinkFileMap.containsKey(subdirAndSymlinksKey)) { + FileValue symlinkFileValue = (FileValue) subdirAndSymlinksResult.next(); + if (symlinkFileValue == null) { + return null; + } if (!symlinkFileValue.isSymlink()) { throw new GlobFunctionException( new InconsistentFilesystemException( "readdir and stat disagree about whether " - + ((RootedPath) lookedUpKeyAndValue.getKey().argument()).asPath() + + ((RootedPath) subdirAndSymlinksKey.argument()).asPath() + " is a symlink."), Transience.TRANSIENT); } @@ -278,7 +282,7 @@ throw new GlobFunctionException(symlinkException, Transience.PERSISTENT); } - Dirent dirent = symlinkFileMap.get(lookedUpKeyAndValue.getKey()); + Dirent dirent = symlinkFileMap.get(subdirAndSymlinksKey); String fileName = dirent.getName(); if (symlinkFileValue.isDirectory()) { SkyKey keyToRequest = getSkyKeyForSubdir(fileName, glob, subdirPattern); @@ -289,18 +293,28 @@ sortedResultMap.put(dirent, glob.getSubdir().getRelative(fileName)); } } else { - processSubdir(lookedUpKeyAndValue, subdirMap, glob, sortedResultMap); + SkyValue value = subdirAndSymlinksResult.next(); + if (value == null) { + return null; + } + processSubdir(Map.entry(subdirAndSymlinksKey, value), subdirMap, glob, sortedResultMap); } } - Map<SkyKey, SkyValue> symlinkSubdirResult = env.getValues(symlinkSubdirMap.keySet()); + Set<SkyKey> symlinkSubdirKeys = symlinkSubdirMap.keySet(); + SkyframeIterableResult symlinkSubdirResult = + env.getOrderedValuesAndExceptions(symlinkSubdirKeys); if (env.valuesMissing()) { return null; } // Third pass: do needed subdirectories of symlinked directories discovered during the second // pass. - for (Map.Entry<SkyKey, SkyValue> lookedUpKeyAndValue : symlinkSubdirResult.entrySet()) { - processSubdir(lookedUpKeyAndValue, symlinkSubdirMap, glob, sortedResultMap); + for (SkyKey symlinkSubdirKey : symlinkSubdirKeys) { + processSubdir( + Map.entry(symlinkSubdirKey, symlinkSubdirResult.next()), + symlinkSubdirMap, + glob, + sortedResultMap); } for (Map.Entry<Dirent, Object> fileMatches : sortedResultMap.entrySet()) { addToMatches(fileMatches.getValue(), matches);