Incorporate skymeld inconsistencies into the rewinding/node dropping receivers.
Current state: blaze in skymeld + rewinding just uses the `RewindableGraphInconsistencyReceiver`, which doesn't take into account skymeld inconsistencies and is mutually exclusive with `SkymeldInconsistencyReceiver`.
After this CL: `RewindableGraphInconsistencyReceiver` and `NodeDroppingInconsistencyReceiver` handles skymeld-specific inconsistencies as well.
Code changes:
- `SkymeldInconsistencyReceiver` is removed and its content is distributed to the rewinding/node dropping inconsistency receivers.
Behavior changes:
- `RewindableGraphInconsistencyReceiver` and `NodeDroppingInconsistencyReceiver` handles skymeld-specific inconsistencies as well.
- Unexpected skymeld inconsistencies will throw, instead of sending a BugReport like before.
---
Since https://github.com/bazelbuild/bazel/commit/5b52f018b193be02fd11de9bc89c9f9d49bf5b3f, rewinding is already compatible with Skymeld.
PiperOrigin-RevId: 631332866
Change-Id: I3d222c23d5e40aa7a86546039bfb795f7b812c2c
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 a1b0f79..df4c63f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -187,7 +187,6 @@
":skyframe_executor_repository_helpers_holder",
":skyframe_incremental_build_monitor",
":skyframe_stats",
- ":skymeld_inconsistency_receiver",
":starlark_builtins_value",
":state_informing_sky_function_environment",
":target_completion_value",
@@ -3194,21 +3193,6 @@
)
java_library(
- name = "skymeld_inconsistency_receiver",
- srcs = ["SkymeldInconsistencyReceiver.java"],
- deps = [
- ":node_dropping_inconsistency_receiver",
- ":sky_functions",
- "//src/main/java/com/google/devtools/build/lib/bugreport",
- "//src/main/java/com/google/devtools/build/skyframe",
- "//src/main/java/com/google/devtools/build/skyframe:graph_inconsistency_java_proto",
- "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
- "//third_party:guava",
- "//third_party:jsr305",
- ],
-)
-
-java_library(
name = "sky_key_stats",
srcs = ["SkyKeyStats.java"],
)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/NodeDroppingInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/NodeDroppingInconsistencyReceiver.java
index 5283fa8..ebdf9dc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/NodeDroppingInconsistencyReceiver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/NodeDroppingInconsistencyReceiver.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.FileValue;
@@ -28,31 +27,51 @@
/**
* {@link GraphInconsistencyReceiver} for evaluations operating on graphs when {@code
- * --heuristically_drop_nodes} flag is applied.
+ * --heuristically_drop_nodes} flag is applied, or when some form of node dropping is done in
+ * combination with skymeld mode.
*
- * <p>The expected inconsistency caused by heuristically dropping state nodes should be tolerated
- * while all other inconsistencies should result in throwing an exception.
+ * <p>The expected inconsistency should be tolerated while all other inconsistencies should result
+ * in throwing an exception.
*
* <p>{@code RewindableGraphInconsistencyReceiver} implements similar logic to handle heuristically
* dropping state nodes.
*/
-public class NodeDroppingInconsistencyReceiver implements GraphInconsistencyReceiver {
+public final class NodeDroppingInconsistencyReceiver implements GraphInconsistencyReceiver {
+ private final boolean heuristicallyDropNodes;
+ private final boolean skymeldInconsistenciesExpected;
private static final ImmutableMap<SkyFunctionName, SkyFunctionName> EXPECTED_MISSING_CHILDREN =
ImmutableMap.of(
FileValue.FILE, FileStateKey.FILE_STATE,
SkyFunctions.DIRECTORY_LISTING, SkyFunctions.DIRECTORY_LISTING_STATE,
SkyFunctions.CONFIGURED_TARGET, GenQueryDirectPackageProviderFactory.GENQUERY_SCOPE);
+ // TODO: b/290998109#comment60 - After the GLOB nodes are replaced by GLOBS, the missing children
+ // below might be unexpected.
+ // These are only expected when Skymeld is enabled and we're dropping nodes.
+ private static final ImmutableMap<SkyFunctionName, SkyFunctionName>
+ SKYMELD_EXPECTED_MISSING_CHILDREN =
+ ImmutableMap.of(SkyFunctions.ACTION_EXECUTION, SkyFunctions.GLOB);
+
+ public NodeDroppingInconsistencyReceiver(
+ boolean heuristicallyDropNodes, boolean skymeldInconsistenciesExpected) {
+ this.heuristicallyDropNodes = heuristicallyDropNodes;
+ this.skymeldInconsistenciesExpected = skymeldInconsistenciesExpected;
+ }
+
@Override
public void noteInconsistencyAndMaybeThrow(
SkyKey key, @Nullable Collection<SkyKey> otherKeys, Inconsistency inconsistency) {
- checkState(
- isExpectedInconsistency(key, otherKeys, inconsistency),
- "Unexpected inconsistency: %s, %s, %s",
- key,
- otherKeys,
- inconsistency);
+ if (heuristicallyDropNodes && isExpectedInconsistency(key, otherKeys, inconsistency)) {
+ return;
+ }
+ if (skymeldInconsistenciesExpected
+ && isExpectedInconsistencySkymeld(key, otherKeys, inconsistency)) {
+ return;
+ }
+
+ throw new IllegalStateException(
+ String.format("Unexpected inconsistency: %s, %s, %s", key, otherKeys, inconsistency));
}
/**
@@ -61,19 +80,40 @@
*/
public static boolean isExpectedInconsistency(
SkyKey key, @Nullable Collection<SkyKey> otherKeys, Inconsistency inconsistency) {
- return isExpectedInconsistency(key, otherKeys, inconsistency, EXPECTED_MISSING_CHILDREN);
+ return isExpectedInternal(
+ key,
+ otherKeys,
+ inconsistency,
+ EXPECTED_MISSING_CHILDREN,
+ /* isSkymeldInconsistency= */ false);
}
- static boolean isExpectedInconsistency(
+ /**
+ * Checks whether the input inconsistency is an expected scenario caused by skymeld + some form of
+ * node dropping.
+ */
+ public static boolean isExpectedInconsistencySkymeld(
+ SkyKey key, @Nullable Collection<SkyKey> otherKeys, Inconsistency inconsistency) {
+ return isExpectedInternal(
+ key,
+ otherKeys,
+ inconsistency,
+ SKYMELD_EXPECTED_MISSING_CHILDREN,
+ /* isSkymeldInconsistency= */ true);
+ }
+
+ private static boolean isExpectedInternal(
SkyKey key,
@Nullable Collection<SkyKey> otherKeys,
Inconsistency inconsistency,
- ImmutableMap<SkyFunctionName, SkyFunctionName> expectedMissingChildrenTypes) {
- SkyFunctionName expectedMissingChildType = expectedMissingChildrenTypes.get(key.functionName());
+ ImmutableMap<SkyFunctionName, SkyFunctionName> expectedMissingChildTypes,
+ boolean isSkymeldInconsistency) {
+ SkyFunctionName expectedMissingChildType = expectedMissingChildTypes.get(key.functionName());
if (expectedMissingChildType == null) {
return false;
}
- if (inconsistency == Inconsistency.RESET_REQUESTED) {
+ // Skymeld shouldn't cause any inconsistency of this type.
+ if (!isSkymeldInconsistency && inconsistency == Inconsistency.RESET_REQUESTED) {
return otherKeys == null;
}
if (inconsistency == Inconsistency.ALREADY_DECLARED_CHILD_MISSING
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index faade35..38ce06c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -298,28 +298,27 @@
}
private GraphInconsistencyReceiver getGraphInconsistencyReceiverForCommand(
- OptionsProvider options) throws AbruptExitException {
- if (rewindingEnabled(options)) {
- // Currently incompatible with Skymeld i.e. this code path won't be run in Skymeld mode. We
- // may need to combine these GraphInconsistencyReceiver implementations in the future.
- var rewindableReceiver = new RewindableGraphInconsistencyReceiver();
- rewindableReceiver.setHeuristicallyDropNodes(heuristicallyDropNodes);
- return rewindableReceiver;
- }
- if (isMergedSkyframeAnalysisExecution()
- && ((options.getOptions(AnalysisOptions.class) != null
+ OptionsProvider options) {
+ var someNodeDroppingExpected =
+ (options.getOptions(AnalysisOptions.class) != null
&& options.getOptions(AnalysisOptions.class).discardAnalysisCache)
|| !trackIncrementalState
- || heuristicallyDropNodes)) {
- return new SkymeldInconsistencyReceiver(heuristicallyDropNodes);
+ || heuristicallyDropNodes;
+ var skymeldInconsistenciesExpected =
+ someNodeDroppingExpected && isMergedSkyframeAnalysisExecution();
+ if (rewindingEnabled(options)) {
+ return new RewindableGraphInconsistencyReceiver(
+ heuristicallyDropNodes, skymeldInconsistenciesExpected);
}
- if (heuristicallyDropNodes) {
- return new NodeDroppingInconsistencyReceiver();
+
+ if (heuristicallyDropNodes || skymeldInconsistenciesExpected) {
+ return new NodeDroppingInconsistencyReceiver(
+ heuristicallyDropNodes, skymeldInconsistenciesExpected);
}
return GraphInconsistencyReceiver.THROWING;
}
- private boolean rewindingEnabled(OptionsProvider options) throws AbruptExitException {
+ private boolean rewindingEnabled(OptionsProvider options) {
var buildRequestOptions = options.getOptions(BuildRequestOptions.class);
return buildRequestOptions != null && buildRequestOptions.rewindLostInputs;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java
deleted file mode 100644
index 7f57eef..0000000
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java
+++ /dev/null
@@ -1,62 +0,0 @@
-// Copyright 2023 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.skyframe;
-
-
-import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.bugreport.BugReport;
-import com.google.devtools.build.skyframe.GraphInconsistencyReceiver;
-import com.google.devtools.build.skyframe.SkyFunctionName;
-import com.google.devtools.build.skyframe.SkyKey;
-import com.google.devtools.build.skyframe.proto.GraphInconsistency.Inconsistency;
-import java.util.Collection;
-import javax.annotation.Nullable;
-
-/**
- * The {@link GraphInconsistencyReceiver} that tolerates inconsistencies resulted in dropping
- * pre-execution nodes in Skymeld mode.
- */
-public class SkymeldInconsistencyReceiver implements GraphInconsistencyReceiver {
- // TODO: b/290998109#comment60 - After the GLOB nodes are replaced by GLOBS, the missing children
- // below might be unexpected.
- private static final ImmutableMap<SkyFunctionName, SkyFunctionName>
- SKYMELD_EXPECTED_MISSING_CHILDREN =
- ImmutableMap.of(SkyFunctions.ACTION_EXECUTION, SkyFunctions.GLOB);
-
- private final boolean heuristicallyDropNodes;
-
- public SkymeldInconsistencyReceiver(boolean heuristicallyDropNodes) {
- this.heuristicallyDropNodes = heuristicallyDropNodes;
- }
-
- @Override
- public void noteInconsistencyAndMaybeThrow(
- SkyKey key, @Nullable Collection<SkyKey> otherKeys, Inconsistency inconsistency) {
- if (heuristicallyDropNodes
- && NodeDroppingInconsistencyReceiver.isExpectedInconsistency(
- key, otherKeys, inconsistency)) {
- // If `--heuristically_drop_nodes` is enabled, check whether the inconsistency is caused by
- // dropped state node. If so, tolerate the inconsistency and return.
- return;
- }
-
- if (!NodeDroppingInconsistencyReceiver.isExpectedInconsistency(
- key, otherKeys, inconsistency, SKYMELD_EXPECTED_MISSING_CHILDREN)) {
- // Instead of crashing, simply send a bug report here so we can evaluate whether this is an
- // actual bug or just something else to be added to the expected list.
- BugReport.logUnexpected(
- "Unexpected inconsistency: %s, %s, %s", key, otherKeys, inconsistency);
- }
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java
index 7b01edf..86ee46a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java
@@ -52,10 +52,13 @@
private final Multiset<Inconsistency> selfCounts = ConcurrentHashMultiset.create();
private final Multiset<Inconsistency> childCounts = ConcurrentHashMultiset.create();
private boolean rewindingInitiated = false;
- private boolean heuristicallyDropNodes = false;
+ private final boolean heuristicallyDropNodes;
+ private final boolean skymeldInconsistenciesExpected;
- public void setHeuristicallyDropNodes(boolean heuristicallyDropNodes) {
+ public RewindableGraphInconsistencyReceiver(
+ boolean heuristicallyDropNodes, boolean skymeldInconsistenciesExpected) {
this.heuristicallyDropNodes = heuristicallyDropNodes;
+ this.skymeldInconsistenciesExpected = skymeldInconsistenciesExpected;
}
@Override
@@ -69,6 +72,12 @@
return;
}
+ if (skymeldInconsistenciesExpected
+ && NodeDroppingInconsistencyReceiver.isExpectedInconsistencySkymeld(
+ key, otherKeys, inconsistency)) {
+ return;
+ }
+
// RESET_REQUESTED and PARENT_FORCE_REBUILD_OF_CHILD may be the first inconsistencies seen with
// rewinding. BUILDING_PARENT_FOUND_UNDONE_CHILD may also be seen, but it will not be the first.
switch (inconsistency) {
