Remove OutputStore#injectedFiles.
This set was previously used to track which files were injected with the goal of skipping chmod calls on injected files. After recent changes, there is no longer any code path that attempts to call setPathReadOnlyAndExecutable on an artifact that was already injected in a prior call.
This change also removes duplicate chmod calls for local tree artifact files and adds tests counting chmod calls. Because setPathReadOnlyAndExecutable always stats the path to check if it's a file, this also reduces the number of stats on tree artifact files.
Fun fact: OutputStore is now down to just 2 maps - at one point it was up to 5 data stores (4 maps & 1 set).
PiperOrigin-RevId: 321060966
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 2eef29f..59d7945 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
@@ -237,7 +237,7 @@
//
// We only cache nonexistence here, not file system errors. It is unlikely that the file will be
// requested from this cache too many times.
- value = constructFileArtifactValueFromFilesystem(artifact);
+ value = constructFileArtifactValueFromFilesystem(artifact, /*chmod=*/ executionMode.get());
store.putArtifactData(artifact, value);
return checkExists(value, artifact);
}
@@ -255,23 +255,13 @@
}
private TreeArtifactValue getTreeArtifactValue(SpecialArtifact artifact) throws IOException {
+ Preconditions.checkState(artifact.isTreeArtifact(), "%s is not a tree artifact", artifact);
+
TreeArtifactValue value = store.getTreeArtifactData(artifact);
if (value != null) {
return checkExists(value, artifact);
}
- if (executionMode.get()) {
- // Preserve existing behavior: we don't set non-TreeArtifact directories
- // read only and executable. However, it's unusual for non-TreeArtifact outputs
- // to be directories.
- if (artifactPathResolver.toPath(artifact).isDirectory()) {
- setTreeReadOnlyAndExecutable(artifact, PathFragment.EMPTY_FRAGMENT);
- } else {
- setPathReadOnlyAndExecutable(
- TreeFileArtifact.createTreeOutput(artifact, PathFragment.EMPTY_FRAGMENT));
- }
- }
-
value = constructTreeArtifactValueFromFilesystem(artifact);
store.putTreeArtifactData(artifact, value);
return checkExists(value, artifact);
@@ -279,7 +269,14 @@
private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifact parent)
throws IOException {
- Preconditions.checkState(parent.isTreeArtifact(), parent);
+ if (executionMode.get()) {
+ if (artifactPathResolver.toPath(parent).isDirectory()) {
+ setTreeReadOnlyAndExecutable(parent, PathFragment.EMPTY_FRAGMENT);
+ } else {
+ setPathReadOnlyAndExecutable(
+ TreeFileArtifact.createTreeOutput(parent, PathFragment.EMPTY_FRAGMENT));
+ }
+ }
// Make sure the tree artifact root is a regular directory. Note that this is how the Action
// is initialized, so this should hold unless the Action itself has deleted the root.
@@ -295,7 +292,8 @@
TreeFileArtifact treeFileArtifact = TreeFileArtifact.createTreeOutput(parent, path);
FileArtifactValue fileMetadata;
try {
- fileMetadata = constructFileArtifactValueFromFilesystem(treeFileArtifact);
+ // We have already called chmod if necessary in the setTreeReadOnlyAndExecutable call above.
+ fileMetadata = constructFileArtifactValueFromFilesystem(treeFileArtifact, /*chmod=*/ false);
} catch (FileNotFoundException e) {
String errorMessage =
String.format(
@@ -321,16 +319,15 @@
@Override
public FileArtifactValue constructMetadataForDigest(
Artifact output, FileStatus statNoFollow, byte[] digest) throws IOException {
- Preconditions.checkState(executionMode.get());
- Preconditions.checkState(!output.isSymlink());
- Preconditions.checkNotNull(digest);
+ Preconditions.checkArgument(!output.isSymlink(), "%s is a symlink", output);
+ Preconditions.checkNotNull(digest, "Missing digest for %s", output);
+ Preconditions.checkNotNull(statNoFollow, "Missing stat for %s", output);
+ Preconditions.checkState(
+ executionMode.get(), "Tried to construct metadata for %s outside of execution", output);
- // We have to add the artifact to injectedFiles before calling constructFileArtifactValue to
- // avoid duplicate chmod calls.
- store.injectedFiles().add(output);
-
+ // We already have a stat, so no need to call chmod.
return constructFileArtifactValue(
- output, FileStatusWithDigestAdapter.adapt(statNoFollow), digest);
+ output, /*chmod=*/ false, FileStatusWithDigestAdapter.adapt(statNoFollow), digest);
}
@Override
@@ -343,6 +340,7 @@
output);
Preconditions.checkState(
executionMode.get(), "Tried to inject %s outside of execution", output);
+
store.injectOutputData(output, metadata);
}
@@ -355,6 +353,7 @@
output.isTreeArtifact(), "Output must be a tree artifact: %s", output);
Preconditions.checkState(
executionMode.get(), "Tried to inject %s outside of execution", output);
+
store.putTreeArtifactData(output, TreeArtifactValue.create(children));
}
@@ -385,14 +384,6 @@
public void discardOutputMetadata() {
boolean wasExecutionMode = executionMode.getAndSet(true);
Preconditions.checkState(!wasExecutionMode);
- Preconditions.checkState(
- store.injectedFiles().isEmpty(),
- "Files cannot be injected before action execution: %s",
- store.injectedFiles());
- Preconditions.checkState(
- omittedOutputs.isEmpty(),
- "Artifacts cannot be marked omitted before action execution: %s",
- omittedOutputs);
store.clear();
}
@@ -411,28 +402,27 @@
}
/**
- * Constructs a new {@link FileArtifactValue} by reading from the file system and checks
- * inconsistent data. This calls chmod on the file if we're in execution mode, unless it is in
- * {@link OutputStore#injectedFiles()}.
+ * Constructs a new {@link FileArtifactValue} by reading from the file system, optionally calling
+ * chmod on the file.
*/
- private FileArtifactValue constructFileArtifactValueFromFilesystem(Artifact artifact)
- throws IOException {
- return constructFileArtifactValue(artifact, /*statNoFollow=*/ null, /*injectedDigest=*/ null);
+ private FileArtifactValue constructFileArtifactValueFromFilesystem(
+ Artifact artifact, boolean chmod) throws IOException {
+ return constructFileArtifactValue(
+ artifact, chmod, /*statNoFollow=*/ null, /*injectedDigest=*/ null);
}
- /**
- * Constructs a new {@link FileArtifactValue} and checks inconsistent data. This calls chmod on
- * the file if we're in execution mode, unless it is in {@link OutputStore#injectedFiles()}.
- */
+ /** Constructs a new {@link FileArtifactValue}, optionally calling chmod on the file. */
private FileArtifactValue constructFileArtifactValue(
Artifact artifact,
+ boolean chmod,
@Nullable FileStatusWithDigest statNoFollow,
@Nullable byte[] injectedDigest)
throws IOException {
- // We first chmod the output files before we construct the FileContentsProxy. The proxy may use
- // ctime, which is affected by chmod.
- if (executionMode.get()) {
- Preconditions.checkState(!artifact.isTreeArtifact());
+ Preconditions.checkState(!artifact.isTreeArtifact(), "%s is a tree artifact", artifact);
+
+ // If necessary, we first chmod the output file before we construct the FileContentsProxy. The
+ // proxy may use ctime, which is affected by chmod.
+ if (chmod) {
setPathReadOnlyAndExecutable(artifact);
}
@@ -594,11 +584,6 @@
}
private void setPathReadOnlyAndExecutable(Artifact artifact) throws IOException {
- // If the metadata was injected, we assume the mode is set correct and bail out early to avoid
- // the additional overhead of resetting it.
- if (store.injectedFiles().contains(artifact)) {
- return;
- }
Path path = artifactPathResolver.toPath(artifact);
if (path.isFile(Symlinks.NOFOLLOW)) { // i.e. regular files only.
// We trust the files created by the execution engine to be non symlinks with expected
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
index 5bf4570..2c6698e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
@@ -15,12 +15,10 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import javax.annotation.Nullable;
@@ -42,8 +40,6 @@
private final ConcurrentMap<SpecialArtifact, TreeArtifactValue> treeArtifactData =
new ConcurrentHashMap<>();
- private final Set<Artifact> injectedFiles = Sets.newConcurrentHashSet();
-
@Nullable
final FileArtifactValue getArtifactData(Artifact artifact) {
return artifactData.get(artifact);
@@ -79,21 +75,15 @@
return ImmutableMap.copyOf(treeArtifactData);
}
+ // TODO(b/160603797): This is the same as putArtifactData. Merge and remove MinimalOutputStore.
final void injectOutputData(Artifact output, FileArtifactValue artifactValue) {
- injectedFiles.add(output);
artifactData.put(output, artifactValue);
}
- /** Returns a set that tracks which Artifacts have had metadata injected. */
- final Set<Artifact> injectedFiles() {
- return injectedFiles;
- }
-
/** Clears all data in this store. */
final void clear() {
artifactData.clear();
treeArtifactData.clear();
- injectedFiles.clear();
}
/**
@@ -108,6 +98,5 @@
} else {
artifactData.remove(artifact);
}
- injectedFiles.remove(artifact);
}
}
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
index 28ff947..9fbec71 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
@@ -15,10 +15,12 @@
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.ActionInputMap;
@@ -36,10 +38,14 @@
import com.google.devtools.build.lib.testutil.ManualClock;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
+import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import java.io.FileNotFoundException;
+import java.io.IOException;
import java.util.Map;
+import java.util.Set;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -49,7 +55,21 @@
@RunWith(JUnit4.class)
public final class ActionMetadataHandlerTest {
- private final Scratch scratch = new Scratch();
+ private final Set<Path> chmodCalls = Sets.newConcurrentHashSet();
+
+ private final Scratch scratch =
+ new Scratch(
+ new InMemoryFileSystem() {
+ @Override
+ public void chmod(Path path, int mode) throws IOException {
+ assertThat(mode).isEqualTo(0555); // Read only and executable.
+ if (!chmodCalls.add(path)) {
+ fail("chmod called on " + path + " twice");
+ }
+ super.chmod(path, mode);
+ }
+ });
+
private final TimestampGranularityMonitor tsgm =
new TimestampGranularityMonitor(new ManualClock());
@@ -84,6 +104,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(input)).isNull();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -106,6 +127,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isEqualTo(metadata);
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -125,6 +147,7 @@
outputRoot.getRoot().asPath());
Exception e = assertThrows(IllegalStateException.class, () -> handler.getMetadata(artifact));
assertThat(e).hasMessageThat().contains(artifact + " is not present in declared outputs");
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -143,6 +166,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNull();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -161,6 +185,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNull();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -180,6 +205,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNotNull();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -198,6 +224,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThrows(FileNotFoundException.class, () -> handler.getMetadata(artifact));
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -216,6 +243,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThrows(IllegalStateException.class, () -> handler.getMetadata(artifact));
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -236,6 +264,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNull();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -258,6 +287,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNotNull();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -278,6 +308,7 @@
new MinimalOutputStore(),
outputRoot.getRoot().asPath());
assertThrows(IllegalStateException.class, () -> handler.getMetadata(artifact));
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -313,13 +344,14 @@
assertThat(tree.getChildValues())
.containsExactly(child1, child1Metadata, child2, child2Metadata);
assertThat(store.getAllArtifactData()).isEmpty(); // All data should be in treeArtifactData.
+ assertThat(chmodCalls).isEmpty();
}
@Test
public void resettingOutputs() throws Exception {
- scratch.file("/output/bin/foo/bar", "not empty");
PathFragment path = PathFragment.create("foo/bar");
Artifact artifact = ActionsTestUtil.createArtifactWithRootRelativePath(outputRoot, path);
+ Path outputPath = scratch.file(artifact.getPath().getPathString(), "not empty");
ActionInputMap map = new ActionInputMap(1);
ActionMetadataHandler handler =
new ActionMetadataHandler(
@@ -335,6 +367,7 @@
// The handler doesn't have any info. It'll stat the file and discover that it's 10 bytes long.
assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(10);
+ assertThat(chmodCalls).containsExactly(outputPath);
// Inject a remote file of size 42.
handler.injectFile(
@@ -343,7 +376,9 @@
// Reset this output, which will make the handler stat the file again.
handler.resetOutputs(ImmutableList.of(artifact));
+ chmodCalls.clear(); // Permit a second chmod call for the artifact.
assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(10);
+ assertThat(chmodCalls).containsExactly(outputPath);
}
@Test
@@ -371,6 +406,7 @@
assertThat(v).isNotNull();
assertThat(v.getDigest()).isEqualTo(digest);
assertThat(v.getSize()).isEqualTo(size);
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -398,6 +434,7 @@
assertThrows(IllegalArgumentException.class, () -> handler.injectFile(child, childValue));
assertThat(store.getAllArtifactData()).isEmpty();
assertThat(store.getAllTreeArtifactData()).isEmpty();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -427,6 +464,7 @@
assertThat(store.getAllArtifactData()).containsExactly(output, value);
assertThat(store.getAllTreeArtifactData()).isEmpty();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -463,6 +501,7 @@
TreeArtifactValue treeValue = store.getTreeArtifactData(treeArtifact);
assertThat(treeValue).isNotNull();
assertThat(treeValue.getDigest()).isEqualTo(value.getDigest());
+ assertThat(chmodCalls).isEmpty();
assertThat(treeValue.getChildPaths())
.containsExactly(PathFragment.create("foo"), PathFragment.create("bar"));
@@ -510,6 +549,7 @@
assertThat(handler.getMetadata(createInput("file"))).isEqualTo(regularFav);
assertThat(handler.getMetadata(createInput("bytes"))).isNull();
assertThat(handler.getMetadata(createInput("does_not_exist"))).isNull();
+ assertThat(chmodCalls).isEmpty();
}
private FilesetOutputSymlink createFilesetOutputSymlink(HasDigest digest, String identifier) {
@@ -553,6 +593,7 @@
assertThat(store.getAllArtifactData())
.containsExactly(omitted, FileArtifactValue.OMITTED_FILE_MARKER);
assertThat(store.getAllTreeArtifactData()).isEmpty();
+ assertThat(chmodCalls).isEmpty();
}
@Test
@@ -584,5 +625,69 @@
assertThat(store.getAllTreeArtifactData())
.containsExactly(omittedTree, TreeArtifactValue.OMITTED_TREE_MARKER);
assertThat(store.getAllArtifactData()).isEmpty();
+ assertThat(chmodCalls).isEmpty();
+ }
+
+ @Test
+ public void outputArtifactNotPreviouslyInjectedInExecutionMode() throws Exception {
+ Artifact output =
+ ActionsTestUtil.createArtifactWithRootRelativePath(
+ outputRoot, PathFragment.create("dir/file.out"));
+ Path outputPath = scratch.file(output.getPath().getPathString(), "contents");
+ OutputStore store = new OutputStore();
+ ActionMetadataHandler handler =
+ new ActionMetadataHandler(
+ new ActionInputMap(1),
+ /*expandedFilesets=*/ ImmutableMap.of(),
+ /*missingArtifactsAllowed=*/ false,
+ ImmutableSet.of(output),
+ tsgm,
+ ArtifactPathResolver.IDENTITY,
+ store,
+ outputRoot.getRoot().asPath());
+ handler.discardOutputMetadata();
+
+ FileArtifactValue metadata = handler.getMetadata(output);
+
+ assertThat(metadata.getDigest()).isEqualTo(outputPath.getDigest());
+ assertThat(store.getAllArtifactData()).containsExactly(output, metadata);
+ assertThat(store.getAllTreeArtifactData()).isEmpty();
+ assertThat(chmodCalls).containsExactly(outputPath);
+ }
+
+ @Test
+ public void outputTreeArtifactNotPreviouslyInjectedInExecutionMode() throws Exception {
+ SpecialArtifact treeArtifact =
+ ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+ outputRoot, PathFragment.create("bin/foo/bar"));
+ TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(treeArtifact, "child1");
+ TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(treeArtifact, "subdir/child2");
+ Path child1Path = scratch.file(child1.getPath().getPathString(), "contents1");
+ Path child2Path = scratch.file(child2.getPath().getPathString(), "contents2");
+ OutputStore store = new OutputStore();
+ ActionMetadataHandler handler =
+ new ActionMetadataHandler(
+ new ActionInputMap(1),
+ /*expandedFilesets=*/ ImmutableMap.of(),
+ /*missingArtifactsAllowed=*/ false,
+ /*outputs=*/ ImmutableSet.of(treeArtifact),
+ tsgm,
+ ArtifactPathResolver.IDENTITY,
+ store,
+ outputRoot.getRoot().asPath());
+ handler.discardOutputMetadata();
+
+ FileArtifactValue treeMetadata = handler.getMetadata(treeArtifact);
+ FileArtifactValue child1Metadata = handler.getMetadata(child1);
+ FileArtifactValue child2Metadata = handler.getMetadata(child2);
+ TreeArtifactValue tree = store.getTreeArtifactData(treeArtifact);
+
+ assertThat(tree.getMetadata()).isEqualTo(treeMetadata);
+ assertThat(tree.getChildValues())
+ .containsExactly(child1, child1Metadata, child2, child2Metadata);
+ assertThat(store.getAllArtifactData()).isEmpty(); // All data should be in treeArtifactData.
+ assertThat(chmodCalls)
+ .containsExactly(
+ treeArtifact.getPath(), child1Path, child2Path, child2Path.getParentDirectory());
}
}
