Change getValuesOrThrow one exception version method to `get(Ordere…

…d)ValuesAndExceptions` in build/lib directories.

Instead of a `Map`, use `SkyframeIterableResult` for traversal and `SkyframeLookupResult` for lookups. Traversing is more efficient than map lookups, and these custom classes create less garbage.

This is a no-behavior-change refactoring, new tests are not necessary.

PiperOrigin-RevId: 432034415

src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java

@@ -51,8 +51,7 @@
 import com.google.devtools.build.lib.util.OrderedSetMultimap;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
-import com.google.devtools.build.skyframe.SkyKey;
-import com.google.devtools.build.skyframe.ValueOrException;
+import com.google.devtools.build.skyframe.SkyframeIterableResult;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -352,20 +351,19 @@ private ImmutableList<Dependency> resolveGenericTransition(
       throw new ConfiguredValueCreationException(ctgValue, e.getMessage());
     }
 
-    Map<SkyKey, ValueOrException<InvalidConfigurationException>> depConfigValues =
-        env.getValuesOrThrow(configurationKeys.values(), InvalidConfigurationException.class);
+    SkyframeIterableResult depConfigValues =
+        env.getOrderedValuesAndExceptions(configurationKeys.values());
     List<Dependency> dependencies = new ArrayList<>();
     try {
       for (Map.Entry<String, BuildConfigurationKey> entry : configurationKeys.entrySet()) {
         String transitionKey = entry.getKey();
-        ValueOrException<InvalidConfigurationException> valueOrException =
-            depConfigValues.get(entry.getValue());
-        if (valueOrException.get() == null) {
+        // TODO(blaze-configurability-team): Should be able to just use BuildConfigurationKey
+        BuildConfigurationValue configuration =
+            (BuildConfigurationValue)
+                depConfigValues.nextOrThrow(InvalidConfigurationException.class);
+        if (configuration == null) {
           continue;
         }
-        // TODO(blaze-configurability-team): Should be able to just use BuildConfigurationKey
-        BuildConfigurationValue configuration = (BuildConfigurationValue) valueOrException.get();
-        if (configuration != null) {
           Dependency resolvedDep =
               dependencyBuilder
                   // Copy the builder so we don't overwrite the other dependencies.
@@ -375,7 +373,6 @@ private ImmutableList<Dependency> resolveGenericTransition(
                   .setTransitionKey(transitionKey)
                   .build();
           dependencies.add(resolvedDep);
-        }
       }
       if (env.valuesMissing()) {
         return null; // Need dependency configurations.

src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java

@@ -14,17 +14,18 @@
 package com.google.devtools.build.lib.bazel.rules.android;
 
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.Streams.stream;
 
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
+import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.io.InconsistentFilesystemException;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Type;
@@ -43,7 +44,7 @@
 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.ValueOrException;
+import com.google.devtools.build.skyframe.SkyframeLookupResult;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.Iterator;
@@ -481,13 +482,13 @@ private ImmutableSortedSet<PathFragment> getAndroidDeviceSystemImageDirs(
   /**
    * Gets DirectoryListingValues for subdirectories of the directory or returns null.
    *
-   * Ignores all non-directory files.
+   * <p>Ignores all non-directory files.
    */
   private static ImmutableMap<PathFragment, DirectoryListingValue> getSubdirectoryListingValues(
       final Path root, final PathFragment path, DirectoryListingValue directory, Environment env)
       throws RepositoryFunctionException, InterruptedException {
     Map<PathFragment, SkyKey> skyKeysForSubdirectoryLookups =
-        Streams.stream(directory.getDirents())
+        stream(directory.getDirents())
             .filter(dirent -> dirent.getType().equals(Dirent.Type.DIRECTORY))
             .collect(
                 toImmutableMap(
@@ -498,16 +499,22 @@ private static ImmutableMap<PathFragment, DirectoryListingValue> getSubdirectory
                                 Root.fromPath(root),
                                 root.getRelative(path).getRelative(input.getName())))));
 
-    Map<SkyKey, ValueOrException<InconsistentFilesystemException>> values =
-        env.getValuesOrThrow(
-            skyKeysForSubdirectoryLookups.values(), InconsistentFilesystemException.class);
+    SkyframeLookupResult values =
+        env.getValuesAndExceptions(skyKeysForSubdirectoryLookups.values());
+    boolean valuesMissing = env.valuesMissing();
 
     ImmutableMap.Builder<PathFragment, DirectoryListingValue> directoryListingValues =
         new ImmutableMap.Builder<>();
     for (PathFragment pathFragment : skyKeysForSubdirectoryLookups.keySet()) {
       try {
-        SkyValue skyValue = values.get(skyKeysForSubdirectoryLookups.get(pathFragment)).get();
+        SkyKey skyKey = skyKeysForSubdirectoryLookups.get(pathFragment);
+        SkyValue skyValue = values.getOrThrow(skyKey, InconsistentFilesystemException.class);
         if (skyValue == null) {
+          if (!valuesMissing) {
+            BugReport.sendBugReport(
+                new IllegalStateException(
+                    "SkyValue " + skyKey + " was missing, this should never happern"));
+          }
           return null;
         }
         directoryListingValues.put(pathFragment, (DirectoryListingValue) skyValue);

src/main/java/com/google/devtools/build/lib/bazel/rules/android/BUILD

@@ -37,6 +37,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/bazel/rules/cpp:bazel_cpp_semantics",
         "//src/main/java/com/google/devtools/build/lib/bazel/rules/java",
         "//src/main/java/com/google/devtools/build/lib/bazel/rules/java:bazel_java_semantics",
+        "//src/main/java/com/google/devtools/build/lib/bugreport",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/events",

src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java

@@ -20,6 +20,7 @@
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
@@ -97,7 +98,7 @@
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-import com.google.devtools.build.skyframe.ValueOrException;
+import com.google.devtools.build.skyframe.SkyframeIterableResult;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.TriState;
@@ -487,27 +488,32 @@ public Map<String, Collection<Target>> preloadTargetPatterns(
       boolean ok = true;
       Map<String, Collection<Target>> preloadedPatterns =
           Maps.newHashMapWithExpectedSize(patterns.size());
-      Map<TargetPatternKey, String> patternKeys = Maps.newHashMapWithExpectedSize(patterns.size());
+      ImmutableMap.Builder<TargetPatternKey, String> targetBuilder =
+          ImmutableMap.builderWithExpectedSize(patterns.size());
       for (String pattern : patterns) {
         checkValidPatternType(pattern);
-        patternKeys.put(
+        targetBuilder.put(
             TargetPatternValue.key(
                 SignedTargetPattern.parse(pattern, TargetPattern.defaultParser()),
                 FilteringPolicies.NO_FILTER),
             pattern);
       }
+      ImmutableMap<TargetPatternKey, String> patternKeys = targetBuilder.buildOrThrow();
       Set<SkyKey> packageKeys = new HashSet<>();
       Map<String, ResolvedTargets<Label>> resolvedLabelsMap =
           Maps.newHashMapWithExpectedSize(patterns.size());
       synchronized (this) {
-        for (Map.Entry<SkyKey, ValueOrException<TargetParsingException>> entry :
-          env.getValuesOrThrow(patternKeys.keySet(), TargetParsingException.class).entrySet()) {
-          TargetPatternValue patternValue = (TargetPatternValue) entry.getValue().get();
+        ImmutableSet<TargetPatternKey> targetPatternKeys = patternKeys.keySet();
+        SkyframeIterableResult patternKeysResult =
+            env.getOrderedValuesAndExceptions(targetPatternKeys);
+        for (TargetPatternKey targetPatternKey : targetPatternKeys) {
+          TargetPatternValue patternValue =
+              (TargetPatternValue) patternKeysResult.nextOrThrow(TargetParsingException.class);
           if (patternValue == null) {
             ok = false;
           } else {
             ResolvedTargets<Label> resolvedLabels = patternValue.getTargets();
-            resolvedLabelsMap.put(patternKeys.get(entry.getKey()), resolvedLabels);
+            resolvedLabelsMap.put(patternKeys.get(targetPatternKey), resolvedLabels);
             for (Label label
                 : Iterables.concat(resolvedLabels.getTargets(),
                     resolvedLabels.getFilteredTargets())) {
@@ -522,12 +528,14 @@ public Map<String, Collection<Target>> preloadTargetPatterns(
       Map<PackageIdentifier, Package> packages =
           Maps.newHashMapWithExpectedSize(packageKeys.size());
       synchronized (this) {
-        for (Map.Entry<SkyKey, ValueOrException<NoSuchPackageException>> entry :
-          env.getValuesOrThrow(packageKeys, NoSuchPackageException.class).entrySet()) {
-          PackageIdentifier pkgName = (PackageIdentifier) entry.getKey().argument();
+        SkyframeIterableResult packageKeysResult = env.getOrderedValuesAndExceptions(packageKeys);
+        // packageKeys is not mutated, the iteration order is the same.
+        for (SkyKey depKey : packageKeys) {
+          PackageIdentifier pkgName = (PackageIdentifier) depKey.argument();
           Package pkg;
           try {
-            PackageValue packageValue = (PackageValue) entry.getValue().get();
+            PackageValue packageValue =
+                (PackageValue) packageKeysResult.nextOrThrow(NoSuchPackageException.class);
             if (packageValue == null) {
               ok = false;
               continue;

src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java

@@ -101,7 +101,6 @@
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.SkyframeIterableResult;
 import com.google.devtools.build.skyframe.SkyframeLookupResult;
-import com.google.devtools.build.skyframe.ValueOrException;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -906,16 +905,15 @@ private DiscoveredState addDiscoveredInputs(
       throws InterruptedException, ActionExecutionException {
     // TODO(janakr): This code's assumptions are wrong in the face of Starlark actions with unused
     //  inputs, since ActionExecutionExceptions can come through here and should be aggregated. Fix.
-    Map<SkyKey, ValueOrException<SourceArtifactException>> nonMandatoryDiscovered =
-        env.getValuesOrThrow(
-            Iterables.transform(discoveredInputs, Artifact::key), SourceArtifactException.class);
-    if (nonMandatoryDiscovered.isEmpty()) {
+    SkyframeIterableResult nonMandatoryDiscovered =
+        env.getOrderedValuesAndExceptions(Iterables.transform(discoveredInputs, Artifact::key));
+    if (!nonMandatoryDiscovered.hasNext()) {
       return DiscoveredState.NO_DISCOVERED_DATA;
     }
     for (Artifact input : discoveredInputs) {
       SkyValue retrievedMetadata;
       try {
-        retrievedMetadata = nonMandatoryDiscovered.get(Artifact.key(input)).get();
+        retrievedMetadata = nonMandatoryDiscovered.nextOrThrow(SourceArtifactException.class);
       } catch (SourceArtifactException e) {
         if (!input.isSourceArtifact()) {
           bugReporter.sendBugReport(
@@ -990,8 +988,7 @@ private static <E extends Exception> Object establishSkyframeDependencies(
       @SuppressWarnings("unchecked")
       SkyframeAwareAction<E> skyframeAwareAction = (SkyframeAwareAction<E>) action;
       ImmutableList<? extends SkyKey> keys = skyframeAwareAction.getDirectSkyframeDependencies();
-      Map<SkyKey, ValueOrException<E>> values =
-          env.getValuesOrThrow(keys, skyframeAwareAction.getExceptionType());
+      SkyframeIterableResult values = env.getOrderedValuesAndExceptions(keys);
 
       try {
         return skyframeAwareAction.processSkyframeValues(keys, values, env.valuesMissing());

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -2510,10 +2510,10 @@ java_library(
     srcs = ["TargetPatternUtil.java"],
     deps = [
         ":target_pattern_value",
+        "//src/main/java/com/google/devtools/build/lib/bugreport",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/pkgcache",
         "//src/main/java/com/google/devtools/build/skyframe",
-        "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
         "//third_party:guava",
         "//third_party:jsr305",
     ],

src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java

@@ -59,7 +59,7 @@
 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.ValueOrException;
+import com.google.devtools.build.skyframe.SkyframeIterableResult;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
@@ -966,12 +966,11 @@ private static List<BzlLoadValue> computeBzlLoadsWithSkyframe(
       Environment env, List<BzlLoadValue.Key> keys, List<Pair<String, Location>> programLoads)
       throws BzlLoadFailedException, InterruptedException {
     List<BzlLoadValue> bzlLoads = Lists.newArrayListWithExpectedSize(keys.size());
-    Map<SkyKey, ValueOrException<BzlLoadFailedException>> values =
-        env.getValuesOrThrow(keys, BzlLoadFailedException.class);
+    SkyframeIterableResult values = env.getOrderedValuesAndExceptions(keys);
     // Process loads (and report first error) in source order.
     for (int i = 0; i < keys.size(); i++) {
       try {
-        bzlLoads.add((BzlLoadValue) values.get(keys.get(i)).get());
+        bzlLoads.add((BzlLoadValue) values.nextOrThrow(BzlLoadFailedException.class));
       } catch (BzlLoadFailedException ex) {
         throw BzlLoadFailedException.whileLoadingDep(programLoads.get(i).second, ex);
       }

src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java

@@ -77,7 +77,6 @@
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.SkyframeIterableResult;
 import com.google.devtools.build.skyframe.SkyframeLookupResult;
-import com.google.devtools.build.skyframe.ValueOrException;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -675,13 +674,11 @@ private static List<BzlLoadValue> computeBzlLoadsNoInlining(
       Environment env, List<BzlLoadValue.Key> keys)
       throws InterruptedException, BzlLoadFailedException {
     List<BzlLoadValue> bzlLoads = Lists.newArrayListWithExpectedSize(keys.size());
-    Map<SkyKey, ValueOrException<BzlLoadFailedException>> starlarkLookupResults =
-        env.getValuesOrThrow(keys, BzlLoadFailedException.class);
-    // TODO(b/172462551): use list-based utility (see CL 342917514).
-    for (BzlLoadValue.Key key : keys) {
+    SkyframeIterableResult starlarkLookupResults = env.getOrderedValuesAndExceptions(keys);
+    for (int i = 0; i < keys.size(); i++) {
       // TODO(adonovan): if get fails, report the source location
       // in the corresponding programLoads[i] (see caller).
-      bzlLoads.add((BzlLoadValue) starlarkLookupResults.get(key).get());
+      bzlLoads.add((BzlLoadValue) starlarkLookupResults.nextOrThrow(BzlLoadFailedException.class));
     }
     return env.valuesMissing() ? null : bzlLoads;
   }

src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java

@@ -23,6 +23,7 @@
 import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
+import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -33,9 +34,7 @@
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.server.FailureDetails.Toolchain.Code;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyframeIterableResult;
-import com.google.devtools.build.skyframe.ValueOrException;
 import java.util.HashMap;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -81,21 +80,25 @@ private static void validatePlatformKeys(
             .map(PackageValue::key)
             .collect(toImmutableSet());
 
-    Map<SkyKey, ValueOrException<NoSuchPackageException>> values =
-        env.getValuesOrThrow(packageKeys, NoSuchPackageException.class);
+    SkyframeIterableResult values = env.getOrderedValuesAndExceptions(packageKeys);
     boolean valuesMissing = env.valuesMissing();
     Map<PackageIdentifier, Package> packages = valuesMissing ? null : new HashMap<>();
-    for (Map.Entry<SkyKey, ValueOrException<NoSuchPackageException>> value : values.entrySet()) {
+    while (values.hasNext()) {
       try {
-        PackageValue packageValue = (PackageValue) value.getValue().get();
+        PackageValue packageValue = (PackageValue) values.nextOrThrow(NoSuchPackageException.class);
         if (!valuesMissing && packageValue != null) {
           packages.put(packageValue.getPackage().getPackageIdentifier(), packageValue.getPackage());
         }
       } catch (NoSuchPackageException e) {
         throw new InvalidPlatformException(e);
       }
     }
-    if (valuesMissing) {
+    if (env.valuesMissing()) {
+      if (valuesMissing != env.valuesMissing()) {
+        BugReport.sendBugReport(
+            new IllegalStateException(
+                "Some value from " + packageKeys + " was missing, this should never happen"));
+      }
       return;
     }