Change `getValuesOrThrow` method to `getOrderedValuesAndExceptions` or `getValuesAndExceptions` in Transitive Function related files.
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.
The error message selection is also changed. Previously the error is obtained via traversal through entries of a `Map`, which is non-deterministic. The error message was selected by length and lexicographic order to make it deterministic. Now the traversal is deterministic, based on the key order, so the first error message is enough for determinism.
PiperOrigin-RevId: 431487328
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 42dde8a..54c4103 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -2760,6 +2760,7 @@
":sky_functions",
":transitive_base_traversal_function",
":transitive_traversal_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/events",
"//src/main/java/com/google/devtools/build/lib/packages",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
index d179d35..f269f3e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
@@ -39,11 +39,9 @@
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.ValueOrException2;
+import com.google.devtools.build.skyframe.SkyframeLookupResult;
import java.util.Collection;
import java.util.List;
-import java.util.Map;
-import java.util.Set;
import javax.annotation.Nullable;
/**
@@ -82,8 +80,8 @@
ProcessedTargetsT processedTargets,
EventHandler eventHandler,
TargetAndErrorIfAny targetAndErrorIfAny,
- Iterable<Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
- depEntries);
+ SkyframeLookupResult depEntries,
+ Iterable<? extends SkyKey> depKeys);
/**
* Returns a {@link SkyValue} based on the target and any errors it has, and the values
@@ -114,9 +112,7 @@
// skyframe for building this node was for the corresponding PackageValue.
Collection<SkyKey> labelDepKeys = getLabelDepKeys(env, targetAndErrorIfAny);
- Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap =
- env.getValuesOrThrow(labelDepKeys, NoSuchPackageException.class,
- NoSuchTargetException.class);
+ SkyframeLookupResult depMap = env.getValuesAndExceptions(labelDepKeys);
if (env.valuesMissing()) {
return null;
}
@@ -124,18 +120,19 @@
// made to skyframe for building this node was for the corresponding PackageValue.
Iterable<SkyKey> labelAspectKeys =
getStrictLabelAspectDepKeys(env, depMap, targetAndErrorIfAny);
- Set<Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
- labelAspectEntries =
- env.getValuesOrThrow(
- labelAspectKeys, NoSuchPackageException.class, NoSuchTargetException.class)
- .entrySet();
+ SkyframeLookupResult labelAspectEntries = env.getValuesAndExceptions(labelAspectKeys);
if (env.valuesMissing()) {
return null;
}
ProcessedTargetsT processedTargets = processTarget(targetAndErrorIfAny);
- processDeps(processedTargets, env.getListener(), targetAndErrorIfAny, depMap.entrySet());
- processDeps(processedTargets, env.getListener(), targetAndErrorIfAny, labelAspectEntries);
+ processDeps(processedTargets, env.getListener(), targetAndErrorIfAny, depMap, labelDepKeys);
+ processDeps(
+ processedTargets,
+ env.getListener(),
+ targetAndErrorIfAny,
+ labelAspectEntries,
+ labelAspectKeys);
return computeSkyValue(targetAndErrorIfAny, processedTargets);
}
@@ -153,7 +150,7 @@
Iterable<SkyKey> getStrictLabelAspectDepKeys(
SkyFunction.Environment env,
- Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap,
+ SkyframeLookupResult depMap,
TargetAndErrorIfAny targetAndErrorIfAny)
throws InterruptedException {
return getStrictLabelAspectKeys(targetAndErrorIfAny.getTarget(), depMap, env);
@@ -171,10 +168,7 @@
* dependencies from the env to do so.
*/
private Iterable<SkyKey> getStrictLabelAspectKeys(
- Target target,
- Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap,
- Environment env)
- throws InterruptedException {
+ Target target, SkyframeLookupResult depMap, Environment env) throws InterruptedException {
if (!(target instanceof Rule)) {
// Aspects can be declared only for Rules.
return ImmutableList.of();
@@ -204,20 +198,21 @@
@Nullable
protected abstract AdvertisedProviderSet getAdvertisedProviderSet(
- Label toLabel,
- @Nullable ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
- Environment env)
- throws InterruptedException;
+ Label toLabel, SkyValue toVal, Environment env) throws InterruptedException;
private final boolean hasDepThatSatisfies(
- Aspect aspect,
- Iterable<Label> depLabels,
- Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> fullDepMap,
- Environment env)
+ Aspect aspect, Iterable<Label> depLabels, SkyframeLookupResult fullDepMap, Environment env)
throws InterruptedException {
for (Label depLabel : depLabels) {
- AdvertisedProviderSet advertisedProviderSet =
- getAdvertisedProviderSet(depLabel, fullDepMap.get(depLabel), env);
+ SkyValue toVal;
+ try {
+ toVal =
+ fullDepMap.getOrThrow(
+ getKey(depLabel), NoSuchPackageException.class, NoSuchTargetException.class);
+ } catch (NoSuchPackageException | NoSuchTargetException e) {
+ continue;
+ }
+ AdvertisedProviderSet advertisedProviderSet = getAdvertisedProviderSet(depLabel, toVal, env);
if (advertisedProviderSet != null
&& AspectDefinition.satisfies(aspect, advertisedProviderSet)) {
return true;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
index 9003587..9141619 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -29,8 +30,7 @@
import com.google.devtools.build.lib.skyframe.TransitiveTargetFunction.TransitiveTargetValueBuilder;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-import com.google.devtools.build.skyframe.ValueOrException2;
-import java.util.Map;
+import com.google.devtools.build.skyframe.SkyframeLookupResult;
import javax.annotation.Nullable;
/**
@@ -62,25 +62,30 @@
TransitiveTargetValueBuilder builder,
EventHandler eventHandler,
TargetAndErrorIfAny targetAndErrorIfAny,
- Iterable<Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
- depEntries) {
+ SkyframeLookupResult depEntries,
+ Iterable<? extends SkyKey> depKeys) {
boolean successfulTransitiveLoading = builder.isSuccessfulTransitiveLoading();
Target target = targetAndErrorIfAny.getTarget();
- for (Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> entry :
- depEntries) {
- Label depLabel = ((TransitiveTargetKey) entry.getKey()).getLabel();
+ for (SkyKey skyKey : depKeys) {
+ Label depLabel = ((TransitiveTargetKey) skyKey).getLabel();
TransitiveTargetValue transitiveTargetValue;
try {
- transitiveTargetValue = (TransitiveTargetValue) entry.getValue().get();
- if (transitiveTargetValue == null) {
- continue;
- }
+ transitiveTargetValue =
+ (TransitiveTargetValue)
+ depEntries.getOrThrow(
+ skyKey, NoSuchPackageException.class, NoSuchTargetException.class);
} catch (NoSuchPackageException | NoSuchTargetException e) {
successfulTransitiveLoading = false;
maybeReportErrorAboutMissingEdge(target, depLabel, e, eventHandler);
continue;
}
+ if (transitiveTargetValue == null) {
+ BugReport.sendBugReport(
+ new IllegalStateException(
+ "TransitiveTargetValue " + skyKey + " was missing, this should never happen"));
+ continue;
+ }
builder.getTransitiveTargets().addTransitive(transitiveTargetValue.getTransitiveTargets());
if (transitiveTargetValue.encounteredLoadingError()) {
successfulTransitiveLoading = false;
@@ -104,10 +109,7 @@
@Nullable
@Override
protected AdvertisedProviderSet getAdvertisedProviderSet(
- Label toLabel,
- @Nullable ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
- Environment env)
- throws InterruptedException {
+ Label toLabel, SkyValue toVal, Environment env) throws InterruptedException {
SkyKey packageKey = PackageValue.key(toLabel.getPackageIdentifier());
Target toTarget;
try {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
index 585d95c..082056a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
@@ -14,21 +14,19 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.AdvertisedProviderSet;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
-import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.util.GroupedList;
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.ValueOrException2;
+import com.google.devtools.build.skyframe.SkyframeLookupResult;
import java.util.Collection;
-import java.util.Comparator;
import java.util.List;
-import java.util.Map;
import javax.annotation.Nullable;
/**
@@ -40,7 +38,7 @@
*/
public class TransitiveTraversalFunction
extends TransitiveBaseTraversalFunction<
- TransitiveTraversalFunction.DeterministicErrorMessageAccumulator> {
+ TransitiveTraversalFunction.FirstErrorMessageAccumulator> {
@Override
Label argumentFromKey(SkyKey key) {
@@ -53,31 +51,36 @@
}
@Override
- DeterministicErrorMessageAccumulator processTarget(TargetAndErrorIfAny targetAndErrorIfAny) {
+ FirstErrorMessageAccumulator processTarget(TargetAndErrorIfAny targetAndErrorIfAny) {
NoSuchTargetException errorIfAny = targetAndErrorIfAny.getErrorLoadingTarget();
String errorMessageIfAny = errorIfAny == null ? null : errorIfAny.getMessage();
- return DeterministicErrorMessageAccumulator.create(errorMessageIfAny);
+ return new FirstErrorMessageAccumulator(errorMessageIfAny);
}
@Override
void processDeps(
- DeterministicErrorMessageAccumulator accumulator,
+ FirstErrorMessageAccumulator accumulator,
EventHandler eventHandler,
TargetAndErrorIfAny targetAndErrorIfAny,
- Iterable<Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
- depEntries) {
- for (Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> entry :
- depEntries) {
+ SkyframeLookupResult depEntries,
+ Iterable<? extends SkyKey> depKeys) {
+ for (SkyKey skyKey : depKeys) {
TransitiveTraversalValue transitiveTraversalValue;
try {
- transitiveTraversalValue = (TransitiveTraversalValue) entry.getValue().get();
- if (transitiveTraversalValue == null) {
- continue;
- }
+ transitiveTraversalValue =
+ (TransitiveTraversalValue)
+ depEntries.getOrThrow(
+ skyKey, NoSuchPackageException.class, NoSuchTargetException.class);
} catch (NoSuchPackageException | NoSuchTargetException e) {
accumulator.maybeSet(e.getMessage());
continue;
}
+ if (transitiveTraversalValue == null) {
+ BugReport.sendBugReport(
+ new IllegalStateException(
+ "TransitiveTargetValue " + skyKey + " was missing, this should never happen"));
+ continue;
+ }
String errorMessage = transitiveTraversalValue.getErrorMessage();
if (errorMessage != null) {
accumulator.maybeSet(errorMessage);
@@ -85,29 +88,17 @@
}
}
- @Nullable
@Override
protected AdvertisedProviderSet getAdvertisedProviderSet(
- Label toLabel,
- @Nullable ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal,
- Environment env) {
- if (toVal == null) {
- return null;
- }
- try {
- return ((TransitiveTraversalValue) toVal.get()).getProviders();
- } catch (NoSuchThingException e) {
- // Do nothing interesting. This error was handled when we computed the corresponding
- // TransitiveTargetValue.
- return null;
- }
+ Label toLabel, SkyValue toVal, Environment env) {
+ return ((TransitiveTraversalValue) toVal).getProviders();
}
@Override
SkyValue computeSkyValue(
- TargetAndErrorIfAny targetAndErrorIfAny, DeterministicErrorMessageAccumulator accumulator) {
+ TargetAndErrorIfAny targetAndErrorIfAny, FirstErrorMessageAccumulator accumulator) {
boolean targetLoadedSuccessfully = targetAndErrorIfAny.getErrorLoadingTarget() == null;
- String errorMessage = accumulator.getErrorMessage();
+ String errorMessage = accumulator.getFirstErrorMessage();
return targetLoadedSuccessfully
? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget(), errorMessage)
: TransitiveTraversalValue.unsuccessfulTransitiveTraversal(
@@ -132,7 +123,7 @@
@Override
Iterable<SkyKey> getStrictLabelAspectDepKeys(
SkyFunction.Environment env,
- Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap,
+ SkyframeLookupResult depMap,
TargetAndErrorIfAny targetAndErrorIfAny)
throws InterruptedException {
// As a performance optimization we may already know the deps we are about to request from
@@ -172,47 +163,28 @@
}
/**
- * Keeps track of a deterministic error message encountered while traversing itself and its
- * dependencies: either the error it was initialized with, or the shortest error it encounters,
- * with ties broken alphabetically.
- *
- * <p>This preserves the behavior that the local target's error is the most important, and is
- * cheap (constant-time) to compute the comparison between strings, unless they have the same
- * length, which is unlikely.
+ * Keeps track of the first error message encountered while traversing itself and its
+ * dependencies.
*/
- interface DeterministicErrorMessageAccumulator {
- @Nullable
- String getErrorMessage();
+ static class FirstErrorMessageAccumulator {
- default void maybeSet(String errorMessage) {}
+ @Nullable private String firstErrorMessage;
- static DeterministicErrorMessageAccumulator create(@Nullable String errorMessage) {
- if (errorMessage != null) {
- return () -> errorMessage;
- }
- return new UpdateableErrorMessageAccumulator();
+ public FirstErrorMessageAccumulator(@Nullable String firstErrorMessage) {
+ this.firstErrorMessage = firstErrorMessage;
}
- class UpdateableErrorMessageAccumulator implements DeterministicErrorMessageAccumulator {
- private static final Comparator<String> LENGTH_THEN_ALPHABETICAL =
- Comparator.nullsLast(
- Comparator.comparingInt(String::length).thenComparing(Comparator.naturalOrder()));
-
- @Nullable private String errorMessage;
-
- @Override
- public void maybeSet(String errorMessage) {
- Preconditions.checkNotNull(errorMessage);
- if (LENGTH_THEN_ALPHABETICAL.compare(this.errorMessage, errorMessage) > 0) {
- this.errorMessage = errorMessage;
- }
+ /** Remembers {@code errorMessage} if it is the first error message. */
+ void maybeSet(String errorMessage) {
+ Preconditions.checkNotNull(errorMessage);
+ if (firstErrorMessage == null) {
+ firstErrorMessage = errorMessage;
}
+ }
- @Nullable
- @Override
- public String getErrorMessage() {
- return errorMessage;
- }
+ @Nullable
+ String getFirstErrorMessage() {
+ return firstErrorMessage;
}
}
}
