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