From b82dcdcfe71e8160c37a0963a5ed1b586e819266 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 26 Feb 2024 08:18:22 -0800 Subject: [PATCH] Introduce `ArgChunk`, a layer of indirection in `CommandLine` expansion that allows lazy command lines to provide an efficient way to obtain the total argument length without materializing the arguments. An `AbstractCommandLine` class is split out to provide simple implementations of the modified interface. Currently, all implementations use `AbstractCommandLine`, but soon `StarlarkCustomCommandLine` won't. At that point, it will use a lazy `Iterable` and an efficient custom implementation of `ArgChunk#totalArgLength`. PiperOrigin-RevId: 610413772 Change-Id: Id6216cdea28345ddd17d7fa15f1cd3857bc92715 --- .../lib/actions/AbstractCommandLine.java | 62 ++++++++ .../google/devtools/build/lib/actions/BUILD | 2 +- .../build/lib/actions/CommandLine.java | 144 ++++++++++++------ .../build/lib/actions/CommandLines.java | 27 ++-- .../analysis/actions/CustomCommandLine.java | 4 +- .../lib/analysis/actions/ShellCommand.java | 3 +- .../starlark/StarlarkCustomCommandLine.java | 3 +- .../lib/rules/cpp/CompileCommandLine.java | 3 +- .../build/lib/rules/cpp/LinkCommandLine.java | 4 +- .../lib/rules/cpp/LtoBackendArtifacts.java | 3 +- 10 files changed, 180 insertions(+), 75 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java new file mode 100644 index 00000000000000..7167484779f3a4 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java @@ -0,0 +1,62 @@ +// Copyright 2014 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.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.util.Fingerprint; +import javax.annotation.Nullable; + +/** + * Partial implementation of a {@link CommandLine} suitable for when expansion eagerly materializes + * strings. + */ +public abstract class AbstractCommandLine extends CommandLine { + + @Override + public final ArgChunk expand() throws CommandLineExpansionException, InterruptedException { + return new SimpleArgChunk(arguments()); + } + + @Override + public final ArgChunk expand(ArtifactExpander artifactExpander, PathMapper pathMapper) + throws CommandLineExpansionException, InterruptedException { + return new SimpleArgChunk(arguments(artifactExpander, pathMapper)); + } + + /** + * Returns the expanded command line with enclosed artifacts expanded by {@code artifactExpander} + * at execution time. + * + *

By default, this method just delegates to {@link #arguments()}, without performing any + * artifact expansion. Subclasses should override this method if they contain tree artifacts and + * need to expand them for proper argument evaluation. + */ + @Override + public Iterable arguments(ArtifactExpander artifactExpander, PathMapper pathMapper) + throws CommandLineExpansionException, InterruptedException { + return arguments(); + } + + @Override + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) + throws CommandLineExpansionException, InterruptedException { + for (String s : arguments()) { + fingerprint.addString(s); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 0195545961a36a..51cbcadd00989b 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -31,6 +31,7 @@ java_library( name = "actions", srcs = [ "AbstractAction.java", + "AbstractCommandLine.java", "Action.java", "ActionAnalysisMetadata.java", "ActionCacheAwareAction.java", @@ -169,7 +170,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/causes", "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/cmdline", - "//src/main/java/com/google/devtools/build/lib/collect", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/collect/nestedset:fingerprint_cache", "//src/main/java/com/google/devtools/build/lib/concurrent", diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java index 9ae00187386a9f..22e0f8a7792870 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.actions; import com.google.common.base.Joiner; +import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; @@ -23,51 +24,115 @@ /** A representation of a list of arguments. */ public abstract class CommandLine { - private static class EmptyCommandLine extends CommandLine { - @Override - public Iterable arguments() { - return ImmutableList.of(); - } - } public static final CommandLine EMPTY = new EmptyCommandLine(); - /** Returns the command line. */ - public abstract Iterable arguments() - throws CommandLineExpansionException, InterruptedException; + /** Returns a {@link CommandLine} backed by the given list of arguments. */ + public static CommandLine of(ImmutableList arguments) { + return arguments.isEmpty() ? CommandLine.EMPTY : new SimpleCommandLine(arguments); + } /** - * Returns the evaluated command line with enclosed artifacts expanded by {@code artifactExpander} - * at execution time. - * - *

By default, this method just delegates to {@link #arguments()}, without performing any - * artifact expansion. Subclasses should override this method if they contain TreeArtifacts and - * need to expand them for proper argument evaluation. + * Returns a {@link CommandLine} that is constructed by appending the {@code args} to {@code + * commandLine}. */ - public Iterable arguments(ArtifactExpander artifactExpander, PathMapper pathMapper) - throws CommandLineExpansionException, InterruptedException { - return arguments(); + public static CommandLine concat(CommandLine commandLine, ImmutableList args) { + if (args.isEmpty()) { + return commandLine; + } + if (commandLine == EMPTY) { + return CommandLine.of(args); + } + return new SuffixedCommandLine(args, commandLine); } /** - * Adds the command line to the provided {@link Fingerprint}. + * Post-expansion representation of command line arguments. * - *

Some of the implementations may require the to expand provided directory in order to produce - * a unique key. Consequently, the result of calling this function can be different depending on - * whether the {@link ArtifactExpander} is provided. Moreover, without it, the produced key may - * not always be unique. + *

This differs from {@link CommandLine} in that consuming the arguments is guaranteed to be + * free of {@link CommandLineExpansionException} and {@link InterruptedException}. + */ + public interface ArgChunk { + + /** + * Returns the arguments. + * + *

The returned {@link Iterable} may lazily materialize strings during iteration, so + * consumers should attempt to avoid iterating more times than necessary. + */ + Iterable arguments(); + + /** + * Counts the total length of all arguments in this chunk. + * + *

Implementations that lazily materialize strings may be able to compute the total argument + * length without actually materializing the arguments. + */ + int totalArgLength(); + } + + /** Implementation of {@link ArgChunk} that delegates to an {@link Iterable}. */ + public static final class SimpleArgChunk implements ArgChunk { + private final Iterable args; + + public SimpleArgChunk(Iterable args) { + this.args = args; + } + + @Override + public Iterable arguments() { + return args; + } + + @Override + public int totalArgLength() { + int total = 0; + for (String arg : args) { + total += arg.length() + 1; + } + return total; + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this).add("args", args).toString(); + } + } + + /** Returns the expanded command line. */ + public abstract ArgChunk expand() throws CommandLineExpansionException, InterruptedException; + + /** + * Returns the expanded command line with enclosed artifacts expanded by {@code artifactExpander} + * at execution time. */ - public void addToFingerprint( + public abstract ArgChunk expand(ArtifactExpander artifactExpander, PathMapper pathMapper) + throws CommandLineExpansionException, InterruptedException; + + /** Identical to calling {@code expand().arguments()}. */ + public abstract Iterable arguments() + throws CommandLineExpansionException, InterruptedException; + + /** Identical to calling {@code expand(artifactExpander, pathMapper).arguments()}. */ + public abstract Iterable arguments( + ArtifactExpander artifactExpander, PathMapper pathMapper) + throws CommandLineExpansionException, InterruptedException; + + /** Adds this command line to the provided {@link Fingerprint}. */ + public abstract void addToFingerprint( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander, Fingerprint fingerprint) - throws CommandLineExpansionException, InterruptedException { - for (String s : arguments()) { - fingerprint.addString(s); + throws CommandLineExpansionException, InterruptedException; + + private static final class EmptyCommandLine extends AbstractCommandLine { + @Override + public ImmutableList arguments() { + return ImmutableList.of(); } } - private static final class SimpleCommandLine extends CommandLine { + private static final class SimpleCommandLine extends AbstractCommandLine { private final ImmutableList args; SimpleCommandLine(ImmutableList args) { @@ -80,12 +145,7 @@ public ImmutableList arguments() { } } - /** Returns a {@link CommandLine} backed by the given list of arguments. */ - public static CommandLine of(ImmutableList arguments) { - return arguments.isEmpty() ? CommandLine.EMPTY : new SimpleCommandLine(arguments); - } - - private static final class SuffixedCommandLine extends CommandLine { + private static final class SuffixedCommandLine extends AbstractCommandLine { private final ImmutableList executableArgs; private final CommandLine commandLine; @@ -106,26 +166,12 @@ public Iterable arguments(ArtifactExpander artifactExpander, PathMapper } } - /** - * Returns a {@link CommandLine} that is constructed by appending the {@code args} to {@code - * commandLine}. - */ - public static CommandLine concat(CommandLine commandLine, ImmutableList args) { - if (args.isEmpty()) { - return commandLine; - } - if (commandLine == EMPTY) { - return CommandLine.of(args); - } - return new SuffixedCommandLine(args, commandLine); - } - /** * This helps when debugging Blaze code that uses {@link CommandLine}s, as you can see their * content directly in the variable inspector. */ @Override - public String toString() { + public final String toString() { try { return Joiner.on(' ').join(arguments()); } catch (CommandLineExpansionException e) { diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java index a330f93331ff16..77db1b66ed98e4 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java @@ -18,6 +18,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.actions.CommandLine.ArgChunk; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.util.Fingerprint; @@ -100,16 +101,16 @@ ExpandedCommandLines expand( for (CommandLineAndParamFileInfo pair : commandLines) { CommandLine commandLine = pair.commandLine; ParamFileInfo paramFileInfo = pair.paramFileInfo; - Iterable args = commandLine.arguments(artifactExpander, pathMapper); + ArgChunk chunk = commandLine.expand(artifactExpander, pathMapper); if (paramFileInfo == null) { - arguments.addAll(args); - cmdLineLength += totalArgLen(args); + arguments.addAll(chunk.arguments()); + cmdLineLength += chunk.totalArgLength(); } else { boolean useParamFile = true; if (!paramFileInfo.always()) { - int tentativeCmdLineLength = cmdLineLength + totalArgLen(args); + int tentativeCmdLineLength = cmdLineLength + chunk.totalArgLength(); if (tentativeCmdLineLength <= conservativeMaxLength) { - arguments.addAll(args); + arguments.addAll(chunk.arguments()); cmdLineLength = tentativeCmdLineLength; useParamFile = false; } @@ -132,10 +133,10 @@ ExpandedCommandLines expand( paramFiles.add( new ParamFileActionInput( paramFileExecPath, - ParameterFile.flagsOnly(args), + ParameterFile.flagsOnly(chunk.arguments()), paramFileInfo.getFileType(), paramFileInfo.getCharset())); - for (String positionalArg : ParameterFile.nonFlags(args)) { + for (String positionalArg : ParameterFile.nonFlags(chunk.arguments())) { arguments.add(positionalArg); cmdLineLength += positionalArg.length() + 1; } @@ -143,7 +144,7 @@ ExpandedCommandLines expand( paramFiles.add( new ParamFileActionInput( paramFileExecPath, - args, + chunk.arguments(), paramFileInfo.getFileType(), paramFileInfo.getCharset())); } @@ -277,14 +278,6 @@ public Iterable getArguments() { */ public abstract ImmutableList unpack(); - private static int totalArgLen(Iterable args) { - int result = 0; - for (String s : args) { - result += s.length() + 1; - } - return result; - } - private static void addParamFileInfoToFingerprint( ParamFileInfo paramFileInfo, Fingerprint fingerprint) { fingerprint.addUUID(PARAM_FILE_UUID); @@ -521,7 +514,7 @@ public ImmutableList unpack() { } } - private static class SingletonCommandLine extends CommandLine { + private static class SingletonCommandLine extends AbstractCommandLine { private final Object arg; SingletonCommandLine(Object arg) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java index 0514de1f3cdc05..5f80db92a19e17 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java @@ -22,12 +22,12 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; import com.google.common.collect.Maps; +import com.google.devtools.build.lib.actions.AbstractCommandLine; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; -import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineItem; import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn; @@ -61,7 +61,7 @@ /** A customizable, serializable class for building memory efficient command lines. */ @Immutable -public class CustomCommandLine extends CommandLine { +public class CustomCommandLine extends AbstractCommandLine { private interface ArgvFragment { /** * Expands this fragment into the passed command line vector. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ShellCommand.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ShellCommand.java index ad6e4c271dbfa1..73b6fba1a079aa 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ShellCommand.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ShellCommand.java @@ -17,6 +17,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.AbstractCommandLine; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.vfs.PathFragment; @@ -27,7 +28,7 @@ * and {@link #command}. Supports optionally padding the command line with an empty argument, which * can be useful to ensure that any subsequent arguments get assigned to {@code $1} etc. */ -final class ShellCommand extends CommandLine { +final class ShellCommand extends AbstractCommandLine { private final PathFragment shExecutable; private final String command; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java index b43c427583a1df..335df8cf04d7b0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; import com.google.common.collect.Sets; +import com.google.devtools.build.lib.actions.AbstractCommandLine; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; @@ -66,7 +67,7 @@ import net.starlark.java.syntax.Location; /** Supports ctx.actions.args() from Starlark. */ -public class StarlarkCustomCommandLine extends CommandLine { +public class StarlarkCustomCommandLine extends AbstractCommandLine { private static final Joiner LINE_JOINER = Joiner.on("\n").skipNulls(); private static final Joiner FIELD_JOINER = Joiner.on(": ").skipNulls(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java index 7fc061f70ee55d..81d62cd560990c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java @@ -17,6 +17,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.AbstractCommandLine; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; @@ -107,7 +108,7 @@ List getArguments( * @param cppCompileAction - {@link CppCompileAction} owning this {@link CompileCommandLine}. */ public CommandLine getFilteredFeatureConfigurationCommandLine(CppCompileAction cppCompileAction) { - return new CommandLine() { + return new AbstractCommandLine() { @Override public Iterable arguments() throws CommandLineExpansionException { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java index 70fda38668b22e..cb66aa5bfcf339 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java @@ -20,8 +20,8 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.AbstractCommandLine; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; -import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLines; import com.google.devtools.build.lib.actions.ParamFileInfo; @@ -43,7 +43,7 @@ * as well as static libraries. */ @Immutable -public final class LinkCommandLine extends CommandLine { +public final class LinkCommandLine extends AbstractCommandLine { private final String actionName; private final String forcedToolPath; private final CcToolchainVariables variables; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendArtifacts.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendArtifacts.java index 334f423f2a9daa..f41ae41419d336 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendArtifacts.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendArtifacts.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.AbstractCommandLine; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -323,7 +324,7 @@ private static void addCommandLineToLtoBackendActionBuilder( CcToolchainVariables buildVariables, boolean usePic) { CommandLine ltoCommandLine = - new CommandLine() { + new AbstractCommandLine() { @Override public Iterable arguments() throws CommandLineExpansionException {