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);
