Clean up CriticalPath things since action is not nullable
CriticalPathComponent.action was once nullable, but now it's not. This
cleans up the critical path component and computer classes' code that
handled the no-longer-possible null case.
RELNOTES: None.
PiperOrigin-RevId: 224190088
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 62b7f64..0769e41 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
@@ -55,14 +55,12 @@
// These two fields are values of BlazeClock.nanoTime() at the relevant points in time.
private long startNanos;
private long finishNanos = 0;
- protected volatile boolean isRunning = true;
+ private volatile boolean isRunning = true;
/** We keep here the critical path time for the most expensive child. */
private long childAggregatedElapsedTime = 0;
- /** May be nulled out after finished running to allow the action to be GC'ed. */
- @Nullable protected Action action;
-
+ private final Action action;
private final Artifact primaryOutput;
/** Spawn metrics for this action. */
@@ -70,11 +68,8 @@
/** An unique identifier of the component for one build execution */
private final int id;
- /**
- * Child with the maximum critical path.
- */
- @Nullable
- private CriticalPathComponent child;
+ /** Child with the maximum critical path. */
+ @Nullable private CriticalPathComponent child;
public CriticalPathComponent(int id, Action action, long startNanos) {
this.id = id;
@@ -107,11 +102,8 @@
return possiblePrimaryOutput == primaryOutput;
}
- /**
- * The action for which we are storing the stat. May be null if the action has finished running.
- */
- @Nullable
- public final Action maybeGetAction() {
+ /** The action for which we are storing the stat. */
+ public final Action getAction() {
return action;
}
@@ -120,12 +112,12 @@
}
public String prettyPrintAction() {
- return getActionNotNull().prettyPrint();
+ return action.prettyPrint();
}
@Nullable
public Label getOwner() {
- ActionOwner owner = getActionNotNull().getOwner();
+ ActionOwner owner = action.getOwner();
if (owner != null && owner.getLabel() != null) {
return owner.getLabel();
}
@@ -133,11 +125,7 @@
}
public String getMnemonic() {
- return getActionNotNull().getMnemonic();
- }
-
- private Action getActionNotNull() {
- return Preconditions.checkNotNull(action, this);
+ return action.getMnemonic();
}
/** An unique identifier of the component for one build execution */
@@ -151,7 +139,7 @@
* are run in parallel we should keep the maximum), we keep the maximum. This is better than just
* keeping the latest one.
*/
- public void addSpawnMetrics(SpawnMetrics spawnMetrics) {
+ void addSpawnMetrics(SpawnMetrics spawnMetrics) {
if (spawnMetrics.totalTime().compareTo(this.spawnMetrics.totalTime()) > 0) {
this.spawnMetrics = spawnMetrics;
}
@@ -193,7 +181,7 @@
}
/** To be used only in debugging: skips state invariance checks to avoid crash-looping. */
- protected Duration getElapsedTimeNoCheck() {
+ private Duration getElapsedTimeNoCheck() {
return Duration.ofNanos(getElapsedTimeNanosNoCheck());
}
@@ -206,11 +194,11 @@
*
* <p>Critical path is defined as : action_execution_time + max(child_critical_path).
*/
- public Duration getAggregatedElapsedTime() {
+ Duration getAggregatedElapsedTime() {
return Duration.ofNanos(getAggregatedElapsedTimeNanos());
}
- long getAggregatedElapsedTimeNanos() {
+ private long getAggregatedElapsedTimeNanos() {
Preconditions.checkState(!isRunning, "Still running %s", this);
return getElapsedTimeNanos() + childAggregatedElapsedTime;
}
@@ -226,13 +214,11 @@
}
/** Returns a string representation of the action. Only for use in crash messages and the like. */
- protected String getActionString() {
- return (action == null ? "(null action)" : action.prettyPrint());
+ private String getActionString() {
+ return action.prettyPrint();
}
- /**
- * Returns a user readable representation of the critical path stats with all the details.
- */
+ /** Returns a user readable representation of the critical path stats with all the details. */
@Override
public String toString() {
StringBuilder sb = new StringBuilder();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java b/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java
index dda61da..3a15dde 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CriticalPathComputer.java
@@ -240,44 +240,24 @@
* @return The component to be used for updating the time stats.
*/
private CriticalPathComponent tryAddComponent(CriticalPathComponent newComponent) {
- Action newAction = Preconditions.checkNotNull(newComponent.maybeGetAction(), newComponent);
+ Action newAction = newComponent.getAction();
Artifact primaryOutput = newAction.getPrimaryOutput();
CriticalPathComponent storedComponent =
outputArtifactToComponent.putIfAbsent(primaryOutput, newComponent);
if (storedComponent != null) {
- Action oldAction = storedComponent.maybeGetAction();
- if (oldAction != null) {
- if (!Actions.canBeShared(actionKeyContext, newAction, oldAction)) {
- throw new IllegalStateException(
- "Duplicate output artifact found for unsharable actions."
- + "This can happen if a previous event registered the action.\n"
- + "Old action: "
- + oldAction
- + "\n\nNew action: "
- + newAction
- + "\n\nArtifact: "
- + primaryOutput
- + "\n");
- }
- } else {
- String mnemonic = storedComponent.getMnemonic();
- String prettyPrint = storedComponent.prettyPrintAction();
- if (!newAction.getMnemonic().equals(mnemonic)
- || !newAction.prettyPrint().equals(prettyPrint)) {
- throw new IllegalStateException(
- "Duplicate output artifact found for unsharable actions."
- + "This can happen if a previous event registered the action.\n"
- + "Old action mnemonic and prettyPrint: "
- + mnemonic
- + ", "
- + prettyPrint
- + "\n\nNew action: "
- + newAction
- + "\n\nArtifact: "
- + primaryOutput
- + "\n");
- }
+ Action oldAction = storedComponent.getAction();
+ if (!Actions.canBeShared(actionKeyContext, newAction, oldAction)) {
+ throw new IllegalStateException(
+ "Duplicate output artifact found for unsharable actions."
+ + "This can happen if a previous event registered the action.\n"
+ + "Old action: "
+ + oldAction
+ + "\n\nNew action: "
+ + newAction
+ + "\n\nArtifact: "
+ + primaryOutput
+ + "\n");
}
} else {
storedComponent = newComponent;
@@ -346,9 +326,8 @@
private void addArtifactDependency(CriticalPathComponent actionStats, Artifact input) {
CriticalPathComponent depComponent = outputArtifactToComponent.get(input);
if (depComponent != null) {
- Action action = depComponent.maybeGetAction();
- if (depComponent.isRunning && action != null) {
- checkCriticalPathInconsistency(input, action, actionStats);
+ if (depComponent.isRunning()) {
+ checkCriticalPathInconsistency(input, depComponent.getAction(), actionStats);
return;
}
actionStats.addDepInfo(depComponent);
