Delete `getValues` method in SkyFunction and Environment files, cause all the usages have been replaced by get(Ordered)ValuesAndExceptions. Clean all the comments about `getValues` as well. PiperOrigin-RevId: 439464826
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java index 27d5222..a8de3e5 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -227,7 +227,8 @@ if (depConfig == null) { // Instead of returning immediately, give the loop a chance to queue up every missing // dependency, then return all at once. That prevents re-executing this code an unnecessary - // number of times. i.e. this is equivalent to calling env.getValues() once over all deps. + // number of times. i.e. this is equivalent to calling env.getOrderedValuesAndExceptions() + // once over all deps. needConfigsFromSkyframe = true; } else { resolvedDeps.putAll(dependencyKind, depConfig);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 4acd94b..69defaf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -1809,10 +1809,12 @@ @Nullable private static ImmutableMap<Artifact, NestedSet<Artifact>> computeTransitivelyUsedModules( SkyFunction.Environment env, Set<DerivedArtifact> usedModules) throws InterruptedException { - // Because this env.getValues call does not specify any exceptions, it is impossible for input - // discovery to recover from exceptions thrown by spurious module deps (for instance, if a - // commented-out include references a header file with an error in it). However, we generally - // don't try to recover from errors around spurious includes discovered in the current build. + // Because SkyframeIterableResult.next call does not specify any exceptions where + // SkyframeIterableResult is returned by env.getOrderedValuesAndExceptions, it is + // impossible for input discovery to recover from exceptions thrown by spurious module deps (for + // instance, if a commented-out include references a header file with an error in it). However, + // we generally don't try to recover from errors around spurious includes discovered in the + // current build. // TODO(janakr): Can errors be aggregated here at least? Collection<SkyKey> skyKeys = Collections2.transform(usedModules, DerivedArtifact::getGeneratingActionKey);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java index 1a9c404..c1d08fb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java
@@ -22,7 +22,6 @@ import com.google.devtools.build.skyframe.SkyframeIterableResult; import com.google.devtools.build.skyframe.SkyframeLookupResult; import com.google.devtools.build.skyframe.Version; -import java.util.Map; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -96,12 +95,6 @@ } @Override - public Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys) - throws InterruptedException { - return delegate.getValues(depKeys); - } - - @Override public boolean valuesMissing() { return delegate.valuesMissing(); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java index 80bf8b7..98b48a4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java
@@ -14,6 +14,9 @@ package com.google.devtools.build.lib.skyframe; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.Streams.stream; + import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -71,7 +74,12 @@ @Override protected List<ValueOrUntypedException> getOrderedValueOrUntypedExceptions( Iterable<? extends SkyKey> depKeys) throws InterruptedException { - throw new UnsupportedOperationException(); + EvaluationResult<SkyValue> evaluationResult = + skyframeExecutor.evaluateSkyKeys(eventHandler, depKeys, true); + return stream(depKeys) + .map(evaluationResult::get) + .map(ValueOrUntypedException::ofValueUntyped) + .collect(toImmutableList()); } @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java index 4d77379..27cf411 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java
@@ -22,7 +22,6 @@ import com.google.devtools.build.skyframe.SkyframeIterableResult; import com.google.devtools.build.skyframe.SkyframeLookupResult; import com.google.devtools.build.skyframe.Version; -import java.util.Map; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -112,17 +111,6 @@ } @Override - public Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys) - throws InterruptedException { - preFetch.inform(); - try { - return delegate.getValues(depKeys); - } finally { - postFetch.inform(); - } - } - - @Override public boolean valuesMissing() { return delegate.valuesMissing(); }
diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/AbstractSkyFunctionEnvironment.java index 26ab7a4..6bf9bda 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractSkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractSkyFunctionEnvironment.java
@@ -15,15 +15,9 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Maps; -import com.google.common.flogger.GoogleLogger; -import com.google.common.flogger.StackSize; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.util.GroupedList; -import java.io.IOException; import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -35,7 +29,6 @@ */ @VisibleForTesting public abstract class AbstractSkyFunctionEnvironment implements SkyFunction.Environment { - private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); protected boolean valuesMissing = false; // Hack for the common case that there are no errors in the retrieved values. In that case, we @@ -71,7 +64,7 @@ @Override @Nullable public SkyValue getValue(SkyKey depKey) throws InterruptedException { - return getValues(ImmutableSet.of(depKey)).get(depKey); + return getValueOrThrowHelper(depKey, null, null, null, null); } @Override @@ -139,65 +132,12 @@ } @Override - public Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys) - throws InterruptedException { - Map<SkyKey, ValueOrUntypedException> valuesOrExceptions = getValueOrUntypedExceptions(depKeys); - checkValuesMissingBecauseOfFilteredError(valuesOrExceptions, null, null, null, null); - return Collections.unmodifiableMap( - Maps.transformValues(valuesOrExceptions, ValueOrUntypedException::getValue)); - } - - @Override public SkyframeLookupResult getValuesAndExceptions(Iterable<? extends SkyKey> depKeys) throws InterruptedException { Map<SkyKey, ValueOrUntypedException> valuesOrExceptions = getValueOrUntypedExceptions(depKeys); return new SkyframeLookupResult(() -> valuesMissing = true, valuesOrExceptions::get); } - private <E1 extends Exception, E2 extends Exception, E3 extends Exception, E4 extends Exception> - void checkValuesMissingBecauseOfFilteredError( - Map<SkyKey, ValueOrUntypedException> voes, - @Nullable Class<E1> exceptionClass1, - @Nullable Class<E2> exceptionClass2, - @Nullable Class<E3> exceptionClass3, - @Nullable Class<E4> exceptionClass4) { - checkValuesMissingBecauseOfFilteredError( - voes.values(), exceptionClass1, exceptionClass2, exceptionClass3, exceptionClass4); - } - - private <E1 extends Exception, E2 extends Exception, E3 extends Exception, E4 extends Exception> - void checkValuesMissingBecauseOfFilteredError( - Collection<ValueOrUntypedException> voes, - @Nullable Class<E1> exceptionClass1, - @Nullable Class<E2> exceptionClass2, - @Nullable Class<E3> exceptionClass3, - @Nullable Class<E4> exceptionClass4) { - if (!errorMightHaveBeenFound) { - // Short-circuit in the common case of no errors. - return; - } - for (ValueOrUntypedException voe : voes) { - SkyValue value = voe.getValue(); - if (value == null) { - Exception e = voe.getException(); - if (e == null - || ((exceptionClass1 == null || !exceptionClass1.isInstance(e)) - && (exceptionClass2 == null || !exceptionClass2.isInstance(e)) - && (exceptionClass3 == null || !exceptionClass3.isInstance(e)) - && (exceptionClass4 == null || !exceptionClass4.isInstance(e)))) { - valuesMissing = true; - // TODO(b/166268889): Remove when debugged. - if (e instanceof IOException) { - logger.atInfo().withStackTrace(StackSize.SMALL).withCause(e).log( - "IOException suppressed by lack of Skyframe declaration (%s %s %s %s)", - exceptionClass1, exceptionClass2, exceptionClass3, exceptionClass4); - } - return; - } - } - } - } - @Override public SkyframeIterableResult getOrderedValuesAndExceptions(Iterable<? extends SkyKey> depKeys) throws InterruptedException {
diff --git a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java index 9a87cf0..996e7e2 100644 --- a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
@@ -351,7 +351,7 @@ * SkyFunction} last build, meaning independently of the values of any other deps in this group * (although possibly depending on deps in earlier groups). Thus the caller may check all the deps * in this group in parallel, since the deps in all previous groups are verified unchanged. See - * {@link SkyFunction.Environment#getValues} for more on dependency groups. + * {@link SkyFunction.Environment#getOrderedValuesAndExceptions} for more on dependency groups. * * <p>The caller should register these as deps of this entry using {@link #addTemporaryDirectDeps} * before checking them.
diff --git a/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java index 11f3363..8d8eccf 100644 --- a/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java
@@ -17,7 +17,6 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.util.GroupedList; import com.google.devtools.build.skyframe.SkyFunction.Environment; -import java.util.Map; import java.util.function.Consumer; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -131,13 +130,6 @@ } @Override - public Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys) - throws InterruptedException { - recordDeps(depKeys); - return delegate.getValues(depKeys); - } - - @Override public boolean valuesMissing() { return delegate.valuesMissing(); }
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java index d0b1595..2245d6c 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
@@ -20,7 +20,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.util.GroupedList; -import java.util.Map; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -126,8 +125,8 @@ * * <p>On a subsequent evaluation, if any of this value's dependencies have changed they will be * re-evaluated in the same order as originally requested by the {@code SkyFunction} using this - * {@code getValue} call (see {@link #getValues} for when preserving the order is not - * important). + * {@code getValue} call (see {@link #getOrderedValuesAndExceptions} for when preserving the + * order is not important). * * <p>This method and the ones below may throw {@link InterruptedException}. Such exceptions * must not be caught by the {@link SkyFunction#compute} implementation. Instead, they should be @@ -178,18 +177,19 @@ throws E1, E2, E3, E4, InterruptedException; /** - * Requests {@code depKeys} "in parallel", independent of each others' values. These keys may be - * thought of as a "dependency group" -- they are requested together by this value. + * Requests {@code depKeys} "in parallel", independent of each others' results. These keys may + * be thought of as a "dependency group" -- they are requested together by this value. * * <p>In general, if the result of one getValue call can affect the argument of a later getValue - * call, the two calls cannot be merged into a single getValues call, since the result of the - * first call might change on a later evaluation. Inversely, if the result of one getValue call - * cannot affect the parameters of the next getValue call, the two keys can form a dependency - * group and the two getValue calls should be merged into one getValues call. In the latter - * case, if we fail to combine the _multiple_ getValue (or getValues) calls into one _single_ - * getValues call, it would result in multiple dependency groups with an implicit ordering - * between them. This would unnecessarily cause sequential evaluations of these groups and could - * impact overall performance. + * call, the two calls cannot be merged into a single getOrderedValuesAndExceptions call, since + * the result of the first call might change on a later evaluation. Inversely, if the result of + * one getValue call cannot affect the parameters of the next getValue call, the two keys can + * form a dependency group and the two getValue calls should be merged into one + * getOrderedValuesAndExceptions call. In the latter case, if we fail to combine the _multiple_ + * getValue (or getOrderedValuesAndExceptions) calls into one _single_ + * getOrderedValuesAndExceptions call, it would result in multiple dependency groups with an + * implicit ordering between them. This would unnecessarily cause sequential evaluations of + * these groups and could impact overall performance. * * <p>On subsequent evaluations, when checking to see if dependencies require re-evaluation, all * the values within one group may be simultaneously checked. A SkyFunction should request a @@ -204,48 +204,42 @@ * value, will request all values in the group again anyway, so they would have to have been * built in any case. * - * <p>Example of when to use getValues: A ListProcessor value is built with key inputListRef. - * The {@link #compute} method first calls getValue(InputList.key(inputListRef)), and retrieves - * inputList. It then iterates through inputList, calling getValue on each input. Finally, it - * processes the whole list and returns. Say inputList is (a, b, c). Since the {@link #compute} - * method will unconditionally call getValue(a), getValue(b), and getValue (c), the {@link - * #compute} method can instead just call getValues({a, b, c}). If the value is later dirtied - * the evaluator will evaluate a, b, and c in parallel (assuming the inputList value was - * unchanged), and re-evaluate the ListProcessor value only if at least one of them was changed. - * On the other hand, if the InputList changes to be (a, b, d), then the evaluator will see that - * the first dep has changed, and call the {@link #compute} method to re-evaluate from scratch, - * without considering the dep group of {a, b, c}. + * <p>Example of when to use getOrderedValuesAndExceptions: A ListProcessor value is built with + * key inputListRef. The {@link #compute} method first calls + * getValue(InputList.key(inputListRef)), and retrieves inputList. It then iterates through + * inputList, calling getValue on each input. Finally, it processes the whole list and returns. + * Say inputList is (a, b, c). Since the {@link #compute} method will unconditionally call + * getValue(a), getValue(b), and getValue (c), the {@link #compute} method can instead just call + * getOrderedValuesAndExceptions({a, b, c}). If the value is later dirtied the evaluator will + * evaluate a, b, and c in parallel (assuming the inputList value was unchanged), and + * re-evaluate the ListProcessor value only if at least one of them was changed. On the other + * hand, if the InputList changes to be (a, b, d), then the evaluator will see that the first + * dep has changed, and call the {@link #compute} method to re-evaluate from scratch, without + * considering the dep group of {a, b, c}. * - * <p>Example of when not to use getValues: A BestMatch value is built with key - * <potentialMatchesRef, matchCriterion>. The {@link #compute} method first calls + * <p>Example of when not to use getOrderedValuesAndExceptions: A BestMatch value is built with + * key <potentialMatchesRef, matchCriterion>. The {@link #compute} method first calls * getValue(PotentialMatches.key(potentialMatchesRef) and retrieves potentialMatches. It then * iterates through potentialMatches, calling getValue on each potential match until it finds * one that satisfies matchCriterion. In this case, if potentialMatches is (a, b, c), it would - * be <i>incorrect</i> to call getValues({a, b, c}), because it is not known yet whether - * requesting b or c will be necessary -- if a matches, then we will never call b or c. + * be <i>incorrect</i> to call getOrderedValuesAndExceptions({a, b, c}), because it is not known + * yet whether requesting b or c will be necessary -- if a matches, then we will never call b or + * c. * - * <p>Returns a map, {@code m}. For all {@code k} in {@code depKeys}, {@code m.containsKey(k)} - * is {@code true}, and, {@code m.get(k) != null} iff the dependency was already evaluated and - * was not in error. + * <p>Returns a {@link SkyframeIterableResult}, which contains the results in the same order as + * {@code depKeys}. */ - Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys) throws InterruptedException; - - /** - * Simailar to {@link #getValues}, but returns a {@link SkyframeLookupResult}, which contains - * the values of {@code depKeys}. Use in preference to all other getting methods (except for - * getOrderedValuesAndExceptions), since this method creates less garbage and allows the calling - * {@code SkyFunction} to declare exceptions on a per-SkyKey basis. - */ - SkyframeLookupResult getValuesAndExceptions(Iterable<? extends SkyKey> depKeys) + SkyframeIterableResult getOrderedValuesAndExceptions(Iterable<? extends SkyKey> depKeys) throws InterruptedException; /** - * Similar to {@link #getValues}, but returns a {@link SkyframeIterableResult}, which contains - * the values in the same order as {@code depKeys}. Use in preference to all other getting - * methods, since this method creates less garbage and allows the calling {@code SkyFunction} to - * declare exceptions on a per-SkyKey basis. + * Similar to {@link #getOrderedValuesAndExceptions}, but returns a {@link + * SkyframeLookupResult}, which allows the calling {@code SkyFunction} to get value or throw + * exception per SkyKey. + * + * <p>Use {@link #getOrderedValuesAndExceptions} in preference, since it creates less garbage. */ - SkyframeIterableResult getOrderedValuesAndExceptions(Iterable<? extends SkyKey> depKeys) + SkyframeLookupResult getValuesAndExceptions(Iterable<? extends SkyKey> depKeys) throws InterruptedException; /** @@ -254,7 +248,6 @@ * * <ul> * <li>getValue[OrThrow](k[, c]) returned {@code null} for some k - * <li>getValues(ks).get(k) == {@code null} for some ks and k such that ks.contains(k) * <li>A call to result#next[OrThrow]([c]) returned {@code null} where result = * getOrderedValuesAndExceptions(ks) for some ks * <li>A call to result#get[OrThrow](k[, c]) returned {@code null} where result =
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java index 413fb57..610945d 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -120,7 +120,7 @@ * The grouped list of values requested during this build as dependencies. On a subsequent build, * if this value is dirty, all deps in the same dependency group can be checked in parallel for * changes. In other words, if dep1 and dep2 are in the same group, then dep1 will be checked in - * parallel with dep2. See {@link SkyFunction.Environment#getValues} for more. + * parallel with dep2. See {@link SkyFunction.Environment#getValuesAndExceptions} for more. */ private final GroupedListHelper<SkyKey> newlyRequestedDeps = new GroupedListHelper<>();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java index e75c4e75..eb341c2 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java
@@ -190,8 +190,8 @@ buildTarget("//hello:BUILD"); // New package path on first build triggers full-graph work. calledGetValues.set(0); - // getValues() called during output file checking (although if an output service is able to - // report modified files in practice there is no iteration). + // getOrderedValuesAndExceptions() called during output file checking (although if an output + // service is able to report modified files in practice there is no iteration). buildTarget("//hello:BUILD"); assertThat(calledGetValues.getAndSet(0)).isEqualTo(1);