From 6adc97614b07d5ddad8f6cc4ef19324dbe76483d Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 24 Nov 2023 02:40:27 -0800 Subject: [PATCH] [7.1.0] Return labels instead of strings from DescribableExecutionUnit methods. Avoids an eager string conversion (and associated memory allocation) when in a memory-sensitive context. PiperOrigin-RevId: 585042874 Change-Id: I060303c844188653846c6849964bdd73aec0560b --- .../lib/actions/ActionExecutionContext.java | 2 +- .../devtools/build/lib/actions/Spawn.java | 10 ++++----- .../build/lib/exec/SpawnLogContext.java | 2 +- ...ctionGraphTextOutputFormatterCallback.java | 7 +++--- .../lib/runtime/commands/RunCommand.java | 2 +- .../com/google/devtools/build/lib/util/BUILD | 2 ++ .../build/lib/util/CommandFailureUtils.java | 15 +++++++------ .../lib/util/DescribableExecutionUnit.java | 5 +++-- .../devtools/build/lib/worker/WorkerKey.java | 2 +- .../lib/util/CommandFailureUtilsTest.java | 22 +++++++++---------- 10 files changed, 35 insertions(+), 34 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java index b14cd4361fe8ba..78585cf4902373 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java @@ -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)); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java index 8fa244bf94ee96..282b7888ef8cae 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java @@ -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; /** @@ -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 @@ -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(); } /** diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java index cf0f3716a76ec3..765034996d46ce 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java @@ -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(); diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java index e1796b7a0798d5..f853e5de26342b 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java @@ -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; @@ -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"); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index 7e31e3949f7f13..54c35c4960c174 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -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()) { diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD index 8801079580351a..a7519e1fe9adf7 100644 --- a/src/main/java/com/google/devtools/build/lib/util/BUILD +++ b/src/main/java/com/google/devtools/build/lib/util/BUILD @@ -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", ], @@ -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", diff --git a/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java b/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java index 91584f4733b41a..537d1d32642645 100644 --- a/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java +++ b/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java @@ -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; @@ -173,7 +174,7 @@ public static String describeCommand( @Nullable List environmentVariablesToClear, @Nullable String cwd, @Nullable String configurationChecksum, - @Nullable String executionPlatformAsLabelString) { + @Nullable Label executionPlatformLabel) { Preconditions.checkNotNull(form); StringBuilder message = new StringBuilder(); @@ -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); } } @@ -288,8 +289,8 @@ static String describeCommandFailure( Map 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. @@ -318,7 +319,7 @@ static String describeCommandFailure( null, cwd, configurationChecksum, - executionPlatformAsLabelString)); + executionPlatformLabel)); return shortCommandName + " failed: " + output; } @@ -332,6 +333,6 @@ public static String describeCommandFailure( cwd, command.getConfigurationChecksum(), command.getTargetLabel(), - command.getExecutionPlatformLabelString()); + command.getExecutionPlatformLabel()); } } diff --git a/src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java b/src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java index afcc1ee18d4c2a..6ccc9a3b468f60 100644 --- a/src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java +++ b/src/main/java/com/google/devtools/build/lib/util/DescribableExecutionUnit.java @@ -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; /** @@ -23,7 +24,7 @@ public interface DescribableExecutionUnit { @Nullable - default String getTargetLabel() { + default Label getTargetLabel() { return null; } @@ -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; } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java index e20296beadcf16..92754b0569af4d 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java @@ -213,6 +213,6 @@ public String toString() { /* environmentVariablesToClear= */ null, execRoot.getPathString(), /* configurationChecksum= */ null, - /* executionPlatformAsLabelString= */ null); + /* executionPlatformLabel= */ null); } } diff --git a/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java b/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java index cf5341895661ff..d4949df013118c 100644 --- a/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java @@ -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"; @@ -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) " @@ -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"; @@ -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" @@ -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++) { @@ -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) " @@ -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++) { @@ -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" @@ -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++) { @@ -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)" @@ -230,7 +230,7 @@ public void describeCommandPrettyPrintArgs() throws Exception { envToClear, cwd, "cfg12345", - executionPlatform.label().toString()); + executionPlatform.label()); assertThat(message) .isEqualTo(