diff options
Diffstat (limited to 'src')
7 files changed, 74 insertions, 96 deletions
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 62d03fd372..fc61397318 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.ActionInput; 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 @@ import java.io.IOException; * <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/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 611e3f2059..aaa9221098 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 @@ -385,9 +385,13 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver actionLookupData, /* inputDiscoveryRan= */ false); } - // This may be recreated if we discover inputs. - ActionMetadataHandler metadataHandler = new ActionMetadataHandler(state.inputArtifactData, - action.getOutputs(), tsgm.get(), pathResolver(state.actionFileSystem)); + // The metadata handler is recreated below if the action performs input discovery. + ActionMetadataHandler metadataHandler = new ActionMetadataHandler( + state.inputArtifactData, + /* missingArtifactsAllowed= */ action.discoversInputs(), + action.getOutputs(), + tsgm.get(), + pathResolver(state.actionFileSystem)); 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()) { @@ -422,19 +426,15 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver // Delete the metadataHandler's cache of the action's outputs, since they are being deleted. metadataHandler.discardOutputMetadata(); - // 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()); if (action.discoversInputs()) { if (state.discoveredInputs == null) { try { state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler); state.discoveredInputs = skyframeActionExecutor.discoverInputs( - action, perActionFileCache, metadataHandler, env, state.actionFileSystem); + action, metadataHandler, env, state.actionFileSystem); Preconditions.checkState(state.discoveredInputs != null, "discoverInputs() returned null on action %s", action); } catch (MissingDepException e) { @@ -447,8 +447,6 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver if (env.valuesMissing()) { return null; } - perActionFileCache = - new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false); // Stage 1 finished, let's do stage 2. The stage 1 of input discovery will have added some // files with addDiscoveredInputs() and then have waited for those files to be available @@ -467,11 +465,13 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver if (env.valuesMissing()) { return null; } - perActionFileCache = - new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false); } metadataHandler = - new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get(), + new ActionMetadataHandler( + state.inputArtifactData, + /* missingArtifactsAllowed= */ false, + action.getOutputs(), + tsgm.get(), pathResolver(state.actionFileSystem)); // Set the MetadataHandler to accept output information. metadataHandler.discardOutputMetadata(); @@ -503,7 +503,6 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler); try (ActionExecutionContext actionExecutionContext = skyframeActionExecutor.getContext( - perActionFileCache, metadataHandler, Collections.unmodifiableMap(state.expandedArtifacts), filesetMappings.build(), @@ -545,7 +544,11 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver // markOmitted is only called for remote execution, and this code only gets executed for // local execution. metadataHandler = - new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get(), + new ActionMetadataHandler( + state.inputArtifactData, + /*missingArtifactsAllowed=*/ false, + action.getOutputs(), + tsgm.get(), pathResolver(state.actionFileSystem)); } } 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 1d4012c60e..49159f401b 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 @@ -83,6 +83,7 @@ public class ActionMetadataHandler implements MetadataHandler { * <p>This should never be read directly. Use {@link #getInputFileArtifactValue} instead. */ private final ActionInputMap inputArtifactData; + private final boolean missingArtifactsAllowed; /** FileValues for each output Artifact. */ private final ConcurrentMap<Artifact, FileValue> outputArtifactData = @@ -135,10 +136,12 @@ public class ActionMetadataHandler implements MetadataHandler { @VisibleForTesting public ActionMetadataHandler( ActionInputMap inputArtifactData, + boolean missingArtifactsAllowed, Iterable<Artifact> outputs, TimestampGranularityMonitor tsgm, ArtifactPathResolver artifactPathResolver) { this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData); + this.missingArtifactsAllowed = missingArtifactsAllowed; this.outputs = ImmutableSet.copyOf(outputs); this.tsgm = tsgm; this.artifactPathResolver = artifactPathResolver; @@ -180,15 +183,25 @@ public class ActionMetadataHandler implements MetadataHandler { } @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 @@ -210,7 +223,11 @@ public class ActionMetadataHandler implements MetadataHandler { // 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). FileValue fileValue = outputArtifactData.get(artifact); if (fileValue != null) { // Non-middleman artifacts should only have additionalOutputData if they have @@ -227,12 +244,28 @@ public class ActionMetadataHandler implements MetadataHandler { } return FileArtifactValue.createNormalFile(fileValue); } - // 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. fileValue = constructFileValue(artifact, /*statNoFollow=*/ null); return maybeStoreAdditionalData(artifact, fileValue, null); } + @Override + public ActionInput getInput(String execPath) { + return inputArtifactData.getInput(execPath); + } + /** * Check that the new {@code data} we just calculated for an {@link Artifact} agrees with the * {@code oldData} (presumably calculated concurrently), if it was present. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java deleted file mode 100644 index f27bd369a9..0000000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright 2014 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.base.Preconditions; -import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.ActionInputMap; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.FileArtifactValue; -import com.google.devtools.build.lib.actions.MetadataProvider; -import javax.annotation.Nullable; - -/** - * Cache provided by an {@link ActionExecutionFunction}, allowing Blaze to obtain artifact metadata - * from the graph. - * - * <p>Data for the action's inputs is injected into this cache on construction, using the graph as - * the source of truth. - */ -class PerActionFileCache implements MetadataProvider { - private final ActionInputMap inputArtifactData; - private final boolean missingArtifactsAllowed; - - /** - * @param inputArtifactData Map from artifact to metadata, used to return metadata upon request. - * @param missingArtifactsAllowed whether to tolerate missing artifacts: can happen during input - * discovery. - */ - PerActionFileCache(ActionInputMap inputArtifactData, boolean missingArtifactsAllowed) { - this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData); - this.missingArtifactsAllowed = missingArtifactsAllowed; - } - - @Nullable - @Override - public FileArtifactValue getMetadata(ActionInput input) { - // TODO(shahan): is this bypass needed? - if (!(input instanceof Artifact)) { - return null; - } - FileArtifactValue result = inputArtifactData.getMetadata(input); - Preconditions.checkState(missingArtifactsAllowed || result != null, "null for %s", input); - return result; - } - - @Override - public ActionInput getInput(String execPath) { - return inputArtifactData.getInput(execPath); - } -} 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 a42fc65807..8cca0c45d4 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 @@ -520,7 +520,6 @@ public final class SkyframeActionExecutor { * tasks related to that action. */ public ActionExecutionContext getContext( - MetadataProvider graphFileCache, MetadataHandler metadataHandler, Map<Artifact, Collection<Artifact>> expandedInputs, ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings, @@ -529,7 +528,7 @@ public final class SkyframeActionExecutor { ArtifactPathResolver.createPathResolver(actionFileSystem, executorEngine.getExecRoot())); return new ActionExecutionContext( executorEngine, - createFileCache(graphFileCache, actionFileSystem), + createFileCache(metadataHandler, actionFileSystem), actionInputPrefetcher, actionKeyContext, metadataHandler, @@ -649,7 +648,6 @@ public final class SkyframeActionExecutor { */ Iterable<Artifact> discoverInputs( Action action, - PerActionFileCache graphFileCache, MetadataHandler metadataHandler, Environment env, @Nullable FileSystem actionFileSystem) @@ -657,7 +655,7 @@ public final class SkyframeActionExecutor { ActionExecutionContext actionExecutionContext = ActionExecutionContext.forInputDiscovery( executorEngine, - createFileCache(graphFileCache, actionFileSystem), + createFileCache(metadataHandler, 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 a84efb34ca..817cf38c6b 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,12 @@ public class ActionCacheCheckerTest { /** 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 FileArtifactValue.create((Artifact) input); + } else { + return null; + } } @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 013f57e162..cd3489ddf5 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.AbstractSkyFunctionEnvironment; 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; @@ -709,7 +710,12 @@ public final class ActionsTestUtil { */ 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(); } |