Skip to content

Commit

Permalink
[7.1.0] Return labels instead of strings from DescribableExecutionUni…
Browse files Browse the repository at this point in the history 
…t methods. (#20788)

Avoids an eager string conversion (and associated memory allocation)
when in a memory-sensitive context.

PiperOrigin-RevId: 585042874
Change-Id: I060303c844188653846c6849964bdd73aec0560b
  • Loading branch information
@tjgq
tjgq authored Jan 10, 2024
1 parent d03128a commit 71cca16
Show file tree
Hide file tree
Showing 10 changed files with 35 additions and 34 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ public void maybeReportSubcommand(Spawn spawn) {
/* environmentVariablesToClear= */ null,
getExecRoot().getPathString(),
spawn.getConfigurationChecksum(),
spawn.getExecutionPlatformLabelString());
spawn.getExecutionPlatformLabel());
getEventHandler().handle(Event.of(EventKind.SUBCOMMAND, null, "# " + reason + "\n" + message));
}

Expand Down
10 changes: 4 additions & 6 deletions src/main/java/com/google/devtools/build/lib/actions/Spawn.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.util.DescribableExecutionUnit;
import java.util.Collection;
import java.util.Objects;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -171,9 +170,9 @@ default boolean isMandatoryOutput(ActionInput output) {

@Override
@Nullable
default String getExecutionPlatformLabelString() {
default Label getExecutionPlatformLabel() {
PlatformInfo executionPlatform = getExecutionPlatform();
return executionPlatform == null ? null : Objects.toString(executionPlatform.label());
return executionPlatform != null ? executionPlatform.label() : null;
}

@Override
Expand All @@ -184,9 +183,8 @@ default String getConfigurationChecksum() {

@Override
@Nullable
default String getTargetLabel() {
Label label = getResourceOwner().getOwner().getLabel();
return label == null ? null : label.toString();
default Label getTargetLabel() {
return getResourceOwner().getOwner().getLabel();
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public void logSpawn(
builder.setMnemonic(spawn.getMnemonic());

if (spawn.getTargetLabel() != null) {
builder.setTargetLabel(spawn.getTargetLabel());
builder.setTargetLabel(spawn.getTargetLabel().toString());
}

SpawnMetrics metrics = result.getMetrics();
Expand Down
7 changes: 3 additions & 4 deletions ...a/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import java.util.Base64;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Collectors;
import net.starlark.java.eval.EvalException;

Expand Down Expand Up @@ -260,9 +259,9 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream)
/* environmentVariablesToClear= */ null,
/* cwd= */ null,
action.getOwner().getConfigurationChecksum(),
action.getExecutionPlatform() == null
? null
: Objects.toString(action.getExecutionPlatform().label())))
action.getExecutionPlatform() != null
? action.getExecutionPlatform().label()
: null))
.append("\n");
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
ENV_VARIABLES_TO_CLEAR,
runCommandLine.workingDir.getPathString(),
builtTargets.configuration.checksum(),
/* executionPlatformAsLabelString= */ null);
/* executionPlatformLabel= */ null);

PathFragment shExecutable = ShToolchain.getPathForHost(builtTargets.configuration);
if (shExecutable.isEmpty()) {
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/util/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ java_library(
name = "describable_execution_unit",
srcs = ["DescribableExecutionUnit.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//third_party:guava",
"//third_party:jsr305",
],
Expand All @@ -100,6 +101,7 @@ java_library(
":describable_execution_unit",
":os",
":shell_escaper",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/shell",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//third_party:error_prone_annotations",
Expand Down
15 changes: 8 additions & 7 deletions src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Ordering;
import com.google.devtools.build.lib.cmdline.Label;
import java.io.File;
import java.util.Collection;
import java.util.Comparator;
Expand Down Expand Up @@ -173,7 +174,7 @@ public static String describeCommand(
@Nullable List<String> environmentVariablesToClear,
@Nullable String cwd,
@Nullable String configurationChecksum,
@Nullable String executionPlatformAsLabelString) {
@Nullable Label executionPlatformLabel) {

Preconditions.checkNotNull(form);
StringBuilder message = new StringBuilder();
Expand Down Expand Up @@ -267,9 +268,9 @@ public static String describeCommand(
message.append("# Configuration: ").append(configurationChecksum);
}

if (executionPlatformAsLabelString != null) {
if (executionPlatformLabel != null) {
message.append("\n");
message.append("# Execution platform: ").append(executionPlatformAsLabelString);
message.append("# Execution platform: ").append(executionPlatformLabel);
}
}

Expand All @@ -288,8 +289,8 @@ static String describeCommandFailure(
Map<String, String> env,
@Nullable String cwd,
@Nullable String configurationChecksum,
@Nullable String targetLabel,
@Nullable String executionPlatformAsLabelString) {
@Nullable Label targetLabel,
@Nullable Label executionPlatformLabel) {

String commandName = commandLineElements.iterator().next();
// Extract the part of the command name after the last "/", if any.
Expand Down Expand Up @@ -318,7 +319,7 @@ static String describeCommandFailure(
null,
cwd,
configurationChecksum,
executionPlatformAsLabelString));
executionPlatformLabel));
return shortCommandName + " failed: " + output;
}

Expand All @@ -332,6 +333,6 @@ public static String describeCommandFailure(
cwd,
command.getConfigurationChecksum(),
command.getTargetLabel(),
command.getExecutionPlatformLabelString());
command.getExecutionPlatformLabel());
}
}
5 changes: 3 additions & 2 deletions src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.Label;
import javax.annotation.Nullable;

/**
Expand All @@ -23,7 +24,7 @@
public interface DescribableExecutionUnit {

@Nullable
default String getTargetLabel() {
default Label getTargetLabel() {
return null;
}

Expand All @@ -38,7 +39,7 @@ default String getTargetLabel() {

/** Returns the Label of the execution platform for the command, if any, as a String. */
@Nullable
default String getExecutionPlatformLabelString() {
default Label getExecutionPlatformLabel() {
return null;
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,6 @@ public String toString() {
/* environmentVariablesToClear= */ null,
execRoot.getPathString(),
/* configurationChecksum= */ null,
/* executionPlatformAsLabelString= */ null);
/* executionPlatformLabel= */ null);
}
}
22 changes: 11 additions & 11 deletions src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class CommandFailureUtilsTest {

@Test
public void describeCommandFailure() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[3];
args[0] = "/bin/sh";
args[1] = "-c";
Expand All @@ -51,7 +51,7 @@ public void describeCommandFailure() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"sh failed: error executing Mnemonic command (from target //foo:bar) "
Expand All @@ -60,7 +60,7 @@ public void describeCommandFailure() throws Exception {

@Test
public void describeCommandFailure_verbose() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[3];
args[0] = "/bin/sh";
args[1] = "-c";
Expand All @@ -80,7 +80,7 @@ public void describeCommandFailure_verbose() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"sh failed: error executing Mnemonic command (from target //foo:bar) \n"
Expand All @@ -94,7 +94,7 @@ public void describeCommandFailure_verbose() throws Exception {

@Test
public void describeCommandFailure_longMessage() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[40];
args[0] = "some_command";
for (int i = 1; i < args.length; i++) {
Expand All @@ -117,7 +117,7 @@ public void describeCommandFailure_longMessage() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"some_command failed: error executing Mnemonic command (from target //foo:bar) "
Expand All @@ -130,7 +130,7 @@ public void describeCommandFailure_longMessage() throws Exception {

@Test
public void describeCommandFailure_longMessage_verbose() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[40];
args[0] = "some_command";
for (int i = 1; i < args.length; i++) {
Expand All @@ -153,7 +153,7 @@ public void describeCommandFailure_longMessage_verbose() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"some_command failed: error executing Mnemonic command (from target //foo:bar) \n"
Expand All @@ -172,7 +172,7 @@ public void describeCommandFailure_longMessage_verbose() throws Exception {

@Test
public void describeCommandFailure_singleSkippedArgument() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[35]; // Long enough to make us skip 1 argument below.
args[0] = "some_command";
for (int i = 1; i < args.length; i++) {
Expand All @@ -191,7 +191,7 @@ public void describeCommandFailure_singleSkippedArgument() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"some_command failed: error executing Mnemonic command (from target //foo:bar)"
Expand Down Expand Up @@ -230,7 +230,7 @@ public void describeCommandPrettyPrintArgs() throws Exception {
envToClear,
cwd,
"cfg12345",
executionPlatform.label().toString());
executionPlatform.label());

assertThat(message)
.isEqualTo(
Expand Down

0 comments on commit 71cca16

Please sign in to comment.