Skip to content

Commit

Permalink
Remove the ability of ActionFileSystem to cause Skyframe restarts.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lberki authored and copybara-github committed Oct 16, 2023
1 parent 0adcc8d commit 774fdb4
Show file tree
Hide file tree
Showing 4 changed files with 1 addition and 60 deletions.
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ java_library(
"MapBasedActionGraph.java",
"MiddlemanAction.java",
"MiddlemanFactory.java",
"MissingDepExecException.java",
"MissingInputFileException.java",
"MutableActionGraph.java",
"NotifyOnActionCacheHit.java",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -28,11 +27,6 @@ public interface InputMetadataProvider {
* <p>The returned {@link FileArtifactValue} instance corresponds to the final target of a symlink
* and therefore must not have a type of {@link FileStateType#SYMLINK}.
*
* <p>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)
Expand All @@ -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;
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 774fdb4

Please sign in to comment.