Require explicit ExecKind in SpawnMetrics.Builder and remove SpawnMetrics.EMPTY
Previously we used `REMOTE` as the default but now we'll require setting
`ExecKind` explicitly. To make it less error prone, I've introduced a few
factory methods that set it. Also, as previously discussed, I've introduced
a new `ExecKind.OTHER` (IMHO sounds a bit better than the initial proposal
of `INTERNAL`, but I don't feel strongly, so let me know if you prefer
the latter).
We also discussed not setting `ExecKind` in `SpawnMetrics.EMPTY` (and using
something like `null`), but I found it a bit tricky to really define what
that would actually mean, now that we have `ExecKind` (e.g., should we allow
aggregating such "empty" values? if yes, should it be commutative? etc.)
But I've noticed that after the introduction of `AggregatedSpawnMetrics`,
we only use `EMPTY` in one place and it's possible to simply remove it, which
avoids the problem.
RELNOTES: None.
PiperOrigin-RevId: 314687611
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AggregatedSpawnMetrics.java b/src/main/java/com/google/devtools/build/lib/actions/AggregatedSpawnMetrics.java
index 792d2b2..4d4ba33 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AggregatedSpawnMetrics.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AggregatedSpawnMetrics.java
@@ -43,7 +43,7 @@
private static final ImmutableMap<SpawnMetrics.ExecKind, SpawnMetrics> createEmptyMetrics() {
EnumMap<SpawnMetrics.ExecKind, SpawnMetrics> map = new EnumMap<>(SpawnMetrics.ExecKind.class);
for (SpawnMetrics.ExecKind kind : SpawnMetrics.ExecKind.values()) {
- map.put(kind, new SpawnMetrics.Builder().setExecKind(kind).build());
+ map.put(kind, SpawnMetrics.Builder.forExec(kind).build());
}
return Maps.immutableEnumMap(map);
}
@@ -93,8 +93,7 @@
public AggregatedSpawnMetrics sumAllMetrics(SpawnMetrics other) {
SpawnMetrics existing = getMetrics(other.execKind());
SpawnMetrics.Builder builder =
- new SpawnMetrics.Builder()
- .setExecKind(other.execKind())
+ SpawnMetrics.Builder.forExec(other.execKind())
.addDurations(existing)
.addDurations(other)
.addNonDurations(existing)
@@ -113,8 +112,7 @@
public AggregatedSpawnMetrics sumDurationsMaxOther(SpawnMetrics other) {
SpawnMetrics existing = getMetrics(other.execKind());
SpawnMetrics.Builder builder =
- new SpawnMetrics.Builder()
- .setExecKind(other.execKind())
+ SpawnMetrics.Builder.forExec(other.execKind())
.addDurations(existing)
.addDurations(other)
.maxNonDurations(existing)
@@ -205,8 +203,7 @@
}
private SpawnMetrics.Builder getBuilder(SpawnMetrics.ExecKind kind) {
- return builderMap.computeIfAbsent(
- kind, (SpawnMetrics.ExecKind k) -> new SpawnMetrics.Builder().setExecKind(k));
+ return builderMap.computeIfAbsent(kind, SpawnMetrics.Builder::forExec);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnMetrics.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnMetrics.java
index 703f091..f6cf871 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SpawnMetrics.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnMetrics.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.actions;
import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
@@ -25,7 +26,12 @@
public static enum ExecKind {
REMOTE("Remote"),
LOCAL("Local"),
- WORKER("Worker");
+ WORKER("Worker"),
+ /**
+ * Other kinds of execution (or when it's not clear whether something happened locally or
+ * remotely).
+ */
+ OTHER("Other");
private final String name;
@@ -42,16 +48,8 @@
/** Any non important stats < than 10% will not be shown in the summary. */
private static final double STATS_SHOW_THRESHOLD = 0.10;
- /** Represents a zero cost/null statistic. */
- public static final SpawnMetrics EMPTY_REMOTE =
- new Builder().setExecKind(ExecKind.REMOTE).build();
-
public static SpawnMetrics forLocalExecution(Duration wallTime) {
- return new Builder()
- .setExecKind(ExecKind.LOCAL)
- .setTotalTime(wallTime)
- .setExecutionWallTime(wallTime)
- .build();
+ return Builder.forLocalExec().setTotalTime(wallTime).setExecutionWallTime(wallTime).build();
}
private final ExecKind execKind;
@@ -69,36 +67,6 @@
private final long inputFiles;
private final long memoryEstimateBytes;
- public SpawnMetrics(
- Duration totalTime,
- Duration parseTime,
- Duration networkTime,
- Duration fetchTime,
- Duration queueTime,
- Duration setupTime,
- Duration uploadTime,
- Duration executionWallTime,
- Duration retryTime,
- Duration processOutputsTime,
- long inputBytes,
- long inputFiles,
- long memoryEstimateBytes) {
- this.execKind = ExecKind.REMOTE;
- this.totalTime = totalTime;
- this.parseTime = parseTime;
- this.networkTime = networkTime;
- this.fetchTime = fetchTime;
- this.queueTime = queueTime;
- this.setupTime = setupTime;
- this.uploadTime = uploadTime;
- this.executionWallTime = executionWallTime;
- this.retryTime = retryTime;
- this.processOutputsTime = processOutputsTime;
- this.inputBytes = inputBytes;
- this.inputFiles = inputFiles;
- this.memoryEstimateBytes = memoryEstimateBytes;
- }
-
private SpawnMetrics(Builder builder) {
this.execKind = builder.execKind;
this.totalTime = builder.totalTime;
@@ -278,7 +246,7 @@
/** Builder class for SpawnMetrics. */
public static class Builder {
- private ExecKind execKind = ExecKind.REMOTE;
+ private ExecKind execKind = null;
private Duration totalTime = Duration.ZERO;
private Duration parseTime = Duration.ZERO;
private Duration networkTime = Duration.ZERO;
@@ -293,7 +261,32 @@
private long inputFiles = 0;
private long memoryEstimateBytes = 0;
+ public static Builder forLocalExec() {
+ return forExec(ExecKind.LOCAL);
+ }
+
+ public static Builder forRemoteExec() {
+ return forExec(ExecKind.REMOTE);
+ }
+
+ public static Builder forWorkerExec() {
+ return forExec(ExecKind.WORKER);
+ }
+
+ public static Builder forOtherExec() {
+ return forExec(ExecKind.OTHER);
+ }
+
+ public static Builder forExec(ExecKind kind) {
+ return new Builder().setExecKind(kind);
+ }
+
+ // Make the constructor private to force users to set the ExecKind by using one of the factory
+ // methods.
+ private Builder() {}
+
public SpawnMetrics build() {
+ Preconditions.checkNotNull(execKind, "ExecKind must be explicitly set using `setExecKind`");
// TODO(ulfjack): Add consistency checks here?
return new SpawnMetrics(this);
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
index b33338a..d0e5ad3 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -126,7 +126,7 @@
MerkleTree merkleTree =
MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
SpawnMetrics.Builder spawnMetrics =
- new SpawnMetrics.Builder()
+ SpawnMetrics.Builder.forRemoteExec()
.setInputBytes(merkleTree.getInputBytes())
.setInputFiles(merkleTree.getInputFiles());
Digest merkleTreeRoot = merkleTree.getRootDigest();
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index 2b295d1..7966e08 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -213,7 +213,7 @@
final MerkleTree merkleTree =
MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
SpawnMetrics.Builder spawnMetrics =
- new SpawnMetrics.Builder()
+ SpawnMetrics.Builder.forRemoteExec()
.setInputBytes(merkleTree.getInputBytes())
.setInputFiles(merkleTree.getInputFiles());
maybeWriteParamFilesLocally(spawn);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComponent.java b/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComponent.java
index 080bbde..b5f599c 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComponent.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComponent.java
@@ -66,7 +66,7 @@
private final Artifact primaryOutput;
/** Spawn metrics for this action. */
- private SpawnMetrics phaseMaxMetrics = SpawnMetrics.EMPTY_REMOTE;
+ @Nullable private SpawnMetrics phaseMaxMetrics = null;
private AggregatedSpawnMetrics totalSpawnMetrics = AggregatedSpawnMetrics.EMPTY;
private Duration longestRunningTotalDuration = Duration.ZERO;
@@ -124,9 +124,9 @@
}
// If the phaseMaxMetrics has Duration, then we want to aggregate it to the total.
- if (!this.phaseMaxMetrics.totalTime().isZero()) {
+ if (this.phaseMaxMetrics != null && !this.phaseMaxMetrics.totalTime().isZero()) {
this.totalSpawnMetrics = this.totalSpawnMetrics.sumDurationsMaxOther(phaseMaxMetrics);
- this.phaseMaxMetrics = SpawnMetrics.EMPTY_REMOTE;
+ this.phaseMaxMetrics = null;
}
}
@@ -197,10 +197,13 @@
this.remote = true;
}
if (this.phaseChange) {
- this.totalSpawnMetrics = this.totalSpawnMetrics.sumDurationsMaxOther(phaseMaxMetrics);
+ if (this.phaseMaxMetrics != null) {
+ this.totalSpawnMetrics = this.totalSpawnMetrics.sumDurationsMaxOther(phaseMaxMetrics);
+ }
this.phaseMaxMetrics = metrics;
this.phaseChange = false;
- } else if (metrics.totalTime().compareTo(this.phaseMaxMetrics.totalTime()) > 0) {
+ } else if (this.phaseMaxMetrics == null
+ || metrics.totalTime().compareTo(this.phaseMaxMetrics.totalTime()) > 0) {
this.phaseMaxMetrics = metrics;
}
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 50f1c35..ae7bcbd 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
@@ -889,7 +889,7 @@
env.getListener()
.post(
new DiscoveredInputsEvent(
- new SpawnMetrics.Builder()
+ SpawnMetrics.Builder.forOtherExec()
.setParseTime(discoveredInputsDuration)
.setTotalTime(discoveredInputsDuration)
.build(),
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
index f227f14..f4f8631 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
@@ -173,9 +173,7 @@
SortedMap<PathFragment, HashCode> workerFiles =
WorkerFilesHash.getWorkerFilesWithHashes(
- spawn,
- context.getArtifactExpander(),
- context.getMetadataProvider());
+ spawn, context.getArtifactExpander(), context.getMetadataProvider());
HashCode workerFilesCombinedHash = WorkerFilesHash.getCombinedHash(workerFiles);
@@ -215,8 +213,7 @@
exitCode == 0 ? SpawnResult.Status.SUCCESS : SpawnResult.Status.NON_ZERO_EXIT)
.setWallTime(wallTime)
.setSpawnMetrics(
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.WORKER)
+ SpawnMetrics.Builder.forWorkerExec()
.setTotalTime(wallTime)
.setExecutionWallTime(wallTime)
.build())
diff --git a/src/test/java/com/google/devtools/build/lib/actions/AggregatedSpawnMetricsTest.java b/src/test/java/com/google/devtools/build/lib/actions/AggregatedSpawnMetricsTest.java
index 34ef58c..5505605 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/AggregatedSpawnMetricsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/AggregatedSpawnMetricsTest.java
@@ -27,8 +27,7 @@
@Test
public void sumAllMetrics() throws Exception {
SpawnMetrics metrics1 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(1))
.setExecutionWallTime(Duration.ofSeconds(2))
.setInputBytes(10)
@@ -36,8 +35,7 @@
.setMemoryEstimateBytes(30)
.build();
SpawnMetrics metrics2 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(10))
.setExecutionWallTime(Duration.ofSeconds(20))
.setInputBytes(100)
@@ -59,8 +57,7 @@
@Test
public void sumDurationMetricsMaxOther() throws Exception {
SpawnMetrics metrics1 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(1))
.setExecutionWallTime(Duration.ofSeconds(2))
.setInputBytes(10)
@@ -68,8 +65,7 @@
.setMemoryEstimateBytes(30)
.build();
SpawnMetrics metrics2 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(10))
.setExecutionWallTime(Duration.ofSeconds(20))
.setInputBytes(100)
@@ -91,20 +87,11 @@
@Test
public void aggregatingMetrics_preservesExecKind() throws Exception {
SpawnMetrics metrics1 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.LOCAL)
- .setTotalTime(Duration.ofSeconds(1))
- .build();
+ SpawnMetrics.Builder.forLocalExec().setTotalTime(Duration.ofSeconds(1)).build();
SpawnMetrics metrics2 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
- .setTotalTime(Duration.ofSeconds(2))
- .build();
+ SpawnMetrics.Builder.forRemoteExec().setTotalTime(Duration.ofSeconds(2)).build();
SpawnMetrics metrics3 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.WORKER)
- .setTotalTime(Duration.ofSeconds(3))
- .build();
+ SpawnMetrics.Builder.forWorkerExec().setTotalTime(Duration.ofSeconds(3)).build();
AggregatedSpawnMetrics aggregated = AggregatedSpawnMetrics.EMPTY;
aggregated = aggregated.sumAllMetrics(metrics1);
@@ -122,21 +109,18 @@
@Test
public void toString_printsOnlyRemote() throws Exception {
SpawnMetrics metrics1 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.LOCAL)
+ SpawnMetrics.Builder.forLocalExec()
.setTotalTime(Duration.ofSeconds(1))
.setExecutionWallTime(Duration.ofSeconds(1))
.build();
SpawnMetrics metrics2 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(2))
.setNetworkTime(Duration.ofSeconds(1))
.setExecutionWallTime(Duration.ofSeconds(1))
.build();
SpawnMetrics metrics3 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.WORKER)
+ SpawnMetrics.Builder.forWorkerExec()
.setTotalTime(Duration.ofSeconds(3))
.setQueueTime(Duration.ofSeconds(1))
.setExecutionWallTime(Duration.ofSeconds(2))
diff --git a/src/test/java/com/google/devtools/build/lib/actions/SpawnMetricsTest.java b/src/test/java/com/google/devtools/build/lib/actions/SpawnMetricsTest.java
index 0f3c1ae..75dee98 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/SpawnMetricsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/SpawnMetricsTest.java
@@ -27,8 +27,7 @@
@Test
public void builder_addDurationsNonDurations() throws Exception {
SpawnMetrics metrics1 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(1))
.setExecutionWallTime(Duration.ofSeconds(2))
.setInputBytes(10)
@@ -36,8 +35,7 @@
.setMemoryEstimateBytes(30)
.build();
SpawnMetrics metrics2 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(10))
.setExecutionWallTime(Duration.ofSeconds(20))
.setInputBytes(100)
@@ -46,8 +44,7 @@
.build();
SpawnMetrics result =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.addDurations(metrics1)
.addDurations(metrics2)
.addNonDurations(metrics1)
@@ -64,8 +61,7 @@
@Test
public void builder_addDurationsMaxNonDurations() throws Exception {
SpawnMetrics metrics1 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(1))
.setExecutionWallTime(Duration.ofSeconds(2))
.setInputBytes(10)
@@ -73,8 +69,7 @@
.setMemoryEstimateBytes(30)
.build();
SpawnMetrics metrics2 =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.setTotalTime(Duration.ofSeconds(10))
.setExecutionWallTime(Duration.ofSeconds(20))
.setInputBytes(100)
@@ -83,8 +78,7 @@
.build();
SpawnMetrics result =
- new SpawnMetrics.Builder()
- .setExecKind(SpawnMetrics.ExecKind.REMOTE)
+ SpawnMetrics.Builder.forRemoteExec()
.addDurations(metrics1)
.addDurations(metrics2)
.maxNonDurations(metrics1)
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
index ad1f45f..aac8abd 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -975,7 +975,7 @@
@Test
public void accountingAddsDurationsForStages() {
SpawnMetrics.Builder builder =
- new SpawnMetrics.Builder()
+ SpawnMetrics.Builder.forRemoteExec()
.setQueueTime(Duration.ofSeconds(1))
.setSetupTime(Duration.ofSeconds(2))
.setExecutionWallTime(Duration.ofSeconds(2))
@@ -1026,10 +1026,7 @@
}
private RemoteSpawnRunner newSpawnRunner() {
- return newSpawnRunner(
- executor,
- /* reporter= */ null,
- /* topLevelOutputs= */ ImmutableSet.of());
+ return newSpawnRunner(executor, /* reporter= */ null, /* topLevelOutputs= */ ImmutableSet.of());
}
private RemoteSpawnRunner newSpawnRunner(Reporter reporter) {
