Implement a MinimalOutputStore that caches less data.
There may be room for further reduction of storage, however it would require more invasive changes to consumers and potentially ActionFS.
RELNOTES: None.
PiperOrigin-RevId: 214833898
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
index 416b2a6..deaaf79 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
@@ -111,6 +111,11 @@
return 0;
}
+ /** Returns {@code true} if this is a special marker as opposed to a representing a real file. */
+ public final boolean isMarkerValue() {
+ return this instanceof Singleton;
+ }
+
/**
* Provides a best-effort determination whether the file was changed since the digest was
* computed. This method performs file system I/O, so may be expensive. It's primarily intended to
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 ebc574b..e21d8db 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
@@ -67,6 +67,7 @@
import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
import javax.annotation.Nullable;
/**
@@ -379,9 +380,18 @@
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 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();
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()) {
@@ -452,9 +462,7 @@
perActionFileCache =
new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false);
- metadataHandler =
- new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get(),
- pathResolver(state.actionFileSystem));
+ metadataHandler = metadataHandlerSupplier.get();
// Set the MetadataHandler to accept output information.
metadataHandler.discardOutputMetadata();
}
@@ -534,9 +542,7 @@
// 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 =
- new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get(),
- pathResolver(state.actionFileSystem));
+ metadataHandler = metadataHandlerSupplier.get();
}
}
Preconditions.checkState(!env.valuesMissing(), action);
@@ -545,11 +551,6 @@
return state.value;
}
- private ArtifactPathResolver pathResolver(@Nullable FileSystem actionFileSystem) {
- return ArtifactPathResolver.createPathResolver(
- actionFileSystem, skyframeActionExecutor.getExecRoot());
- }
-
private static final Function<Artifact, SkyKey> TO_NONMANDATORY_SKYKEY =
new Function<Artifact, SkyKey>() {
@Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index 163dcf1..e62aaff 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -49,11 +49,10 @@
*
* <p>Concerning the data in this class:
*
- * <p>We want to track all output data from an ActionExecutionValue. See {@link
- * ActionMetadataHandler} for a discussion of how its fields {@link
- * ActionMetadataHandler#outputArtifactData} and {@link ActionMetadataHandler#additionalOutputData}
- * relate. The relationship is the same between {@link #artifactData} and {@link
- * #additionalOutputData} here.
+ * <p>We want to track all output data from an ActionExecutionValue. See {@link OutputStore} for a
+ * discussion of how its fields {@link OutputStore#artifactData} and {@link
+ * OutputStore#additionalOutputData} relate. The relationship is the same between {@link
+ * #artifactData} and {@link #additionalOutputData} here.
*/
@Immutable
@ThreadSafe
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 c41c893..79bde1c 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
@@ -99,19 +99,20 @@
*/
private final AtomicBoolean executionMode = new AtomicBoolean(false);
- // TODO(b/115361150): Swap in a different output store when using ActionFS.
- private final OutputStore store = new OutputStore();
+ private final OutputStore store;
@VisibleForTesting
public ActionMetadataHandler(
ActionInputMap inputArtifactData,
Iterable<Artifact> outputs,
TimestampGranularityMonitor tsgm,
- ArtifactPathResolver artifactPathResolver) {
+ ArtifactPathResolver artifactPathResolver,
+ OutputStore store) {
this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData);
this.outputs = ImmutableSet.copyOf(outputs);
this.tsgm = tsgm;
this.artifactPathResolver = artifactPathResolver;
+ this.store = store;
}
/**
@@ -322,10 +323,8 @@
}
// A minor hack: maybeStoreAdditionalData will force the data to be stored via
- // store.putAdditionalOutputData.
- maybeStoreAdditionalData(treeFileArtifact, fileMetadata, null);
- cachedValue = Preconditions.checkNotNull(
- store.getAdditionalOutputData(treeFileArtifact), treeFileArtifact);
+ // store.putAdditionalOutputData, if the underlying OutputStore supports it.
+ cachedValue = maybeStoreAdditionalData(treeFileArtifact, fileMetadata, null);
}
values.put(treeFileArtifact, cachedValue);
@@ -363,7 +362,8 @@
@Override
public Iterable<TreeFileArtifact> getExpandedOutputs(Artifact artifact) {
- return ImmutableSet.copyOf(store.getOrCreateTreeArtifactContents(artifact));
+ Set<TreeFileArtifact> contents = store.getTreeArtifactContents(artifact);
+ return contents != null ? ImmutableSet.copyOf(contents) : ImmutableSet.of();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
new file mode 100644
index 0000000..714bc24
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
@@ -0,0 +1,55 @@
+// 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 com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
+import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
+
+/**
+ * Implementation of {@link OutputStore} that retains a minimal amount of output data necessary to
+ * complete the build.
+ *
+ * <p>This store is intended for use with in-memory file systems, where aggressive caching would not
+ * be worthwhile.
+ */
+// TODO(b/115361150): See if even less data can be retained. Specifically, we shouldn't need to
+// call injectRemoteFile for anything, including children of tree artifacts.
+final class MinimalOutputStore extends OutputStore {
+
+ @Override
+ void putAdditionalOutputData(Artifact artifact, FileArtifactValue value) {
+ if (value.isMarkerValue()) {
+ super.putAdditionalOutputData(artifact, value);
+ }
+ }
+
+ @Override
+ void putArtifactData(Artifact artifact, ArtifactFileMetadata value) {}
+
+ @Override
+ void addTreeArtifactContents(Artifact artifact, TreeFileArtifact contents) {}
+
+ @Override
+ void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) {
+ if (isChildOfTreeArtifact(output)) {
+ super.injectRemoteFile(output, digest, size, locationIndex);
+ }
+ }
+
+ private static boolean isChildOfTreeArtifact(Artifact artifact) {
+ return artifact.hasParent() && artifact.getParent().isTreeArtifact();
+ }
+}
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 95c34df..fcff7ae 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
@@ -146,17 +146,9 @@
return treeArtifactContents.get(artifact);
}
- /**
- * Same as {@link #getTreeArtifactContents} except that a new set of contents is created and
- * stored instead of returning {@code null}.
- */
- Set<TreeFileArtifact> getOrCreateTreeArtifactContents(Artifact artifact) {
- Preconditions.checkArgument(artifact.isTreeArtifact(), artifact);
- return treeArtifactContents.computeIfAbsent(artifact, unused -> Sets.newConcurrentHashSet());
- }
-
void addTreeArtifactContents(Artifact artifact, TreeFileArtifact contents) {
- getOrCreateTreeArtifactContents(artifact).add(contents);
+ Preconditions.checkArgument(artifact.isTreeArtifact(), artifact);
+ treeArtifactContents.computeIfAbsent(artifact, a -> Sets.newConcurrentHashSet()).add(contents);
}
void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
index 1e85b6c..fbad99e 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
@@ -229,4 +229,17 @@
clock.advanceMillis(1);
assertThat(value.wasModifiedSinceDigest(path)).isTrue();
}
+
+ @Test
+ public void testIsMarkerValue_marker() {
+ assertThat(FileArtifactValue.DEFAULT_MIDDLEMAN.isMarkerValue()).isTrue();
+ assertThat(FileArtifactValue.MISSING_FILE_MARKER.isMarkerValue()).isTrue();
+ assertThat(FileArtifactValue.OMITTED_FILE_MARKER.isMarkerValue()).isTrue();
+ }
+
+ @Test
+ public void testIsMarkerValue_notMarker() throws Exception {
+ FileArtifactValue value = create(scratchFile("/dir/artifact1", 0L, "content"));
+ assertThat(value.isMarkerValue()).isFalse();
+ }
}
