Remove unnecessary MinimalOutputStore class.
This is currently used as the store for an action filesystem, but it does not actually store any less data because injectOutputData is used to add data to the store, bypassing the overridden putArtifactData method. Instead, just use OutputStore for all use cases, and delete injectOutputData.
The original thought behind MinimalOutputStore was that we could reduce temporary caching when the action filesystem is in-memory anyway, but now OutputStore is down to just two maps required to construct the final ActionExecutionValue and there are no longer any temporary caches.
RELNOTES: None.
PiperOrigin-RevId: 321394749
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 777e468..01662a2 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
@@ -120,11 +120,6 @@
return "";
}
- /** Returns {@code true} if this is a special marker as opposed to a representing a real file. */
- public boolean isMarkerValue() {
- return this instanceof Singleton;
- }
-
/** Returns {@code true} if the file only exists remotely. */
public boolean isRemote() {
return false;
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 9099ae2..e71cb39 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
@@ -753,7 +753,7 @@
action.getOutputs(),
tsgm.get(),
pathResolver,
- newOutputStore(state),
+ new OutputStore(),
skyframeActionExecutor.getExecRoot());
// We only need to check the action cache if we haven't done it on a previous run.
if (!state.hasCheckedActionCache()) {
@@ -830,7 +830,7 @@
action.getOutputs(),
tsgm.get(),
pathResolver,
- newOutputStore(state),
+ new OutputStore(),
skyframeActionExecutor.getExecRoot());
// Set the MetadataHandler to accept output information.
metadataHandler.discardOutputMetadata();
@@ -862,18 +862,6 @@
state.discoveredInputs != null);
}
- private OutputStore newOutputStore(ContinuationState state) {
- Preconditions.checkState(
- !skyframeActionExecutor.actionFileSystemType().isEnabled()
- || state.actionFileSystem != null,
- "actionFileSystem must not be null");
-
- if (skyframeActionExecutor.actionFileSystemType().inMemoryFileSystem()) {
- return new MinimalOutputStore();
- }
- return new OutputStore();
- }
-
/** Implementation of {@link ActionPostprocessing}. */
private final class ActionPostprocessingImpl implements ActionPostprocessing {
private final ContinuationState state;
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 2fb580b..f4e98a4 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
@@ -345,7 +345,7 @@
Preconditions.checkState(
executionMode.get(), "Tried to inject %s outside of execution", output);
- store.injectOutputData(output, metadata);
+ store.putArtifactData(output, metadata);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index d2c0021..88fd2fa 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -164,7 +164,6 @@
":local_repository_lookup_value",
":managed_directories_knowledge",
":map_as_package_roots",
- ":minimal_output_store",
":output_store",
":package_error_function",
":package_error_message_function",
@@ -1624,15 +1623,6 @@
)
java_library(
- name = "minimal_output_store",
- srcs = ["MinimalOutputStore.java"],
- deps = [
- ":output_store",
- "//src/main/java/com/google/devtools/build/lib/actions",
- ],
-)
-
-java_library(
name = "mutable_supplier",
srcs = ["MutableSupplier.java"],
deps = ["//third_party:guava"],
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
deleted file mode 100644
index 1d9fcdc..0000000
--- a/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
+++ /dev/null
@@ -1,33 +0,0 @@
-// 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.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.
- */
-final class MinimalOutputStore extends OutputStore {
- @Override
- void putArtifactData(Artifact artifact, FileArtifactValue value) {
- if (value.isMarkerValue()) {
- super.putArtifactData(artifact, value);
- }
- }
-}
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 2c6698e..9e7588e 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
@@ -28,12 +28,9 @@
*
* <p>Data stored in {@link #artifactData} and {@link #treeArtifactData} will be passed along to the
* final {@link ActionExecutionValue}.
- *
- * <p>This implementation aggressively stores all data. Subclasses may override mutating methods to
- * avoid storing unnecessary data.
*/
@ThreadSafe
-class OutputStore {
+final class OutputStore {
private final ConcurrentMap<Artifact, FileArtifactValue> artifactData = new ConcurrentHashMap<>();
@@ -41,7 +38,7 @@
new ConcurrentHashMap<>();
@Nullable
- final FileArtifactValue getArtifactData(Artifact artifact) {
+ FileArtifactValue getArtifactData(Artifact artifact) {
return artifactData.get(artifact);
}
@@ -53,16 +50,16 @@
artifactData.put(artifact, value);
}
- final ImmutableMap<Artifact, FileArtifactValue> getAllArtifactData() {
+ ImmutableMap<Artifact, FileArtifactValue> getAllArtifactData() {
return ImmutableMap.copyOf(artifactData);
}
@Nullable
- final TreeArtifactValue getTreeArtifactData(Artifact artifact) {
+ TreeArtifactValue getTreeArtifactData(Artifact artifact) {
return treeArtifactData.get(artifact);
}
- final void putTreeArtifactData(SpecialArtifact treeArtifact, TreeArtifactValue value) {
+ void putTreeArtifactData(SpecialArtifact treeArtifact, TreeArtifactValue value) {
Preconditions.checkArgument(treeArtifact.isTreeArtifact(), "%s is not a tree artifact");
treeArtifactData.put(treeArtifact, value);
}
@@ -71,17 +68,12 @@
* Returns data for TreeArtifacts that was computed during execution. May contain copies of {@link
* TreeArtifactValue#MISSING_TREE_ARTIFACT}.
*/
- final ImmutableMap<Artifact, TreeArtifactValue> getAllTreeArtifactData() {
+ ImmutableMap<Artifact, TreeArtifactValue> getAllTreeArtifactData() {
return ImmutableMap.copyOf(treeArtifactData);
}
- // TODO(b/160603797): This is the same as putArtifactData. Merge and remove MinimalOutputStore.
- final void injectOutputData(Artifact output, FileArtifactValue artifactValue) {
- artifactData.put(output, artifactValue);
- }
-
/** Clears all data in this store. */
- final void clear() {
+ void clear() {
artifactData.clear();
treeArtifactData.clear();
}
@@ -92,7 +84,7 @@
* <p>If a tree artifact parent is given, it will be cleared from {@link #treeArtifactData}. If a
* tree artifact child is given, its enclosing tree artifact will not be removed.
*/
- final void remove(Artifact artifact) {
+ void remove(Artifact artifact) {
if (artifact.isTreeArtifact()) {
treeArtifactData.remove(artifact);
} else {
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
index 66afcb6..a8dfb27 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
@@ -203,11 +203,6 @@
}
@Override
- public boolean isMarkerValue() {
- throw new UnsupportedOperationException();
- }
-
- @Override
public FileContentsProxy getContentsProxy() {
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
index 9fbec71..72c595b 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
@@ -101,7 +101,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(input)).isNull();
assertThat(chmodCalls).isEmpty();
@@ -124,7 +124,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isEqualTo(metadata);
assertThat(chmodCalls).isEmpty();
@@ -143,7 +143,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
Exception e = assertThrows(IllegalStateException.class, () -> handler.getMetadata(artifact));
assertThat(e).hasMessageThat().contains(artifact + " is not present in declared outputs");
@@ -163,7 +163,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNull();
assertThat(chmodCalls).isEmpty();
@@ -182,7 +182,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNull();
assertThat(chmodCalls).isEmpty();
@@ -202,7 +202,7 @@
/*outputs=*/ ImmutableSet.of(artifact),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNotNull();
assertThat(chmodCalls).isEmpty();
@@ -221,7 +221,7 @@
/*outputs=*/ ImmutableSet.of(artifact),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThrows(FileNotFoundException.class, () -> handler.getMetadata(artifact));
assertThat(chmodCalls).isEmpty();
@@ -240,7 +240,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThrows(IllegalStateException.class, () -> handler.getMetadata(artifact));
assertThat(chmodCalls).isEmpty();
@@ -261,7 +261,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNull();
assertThat(chmodCalls).isEmpty();
@@ -284,7 +284,7 @@
/*outputs=*/ ImmutableSet.of(treeArtifact),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThat(handler.getMetadata(artifact)).isNotNull();
assertThat(chmodCalls).isEmpty();
@@ -305,7 +305,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
assertThrows(IllegalStateException.class, () -> handler.getMetadata(artifact));
assertThat(chmodCalls).isEmpty();
@@ -361,7 +361,7 @@
/*outputs=*/ ImmutableSet.of(artifact),
tsgm,
ArtifactPathResolver.IDENTITY,
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
handler.discardOutputMetadata();
@@ -541,7 +541,7 @@
/*outputs=*/ ImmutableSet.of(),
tsgm,
ArtifactPathResolver.forExecRoot(outputRoot.getRoot().asPath()),
- new MinimalOutputStore(),
+ new OutputStore(),
outputRoot.getRoot().asPath());
// Only the regular FileArtifactValue should have its metadata stored.
@@ -567,7 +567,7 @@
@Test
public void omitRegularArtifact() {
- OutputStore store = new MinimalOutputStore();
+ OutputStore store = new OutputStore();
Artifact omitted =
ActionsTestUtil.createArtifactWithRootRelativePath(
outputRoot, PathFragment.create("omitted"));
@@ -598,7 +598,7 @@
@Test
public void omitTreeArtifact() {
- OutputStore store = new MinimalOutputStore();
+ OutputStore store = new OutputStore();
SpecialArtifact omittedTree =
ActionsTestUtil.createTreeArtifactWithGeneratingAction(
outputRoot, PathFragment.create("omitted"));
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
index 80f22a8..8cc35fc 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -199,7 +199,6 @@
"//src/main/java/com/google/devtools/build/lib/skyframe:glob_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:local_repository_lookup_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:managed_directories_knowledge",
- "//src/main/java/com/google/devtools/build/lib/skyframe:minimal_output_store",
"//src/main/java/com/google/devtools/build/lib/skyframe:output_store",
"//src/main/java/com/google/devtools/build/lib/skyframe:package_error_message_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function",
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 6b8ad49..6a22471 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
@@ -243,17 +243,4 @@
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 = createForTesting(scratchFile("/dir/artifact1", 0L, "content"));
- assertThat(value.isMarkerValue()).isFalse();
- }
}
