From 774fdb4be128b642332531f1d0376810b4c5377f Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 16 Oct 2023 00:37:26 -0700 Subject: [PATCH] Remove the ability of ActionFileSystem to cause Skyframe restarts. This mechanism was necessary due to an oddity of the action graph: normally, every input file an action requires is a direct dependency of that action. This didn't use to be the case in one single scenario, C++ header files: these are not necessarily inputs of the action (they are discovered by include scanning) and this used to be expressed by hiding them behind a scheduling middleman action. This, coupled with the fact that Skybuild only guarantees that the direct dependencies of an function are available (but not transitive ones), sometimes necessitated Skyframe restarts. Now scheduling middlemen are gone and headers are also direct (ignoring ArtifactNestedSetValue) Skyframe dependencies of C++ compilation actions, so these restarts can't happen anymore and thus the code is not necessary, either. It's also probably unnecessary for ActionFileSystem to call env.getValue() and the same thing could be implemented by collecting the same data from the SkyValue of the scheduling dependencies, but that requires some more thinking; it's not obvious how to do that without creating too much garbage. RELNOTES: None. PiperOrigin-RevId: 573729592 Change-Id: Id169bbcc3df9f8e92bfab1046b3c4aa7a5da734d --- .../google/devtools/build/lib/actions/BUILD | 1 - .../lib/actions/InputMetadataProvider.java | 14 -------- .../lib/actions/MissingDepExecException.java | 35 ------------------- .../includescanning/LegacyIncludeScanner.java | 11 +----- 4 files changed, 1 insertion(+), 60 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/actions/MissingDepExecException.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 61448de6fc4dac..f458831b49bdde 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -109,7 +109,6 @@ java_library( "MapBasedActionGraph.java", "MiddlemanAction.java", "MiddlemanFactory.java", - "MissingDepExecException.java", "MissingInputFileException.java", "MutableActionGraph.java", "NotifyOnActionCacheHit.java", diff --git a/src/main/java/com/google/devtools/build/lib/actions/InputMetadataProvider.java b/src/main/java/com/google/devtools/build/lib/actions/InputMetadataProvider.java index 82b325a9852973..68b88eac9402a0 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/InputMetadataProvider.java +++ b/src/main/java/com/google/devtools/build/lib/actions/InputMetadataProvider.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.actions; -import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.vfs.FileSystem; import java.io.IOException; @@ -28,11 +27,6 @@ public interface InputMetadataProvider { *

The returned {@link FileArtifactValue} instance corresponds to the final target of a symlink * and therefore must not have a type of {@link FileStateType#SYMLINK}. * - *

If {@link #mayGetGeneratingActionsFromSkyframe} is {@code true} and the {@linkplain - * DerivedArtifact#getGeneratingActionKey generating action} is not immediately available, this - * method returns {@code null} to signify that a skyframe restart is necessary to obtain the - * metadata for the requested {@link Artifact.DerivedArtifact}. - * * @param input the input to retrieve the digest for * @return the artifact's digest or null if digest cannot be obtained (due to artifact * non-existence, lookup errors, or any other reason) @@ -58,12 +52,4 @@ public interface InputMetadataProvider { default FileSystem getFileSystemForInputResolution() { return null; } - - /** - * Indicates whether calls to {@link #getInputMetadata} with a {@link Artifact.DerivedArtifact} - * may require a skyframe lookup. - */ - default boolean mayGetGeneratingActionsFromSkyframe() { - return false; - } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/MissingDepExecException.java b/src/main/java/com/google/devtools/build/lib/actions/MissingDepExecException.java deleted file mode 100644 index 806cde4720d343..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/actions/MissingDepExecException.java +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright 2018 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.actions; - -import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; - -/** - * Exception to be thrown if an action failed to execute because it is missing Skyframe - * dependencies. - * - *

This is expected to be possible when {@link - * InputMetadataProvider#mayGetGeneratingActionsFromSkyframe} is {@code true}. - */ -public final class MissingDepExecException extends ExecException { - - public MissingDepExecException() { - super("Missing skyframe dependency"); - } - - @Override - protected FailureDetail getFailureDetail(String message) { - throw new UnsupportedOperationException("MissingDepException should be handled"); - } -} diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java index 444baef80ddf3c..bcd97d25b166ce 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java @@ -25,7 +25,6 @@ import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.actions.MissingDepExecException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor; import com.google.devtools.build.lib.concurrent.ErrorClassifier; @@ -608,15 +607,7 @@ public final void processAsync( grepIncludes, includeScanningHeaderData); - try { - visitor.processInternal(mainSource, sources, cmdlineIncludes, includes, pathHints); - } catch (MissingDepExecException e) { - // This happens when a skyframe restart is necessary. Callers are responsible for checking - // env.valuesMissing() as per this method's contract, so we can just ignore the exception. - if (!env.valuesMissing()) { - throw new IllegalStateException("Missing dep without skyframe request", e); - } - } + visitor.processInternal(mainSource, sources, cmdlineIncludes, includes, pathHints); } private static void checkForInterrupt(String operation, Object source)