Automated rollback of commit c004ff117ad72fd387102600e4be47fddf7bc2ae.
*** Reason for rollback ***
Second attempt; rollout with a flag this time.
The original change should have been a no-op, but twerth@ swears that
his binary search narrowed down to it, so we're being super extra
careful this time, having been unable to find the root cause to the
issue that caused the rollback.
*** Original change description ***
Remove PerActionFileCache
Instead, make ActionMetadataHandler implement the MetadataProvider interface.
This fixes an issue where an action that runs two spawns where one depends on
an output of the other was unable to get the metadata for the intermediate
output.
We don't currently have actions that do this, but we will have in a future
change (which will also implicitly act as a regression test).
PiperOrigin-RevId: 214919699
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
index 62d03fd..fc61397 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
@@ -17,6 +17,7 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.vfs.FileStatus;
import java.io.IOException;
@@ -30,15 +31,9 @@
* <p>Note that implementations of this interface call chmod on output files if {@link
* #discardOutputMetadata} has been called.
*/
-public interface MetadataHandler {
- /**
- * Returns metadata for the given artifact or throws an exception if the metadata could not be
- * obtained.
- *
- * @return metadata instance
- * @throws IOException if metadata could not be obtained.
- */
- FileArtifactValue getMetadata(Artifact artifact) throws IOException;
+public interface MetadataHandler extends MetadataProvider {
+ @Override
+ FileArtifactValue getMetadata(ActionInput actionInput) throws IOException;
/** Sets digest for virtual artifacts (e.g. middlemen). {@code md5Digest} must not be null. */
void setDigestForVirtualArtifact(Artifact artifact, Md5Digest md5Digest);
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
index a477982..bb82c1a 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
@@ -406,6 +406,20 @@
help = "This option is deprecated and has no effect.")
public boolean discardActionsAfterExecution;
+ @Option(
+ name = "incompatible_use_per_action_file_cache",
+ defaultValue = "true",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+ metadataTags = {
+ OptionMetadataTag.INCOMPATIBLE_CHANGE,
+ OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+ },
+ help = "Whether to use the per action file cache. We saw issues with a previous rollout "
+ + "attempt (which we could not track down to a root cause), so we are extra careful now "
+ + "and use a flag to enable the new code path.")
+ public boolean usePerActionFileCache;
+
/** Converter for jobs: [0, MAX_JOBS] or "auto". */
public static class JobsConverter extends RangeConverter {
/**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index e21d8db..d5adb33 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -34,6 +34,7 @@
import com.google.devtools.build.lib.actions.ArtifactSkyKey;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
+import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.MissingDepException;
import com.google.devtools.build.lib.actions.MissingInputFileException;
import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
@@ -67,7 +68,6 @@
import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicReference;
-import java.util.function.Supplier;
import javax.annotation.Nullable;
/**
@@ -383,15 +383,14 @@
// The metadataHandler may be recreated (via the supplier) if we discover inputs.
ArtifactPathResolver pathResolver = ArtifactPathResolver.createPathResolver(
state.actionFileSystem, skyframeActionExecutor.getExecRoot());
- Supplier<ActionMetadataHandler> metadataHandlerSupplier =
- () ->
- new ActionMetadataHandler(
- state.inputArtifactData,
- action.getOutputs(),
- tsgm.get(),
- pathResolver,
- state.actionFileSystem == null ? new OutputStore() : new MinimalOutputStore());
- ActionMetadataHandler metadataHandler = metadataHandlerSupplier.get();
+ ActionMetadataHandler metadataHandler =
+ new ActionMetadataHandler(
+ state.inputArtifactData,
+ /* missingArtifactsAllowed= */ action.discoversInputs(),
+ action.getOutputs(),
+ tsgm.get(),
+ pathResolver,
+ state.actionFileSystem == null ? new OutputStore() : new MinimalOutputStore());
long actionStartTime = BlazeClock.nanoTime();
// We only need to check the action cache if we haven't done it on a previous run.
if (!state.hasCheckedActionCache()) {
@@ -427,9 +426,10 @@
// This may be recreated if we discover inputs.
// TODO(shahan): this isn't used when using ActionFileSystem so we can avoid creating some
// unused objects.
- PerActionFileCache perActionFileCache =
- new PerActionFileCache(
- state.inputArtifactData, /*missingArtifactsAllowed=*/ action.discoversInputs());
+ MetadataProvider perActionFileCache = skyframeActionExecutor.usePerFileActionCache()
+ ? new PerActionFileCache(
+ state.inputArtifactData, /*missingArtifactsAllowed=*/ action.discoversInputs())
+ : metadataHandler;
if (action.discoversInputs()) {
if (state.discoveredInputs == null) {
try {
@@ -459,12 +459,20 @@
if (env.valuesMissing()) {
return null;
}
- perActionFileCache =
- new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false);
-
- metadataHandler = metadataHandlerSupplier.get();
+ metadataHandler =
+ new ActionMetadataHandler(
+ state.inputArtifactData,
+ /* missingArtifactsAllowed= */ false,
+ action.getOutputs(),
+ tsgm.get(),
+ pathResolver,
+ state.actionFileSystem == null ? new OutputStore() : new MinimalOutputStore());
// Set the MetadataHandler to accept output information.
metadataHandler.discardOutputMetadata();
+
+ perActionFileCache = skyframeActionExecutor.usePerFileActionCache()
+ ? new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false)
+ : metadataHandler;
}
// Make sure this is a regular HashMap rather than ImmutableMapBuilder so that we are safe
@@ -542,7 +550,14 @@
// the documentation on MetadataHandler.artifactOmitted. This works by accident because
// markOmitted is only called for remote execution, and this code only gets executed for
// local execution.
- metadataHandler = metadataHandlerSupplier.get();
+ metadataHandler =
+ new ActionMetadataHandler(
+ state.inputArtifactData,
+ /*missingArtifactsAllowed=*/ false,
+ action.getOutputs(),
+ tsgm.get(),
+ pathResolver,
+ state.actionFileSystem == null ? new OutputStore() : new MinimalOutputStore());
}
}
Preconditions.checkState(!env.valuesMissing(), action);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index 79bde1c..e05fbe1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -80,6 +80,7 @@
* <p>This should never be read directly. Use {@link #getInputFileArtifactValue} instead.
*/
private final ActionInputMap inputArtifactData;
+ private final boolean missingArtifactsAllowed;
/** Outputs that are to be omitted. */
private final Set<Artifact> omittedOutputs = Sets.newConcurrentHashSet();
@@ -90,6 +91,7 @@
* The timestamp granularity monitor for this build.
* Use {@link #getTimestampGranularityMonitor(Artifact)} to fetch this member.
*/
+ @Nullable
private final TimestampGranularityMonitor tsgm;
private final ArtifactPathResolver artifactPathResolver;
@@ -104,11 +106,13 @@
@VisibleForTesting
public ActionMetadataHandler(
ActionInputMap inputArtifactData,
+ boolean missingArtifactsAllowed,
Iterable<Artifact> outputs,
- TimestampGranularityMonitor tsgm,
+ @Nullable TimestampGranularityMonitor tsgm,
ArtifactPathResolver artifactPathResolver,
OutputStore store) {
this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData);
+ this.missingArtifactsAllowed = missingArtifactsAllowed;
this.outputs = ImmutableSet.copyOf(outputs);
this.tsgm = tsgm;
this.artifactPathResolver = artifactPathResolver;
@@ -124,6 +128,7 @@
* @param artifact the artifact for which to fetch the timestamp granularity monitor
* @return the timestamp granularity monitor to use, which may be null
*/
+ @Nullable
private TimestampGranularityMonitor getTimestampGranularityMonitor(Artifact artifact) {
return artifact.isConstantMetadata() ? null : tsgm;
}
@@ -151,15 +156,25 @@
}
@Override
- public FileArtifactValue getMetadata(Artifact artifact) throws IOException {
+ public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException {
+ // TODO(shahan): is this bypass needed?
+ if (!(actionInput instanceof Artifact)) {
+ return null;
+ }
+
+ Artifact artifact = (Artifact) actionInput;
FileArtifactValue value = getInputFileArtifactValue(artifact);
if (value != null) {
return metadataFromValue(value);
}
+
if (artifact.isSourceArtifact()) {
// A discovered input we didn't have data for.
// TODO(bazel-team): Change this to an assertion once Skyframe has native input discovery, so
// all inputs will already have metadata known.
+ if (!missingArtifactsAllowed) {
+ throw new IllegalStateException(String.format("null for %s", artifact));
+ }
return null;
} else if (artifact.isMiddlemanArtifact()) {
// A middleman artifact's data was either already injected from the action cache checker using
@@ -180,7 +195,11 @@
// Calling code depends on this particular exception.
throw new FileNotFoundException(artifact + " not found");
}
- // It's an ordinary artifact.
+ // Fallthrough: the artifact must be a non-tree, non-middleman output artifact.
+
+ // Check for existing metadata. It may have been injected. In either case, this method is called
+ // from SkyframeActionExecutor to make sure that we have metadata for all action outputs, as the
+ // results are then stored in Skyframe (and the action cache).
ArtifactFileMetadata fileMetadata = store.getArtifactData(artifact);
if (fileMetadata != null) {
// Non-middleman artifacts should only have additionalOutputData if they have
@@ -197,14 +216,30 @@
}
return FileArtifactValue.createNormalFile(fileMetadata);
}
- // We do not cache exceptions besides nonexistence here, because it is unlikely that the file
- // will be requested from this cache too many times.
+
+ // No existing metadata; this can happen if the output metadata is not injected after a spawn
+ // is executed. SkyframeActionExecutor.checkOutputs calls this method for every output file of
+ // the action, which hits this code path. Another possibility is that an action runs multiple
+ // spawns, and a subsequent spawn requests the metadata of an output of a previous spawn.
+ //
+ // Stat the file. All output artifacts of an action are deleted before execution, so if a file
+ // exists, it was most likely created by the current action. There is a race condition here if
+ // an external process creates (or modifies) the file between the deletion and this stat, which
+ // we cannot solve.
+ //
+ // We only cache nonexistence here, not file system errors. It is unlikely that the file will be
+ // requested from this cache too many times.
fileMetadata = constructArtifactFileMetadata(artifact, /*statNoFollow=*/ null);
return maybeStoreAdditionalData(artifact, fileMetadata, null);
}
+ @Override
+ public ActionInput getInput(String execPath) {
+ return inputArtifactData.getInput(execPath);
+ }
+
/**
- * See {@link Outputstore#getAllAdditionalOutputData} for why we sometimes need to store
+ * See {@link OutputStore#getAllAdditionalOutputData} for why we sometimes need to store
* additional data, even for normal (non-middleman) artifacts.
*/
@Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 646ff05..7ed0f92 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -155,6 +155,7 @@
// findAndStoreArtifactConflicts, and is preserved across builds otherwise.
private ImmutableMap<ActionAnalysisMetadata, ConflictException> badActionMap = ImmutableMap.of();
private OptionsProvider options;
+ private boolean usePerFileActionCache;
private boolean hadExecutionError;
private MetadataProvider perBuildFileCache;
private ActionInputPrefetcher actionInputPrefetcher;
@@ -358,6 +359,8 @@
this.actionCacheChecker = Preconditions.checkNotNull(actionCacheChecker);
// Don't cache possibly stale data from the last build.
this.options = options;
+ this.usePerFileActionCache =
+ options.getOptions(BuildRequestOptions.class).usePerActionFileCache;
// Cache the finalizeActions value for performance, since we consult it on every action.
this.finalizeActions = options.getOptions(BuildRequestOptions.class).finalizeActions;
this.outputService = outputService;
@@ -373,6 +376,10 @@
this.clientEnv = ImmutableMap.copyOf(clientEnv);
}
+ boolean usePerFileActionCache() {
+ return usePerFileActionCache;
+ }
+
boolean usesActionFileSystem() {
return outputService != null && outputService.supportsActionFileSystem();
}
@@ -509,7 +516,7 @@
* tasks related to that action.
*/
public ActionExecutionContext getContext(
- MetadataProvider graphFileCache,
+ MetadataProvider perActionFileCache,
MetadataHandler metadataHandler,
Map<Artifact, Collection<Artifact>> expandedInputs,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets,
@@ -520,7 +527,7 @@
ArtifactPathResolver.createPathResolver(actionFileSystem, executorEngine.getExecRoot()));
return new ActionExecutionContext(
executorEngine,
- createFileCache(graphFileCache, actionFileSystem),
+ createFileCache(perActionFileCache, actionFileSystem),
actionInputPrefetcher,
actionKeyContext,
metadataHandler,
@@ -643,7 +650,7 @@
*/
Iterable<Artifact> discoverInputs(
Action action,
- PerActionFileCache graphFileCache,
+ MetadataProvider perActionFileCache,
MetadataHandler metadataHandler,
Environment env,
@Nullable FileSystem actionFileSystem)
@@ -651,7 +658,7 @@
ActionExecutionContext actionExecutionContext =
ActionExecutionContext.forInputDiscovery(
executorEngine,
- createFileCache(graphFileCache, actionFileSystem),
+ createFileCache(perActionFileCache, actionFileSystem),
actionInputPrefetcher,
actionKeyContext,
metadataHandler,
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
index a84efb3..eba285b 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
@@ -333,8 +333,11 @@
/** A fake metadata handler that is able to obtain metadata from the file system. */
private static class FakeMetadataHandler extends FakeMetadataHandlerBase {
@Override
- public FileArtifactValue getMetadata(Artifact artifact) throws IOException {
- return FileArtifactValue.create(artifact);
+ public FileArtifactValue getMetadata(ActionInput input) throws IOException {
+ if (!(input instanceof Artifact)) {
+ return null;
+ }
+ return FileArtifactValue.create((Artifact) input);
}
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
index 1aa4022..32caf5b 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
@@ -78,6 +78,7 @@
import com.google.devtools.build.skyframe.BuildDriver;
import com.google.devtools.build.skyframe.ErrorInfo;
import com.google.devtools.build.skyframe.EvaluationResult;
+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.ValueOrUntypedException;
@@ -712,7 +713,12 @@
*/
public static class FakeMetadataHandlerBase implements MetadataHandler {
@Override
- public FileArtifactValue getMetadata(Artifact artifact) throws IOException {
+ public FileArtifactValue getMetadata(ActionInput input) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public ActionInput getInput(String execPath) {
throw new UnsupportedOperationException();
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
new file mode 100644
index 0000000..8c39997
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
@@ -0,0 +1,115 @@
+// 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.skyframe;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.ActionInputMap;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.testutil.Scratch;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link ActionMetadataHandler}. */
+@RunWith(JUnit4.class)
+public class ActionMetadataHandlerTest {
+ private Scratch scratch;
+ private ArtifactRoot sourceRoot;
+
+ @Before
+ public final void setRootDir() throws Exception {
+ scratch = new Scratch();
+ sourceRoot = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.dir("/workspace")));
+ }
+
+ @Test
+ public void withNonArtifactInput() throws Exception {
+ ActionInput input = ActionInputHelper.fromPath("foo/bar");
+ FileArtifactValue metadata = FileArtifactValue.createNormalFile(new byte[] {1, 2, 3}, 10);
+ ActionInputMap map = new ActionInputMap(1);
+ map.put(input, metadata);
+ assertThat(map.getMetadata(input)).isEqualTo(metadata);
+ ActionMetadataHandler handler = new ActionMetadataHandler(
+ map,
+ /* missingArtifactsAllowed= */ false,
+ /* outputs= */ ImmutableList.of(),
+ /* tsgm= */ null,
+ ArtifactPathResolver.IDENTITY,
+ new MinimalOutputStore());
+ assertThat(handler.getMetadata(input)).isNull();
+ }
+
+ @Test
+ public void withArtifactInput() throws Exception {
+ PathFragment path = PathFragment.create("src/a");
+ Artifact artifact = new Artifact(path, sourceRoot);
+ FileArtifactValue metadata = FileArtifactValue.createNormalFile(new byte[] {1, 2, 3}, 10);
+ ActionInputMap map = new ActionInputMap(1);
+ map.put(artifact, metadata);
+ ActionMetadataHandler handler = new ActionMetadataHandler(
+ map,
+ /* missingArtifactsAllowed= */ false,
+ /* outputs= */ ImmutableList.of(),
+ /* tsgm= */ null,
+ ArtifactPathResolver.IDENTITY,
+ new MinimalOutputStore());
+ assertThat(handler.getMetadata(artifact)).isEqualTo(metadata);
+ }
+
+ @Test
+ public void withUnknownSourceArtifactAndNoMissingArtifactsAllowed() throws Exception {
+ PathFragment path = PathFragment.create("src/a");
+ Artifact artifact = new Artifact(path, sourceRoot);
+ ActionInputMap map = new ActionInputMap(1);
+ ActionMetadataHandler handler = new ActionMetadataHandler(
+ map,
+ /* missingArtifactsAllowed= */ false,
+ /* outputs= */ ImmutableList.of(),
+ /* tsgm= */ null,
+ ArtifactPathResolver.IDENTITY,
+ new MinimalOutputStore());
+ try {
+ handler.getMetadata(artifact);
+ fail();
+ } catch (IllegalStateException expected) {
+ assertThat(expected).hasMessageThat().contains("null for ");
+ }
+ }
+
+ @Test
+ public void withUnknownSourceArtifact() throws Exception {
+ PathFragment path = PathFragment.create("src/a");
+ Artifact artifact = new Artifact(path, sourceRoot);
+ ActionInputMap map = new ActionInputMap(1);
+ ActionMetadataHandler handler = new ActionMetadataHandler(
+ map,
+ /* missingArtifactsAllowed= */ true,
+ /* outputs= */ ImmutableList.of(),
+ /* tsgm= */ null,
+ ArtifactPathResolver.IDENTITY,
+ new MinimalOutputStore());
+ assertThat(handler.getMetadata(artifact)).isNull();
+ }
+}
