Consolidate key-caching logic in an abstract class.
This way all actions and action templates can share the same key-caching code.
RELNOTES: None.
PiperOrigin-RevId: 244866405
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index 71250f6..379962f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -36,7 +36,6 @@
import com.google.devtools.build.lib.syntax.SkylarkDict;
import com.google.devtools.build.lib.syntax.SkylarkList;
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
-import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.Symlinks;
@@ -53,7 +52,7 @@
*/
@Immutable
@ThreadSafe
-public abstract class AbstractAction implements Action, ActionApi {
+public abstract class AbstractAction extends ActionKeyCacher implements Action, ActionApi {
/**
* An arbitrary default resource set. We assume that a typical subprocess is single-threaded
* (i.e., uses one CPU core) and CPU-bound, and uses a small-ish amount of memory. In the past,
@@ -62,8 +61,7 @@
* amounts of memory), we suggest to use this default set.
*/
// TODO(ulfjack): Collect actual data to confirm that this is an acceptable approximation.
- public static final ResourceSet DEFAULT_RESOURCE_SET =
- ResourceSet.createWithRamCpu(250, 1);
+ public static final ResourceSet DEFAULT_RESOURCE_SET = ResourceSet.createWithRamCpu(250, 1);
/**
* The owner/inputs/outputs attributes below should never be directly accessed even within
@@ -103,8 +101,6 @@
private final RunfilesSupplier runfilesSupplier;
@VisibleForSerialization protected final ImmutableSet<Artifact> outputs;
- private String cachedKey;
-
/**
* Construct an abstract action with the specified inputs and outputs;
*/
@@ -280,37 +276,6 @@
@Override
public abstract String getMnemonic();
- /**
- * See the javadoc for {@link com.google.devtools.build.lib.actions.Action} and {@link
- * ActionExecutionMetadata#getKey(ActionKeyContext)} for the contract for {@link
- * #computeKey(ActionKeyContext, Fingerprint)}.
- */
- protected abstract void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp)
- throws CommandLineExpansionException;
-
- @Override
- public final synchronized String getKey(ActionKeyContext actionKeyContext) {
- if (cachedKey == null) {
- try {
- Fingerprint fp = new Fingerprint();
- computeKey(actionKeyContext, fp);
-
- // Add a bool indicating whether the execution platform was set.
- fp.addBoolean(getExecutionPlatform() != null);
- if (getExecutionPlatform() != null) {
- // Add the execution platform information.
- getExecutionPlatform().addTo(fp);
- }
-
- // Compute the actual key and store it.
- cachedKey = fp.hexDigestAndReset();
- } catch (CommandLineExpansionException e) {
- cachedKey = KEY_ERROR;
- }
- }
- return cachedKey;
- }
-
@Override
public String describeKey() {
return null;
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java
index e076912..736d229 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java
@@ -14,12 +14,25 @@
package com.google.devtools.build.lib.actions;
import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
+import javax.annotation.Nullable;
/**
* An Analysis phase interface for an {@link Action} or Action-like object, containing only
* side-effect-free query methods for information needed during action analysis.
*/
public interface ActionAnalysisMetadata {
+
+ /**
+ * Return this key from {@link #getKey} to signify a failed key computation.
+ *
+ * <p>Actions that return this value should fail to execute.
+ *
+ * <p>Consumers must either gracefully handle multiple failed actions having the same key,
+ * (recommended), or check against this value explicitly.
+ */
+ String KEY_ERROR = "1ea50e01-0349-4552-80cf-76cf520e8592";
+
/**
* Returns the owner of this executable if this executable can supply verbose information. This is
* typically the rule that constructed it; see ActionOwner class comment for details. Returns
@@ -228,4 +241,11 @@
default boolean hasLooseHeaders() {
return false;
}
+
+ /**
+ * Returns the {@link PlatformInfo} platform this action should be executed on. If the execution
+ * platform is {@code null}, then the host platform is assumed.
+ */
+ @Nullable
+ PlatformInfo getExecutionPlatform();
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java
index 255074f..0b2b6af 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.actions;
-import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import javax.annotation.Nullable;
@@ -27,16 +26,6 @@
public interface ActionExecutionMetadata extends ActionAnalysisMetadata {
/**
- * Return this key to signify a failed key computation.
- *
- * <p>Actions that return this value should fail to execute.
- *
- * <p>Consumers must either gracefully handle multiple failed actions having the same key,
- * (recommended), or check against this value explicitly.
- */
- String KEY_ERROR = "1ea50e01-0349-4552-80cf-76cf520e8592";
-
- /**
* If this executable can supply verbose information, returns a string that can be used as a
* progress message while this executable is running. A return value of {@code null} indicates no
* message should be reported.
@@ -115,11 +104,4 @@
default boolean mayInsensitivelyPropagateInputs() {
return false;
}
-
- /**
- * Returns the {@link PlatformInfo} platform this action should be executed on. If the execution
- * platform is {@code null}, then the host platform is assumed.
- */
- @Nullable
- PlatformInfo getExecutionPlatform();
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java b/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java
new file mode 100644
index 0000000..e325f06
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java
@@ -0,0 +1,56 @@
+// Copyright 2019 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.actions;
+
+import com.google.devtools.build.lib.util.Fingerprint;
+import javax.annotation.Nullable;
+
+/**
+ * An implementation of {@link ActionAnalysisMetadata} that caches its {@linkplain #getKey key} so
+ * that it is only computed once.
+ */
+public abstract class ActionKeyCacher implements ActionAnalysisMetadata {
+
+ @Nullable private String cachedKey = null;
+
+ @Override
+ public final synchronized String getKey(ActionKeyContext actionKeyContext) {
+ if (cachedKey == null) {
+ try {
+ Fingerprint fp = new Fingerprint();
+ computeKey(actionKeyContext, fp);
+
+ // Add a bool indicating whether the execution platform was set.
+ fp.addBoolean(getExecutionPlatform() != null);
+ if (getExecutionPlatform() != null) {
+ // Add the execution platform information.
+ getExecutionPlatform().addTo(fp);
+ }
+
+ // Compute the actual key and store it.
+ cachedKey = fp.hexDigestAndReset();
+ } catch (CommandLineExpansionException e) {
+ cachedKey = KEY_ERROR;
+ }
+ }
+ return cachedKey;
+ }
+
+ /**
+ * See the javadoc for {@link Action} and {@link ActionAnalysisMetadata#getKey} for the contract
+ * of this method.
+ */
+ protected abstract void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp)
+ throws CommandLineExpansionException;
+}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionTemplate.java b/src/main/java/com/google/devtools/build/lib/actions/ActionTemplate.java
index 2a68628..335b5d4 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionTemplate.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionTemplate.java
@@ -14,6 +14,8 @@
package com.google.devtools.build.lib.actions;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
+import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
+import javax.annotation.Nullable;
/**
* A placeholder action that, at execution time, expands into a list of {@link Action}s to be
@@ -84,4 +86,10 @@
/** Returns the output TreeArtifact. */
Artifact getOutputTreeArtifact();
+
+ @Override
+ @Nullable
+ default PlatformInfo getExecutionPlatform() {
+ return null;
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
index c3716e1..4ae6ca2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.ActionKeyCacher;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.ActionTemplate;
@@ -27,16 +28,17 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.CommandLine;
+import com.google.devtools.build.lib.actions.CommandLineExpansionException;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Map;
-/**
- * An {@link ActionTemplate} that expands into {@link SpawnAction}s at execution time.
- */
-public final class SpawnActionTemplate implements ActionTemplate<SpawnAction> {
+/** An {@link ActionTemplate} that expands into {@link SpawnAction}s at execution time. */
+public final class SpawnActionTemplate extends ActionKeyCacher
+ implements ActionTemplate<SpawnAction> {
private final SpecialArtifact inputTreeArtifact;
private final SpecialArtifact outputTreeArtifact;
private final NestedSet<Artifact> commonInputs;
@@ -47,7 +49,6 @@
private final OutputPathMapper outputPathMapper;
private final SpawnAction.Builder spawnActionBuilder;
private final CustomCommandLine commandLineTemplate;
- private String cachedKey;
/**
* Interface providing mapping between expanded input files under the input TreeArtifact and
@@ -115,18 +116,15 @@
}
@Override
- public synchronized String getKey(ActionKeyContext actionKeyContext) {
- if (cachedKey != null) {
- return cachedKey;
- }
+ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp)
+ throws CommandLineExpansionException {
TreeFileArtifact inputTreeFileArtifact =
ActionInputHelper.treeFileArtifact(inputTreeArtifact, "dummy_for_key");
TreeFileArtifact outputTreeFileArtifact =
ActionInputHelper.treeFileArtifact(
outputTreeArtifact, outputPathMapper.parentRelativeOutputPath(inputTreeFileArtifact));
SpawnAction dummyAction = createAction(inputTreeFileArtifact, outputTreeFileArtifact);
- cachedKey = dummyAction.getKey(actionKeyContext);
- return cachedKey;
+ dummyAction.computeKey(actionKeyContext, fp);
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java
index 3ce146e..353f70a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java
@@ -20,6 +20,7 @@
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.ActionKeyCacher;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.ActionTemplate;
@@ -37,10 +38,9 @@
import java.util.ArrayList;
import java.util.List;
-/**
- * An {@link ActionTemplate} that expands into {@link CppCompileAction}s at execution time.
- */
-public final class CppCompileActionTemplate implements ActionTemplate<CppCompileAction> {
+/** An {@link ActionTemplate} that expands into {@link CppCompileAction}s at execution time. */
+public final class CppCompileActionTemplate extends ActionKeyCacher
+ implements ActionTemplate<CppCompileAction> {
private final CppCompileActionBuilder cppCompileActionBuilder;
private final Artifact sourceTreeArtifact;
private final Artifact outputTreeArtifact;
@@ -50,7 +50,6 @@
private final ActionOwner actionOwner;
private final NestedSet<Artifact> mandatoryInputs;
private final NestedSet<Artifact> allInputs;
- private String cachedKey;
/**
* Creates an CppCompileActionTemplate.
@@ -143,10 +142,8 @@
}
@Override
- public synchronized String getKey(ActionKeyContext actionKeyContext) {
- if (cachedKey != null) {
- return cachedKey;
- }
+ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp)
+ throws CommandLineExpansionException {
CompileCommandLine commandLine =
CppCompileAction.buildCommandLine(
sourceTreeArtifact,
@@ -155,28 +152,21 @@
dotdTreeArtifact,
cppCompileActionBuilder.getFeatureConfiguration(),
cppCompileActionBuilder.getVariables());
- Fingerprint fp = new Fingerprint();
- try {
- CppCompileAction.computeKey(
- actionKeyContext,
- fp,
- cppCompileActionBuilder.getActionClassId(),
- cppCompileActionBuilder.getActionEnvironment(),
- commandLine.getEnvironment(),
- cppCompileActionBuilder.getExecutionInfo(),
- CppCompileAction.computeCommandLineKey(
- commandLine.getCompilerOptions(/*overwrittenVariables=*/ null)),
- cppCompileActionBuilder.getCcCompilationContext().getDeclaredIncludeSrcs(),
- cppCompileActionBuilder.buildMandatoryInputs(),
- cppCompileActionBuilder.buildPrunableHeaders(),
- cppCompileActionBuilder.getCcCompilationContext().getDeclaredIncludeDirs(),
- cppCompileActionBuilder.getBuiltinIncludeDirectories(),
- cppCompileActionBuilder.buildInputsForInvalidation());
- cachedKey = fp.hexDigestAndReset();
- } catch (CommandLineExpansionException e) {
- cachedKey = CppCompileAction.KEY_ERROR;
- }
- return cachedKey;
+ CppCompileAction.computeKey(
+ actionKeyContext,
+ fp,
+ cppCompileActionBuilder.getActionClassId(),
+ cppCompileActionBuilder.getActionEnvironment(),
+ commandLine.getEnvironment(),
+ cppCompileActionBuilder.getExecutionInfo(),
+ CppCompileAction.computeCommandLineKey(
+ commandLine.getCompilerOptions(/*overwrittenVariables=*/ null)),
+ cppCompileActionBuilder.getCcCompilationContext().getDeclaredIncludeSrcs(),
+ cppCompileActionBuilder.buildMandatoryInputs(),
+ cppCompileActionBuilder.buildPrunableHeaders(),
+ cppCompileActionBuilder.getCcCompilationContext().getDeclaredIncludeDirs(),
+ cppCompileActionBuilder.getBuiltinIncludeDirectories(),
+ cppCompileActionBuilder.buildInputsForInvalidation());
}
private boolean shouldCompileHeaders() {
