From d7c7a70bbe3b9293a27fdac5fd98401b97ddc25c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 19 Sep 2024 09:25:20 -0700 Subject: [PATCH] Intern `VectorArg`s more aggressively Path mapping requires turning `args.add(file.dirname)` into `args.add_all([file], map_each = _dirname)`. This usage can be made more memory efficient if we * avoid storing the count of 1 by encoding it in `VectorArg#features`; * intern `StarlarkSemantics` in `VectorArg` as a single instance is shared across the build. This saves 4 bytes for each `map_each` usage. Closes #23638. PiperOrigin-RevId: 676443873 Change-Id: I9539431011d3cbca1fb53357d00a7d0c0e53f522 --- .../google/devtools/build/lib/analysis/BUILD | 1 + .../starlark/StarlarkCustomCommandLine.java | 59 +++++++++++++------ 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 640d53b37b809c..38c50027db8d89 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -2571,6 +2571,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", "//src/main/java/com/google/devtools/build/lib/util", + "//src/main/java/com/google/devtools/build/lib/util:hash_codes", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", 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 d36daad0865b49..c318d95a8e246c 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 @@ -55,6 +55,7 @@ import com.google.devtools.build.lib.starlarkbuildapi.FileApi; import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi; import com.google.devtools.build.lib.util.Fingerprint; +import com.google.devtools.build.lib.util.HashCodes; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.ArrayList; @@ -64,6 +65,7 @@ import java.util.Iterator; import java.util.List; import java.util.NoSuchElementException; +import java.util.Objects; import java.util.UUID; import java.util.function.Consumer; import javax.annotation.Nullable; @@ -147,7 +149,12 @@ private enum StringificationType { */ @AutoCodec static final class VectorArg { - private static final Interner interner = BlazeInterners.newStrongInterner(); + + // The strong interner is used when StarlarkSemantics is null. The weak interner is used when + // StarlarkSemantics is present (implying a map_each), since StarlarkSemantics can change + // between builds. + private static final Interner strongInterner = BlazeInterners.newStrongInterner(); + private static final Interner weakInterner = BlazeInterners.newWeakInterner(); private static final int HAS_MAP_EACH = 1; private static final int IS_NESTED_SET = 1 << 1; @@ -160,6 +167,7 @@ static final class VectorArg { private static final int HAS_JOIN_WITH = 1 << 8; private static final int HAS_FORMAT_JOINED = 1 << 9; private static final int HAS_TERMINATE_WITH = 1 << 10; + private static final int HAS_SINGLE_ARG = 1 << 11; private static final UUID EXPAND_DIRECTORIES_UUID = UUID.fromString("9d7520d2-a187-11e8-98d0-529269fb1459"); @@ -182,19 +190,28 @@ static final class VectorArg { private final int features; private final StringificationType stringificationType; + @Nullable private final StarlarkSemantics starlarkSemantics; // Null unless map_each is present. - private VectorArg(int features, StringificationType stringificationType) { + private VectorArg( + int features, + StringificationType stringificationType, + @Nullable StarlarkSemantics starlarkSemantics) { this.features = features; this.stringificationType = stringificationType; + this.starlarkSemantics = starlarkSemantics; } - private static VectorArg create(int features, StringificationType stringificationType) { - return interner.intern(new VectorArg(features, stringificationType)); + private static VectorArg create( + int features, + StringificationType stringificationType, + @Nullable StarlarkSemantics starlarkSemantics) { + return intern(new VectorArg(features, stringificationType, starlarkSemantics)); } @VisibleForSerialization @AutoCodec.Interner static VectorArg intern(VectorArg vectorArg) { + var interner = vectorArg.starlarkSemantics == null ? strongInterner : weakInterner; return interner.intern(vectorArg); } @@ -216,18 +233,25 @@ private static void push( features |= arg.joinWith != null ? HAS_JOIN_WITH : 0; features |= arg.formatJoined != null ? HAS_FORMAT_JOINED : 0; features |= arg.terminateWith != null ? HAS_TERMINATE_WITH : 0; - arguments.add(VectorArg.create(features, arg.nestedSetStringificationType)); + features |= arg.nestedSet == null && arg.list.size() == 1 ? HAS_SINGLE_ARG : 0; + arguments.add( + VectorArg.create( + features, + arg.nestedSetStringificationType, + arg.mapEach != null ? starlarkSemantics : null)); if (arg.mapEach != null) { arguments.add(arg.mapEach); arguments.add(arg.location); - arguments.add(starlarkSemantics); } if (arg.nestedSet != null) { arguments.add(arg.nestedSet); } else { List list = arg.list; int count = list.size(); - arguments.add(count); + if (count != 1) { + // A count of 1 is encoded via the HAS_SINGLE_ARG feature. + arguments.add(count); + } for (int i = 0; i < count; ++i) { arguments.add(list.get(i)); } @@ -277,12 +301,10 @@ private int preprocess( @Nullable RepositoryMapping mainRepoMapping) throws CommandLineExpansionException, InterruptedException { StarlarkCallable mapEach = null; - StarlarkSemantics starlarkSemantics = null; Location location = null; if ((features & HAS_MAP_EACH) != 0) { mapEach = (StarlarkCallable) arguments.get(argi++); location = (Location) arguments.get(argi++); - starlarkSemantics = (StarlarkSemantics) arguments.get(argi++); } List originalValues; @@ -291,7 +313,7 @@ private int preprocess( NestedSet nestedSet = (NestedSet) arguments.get(argi++); originalValues = nestedSet.toList(); } else { - int count = (Integer) arguments.get(argi++); + int count = (features & HAS_SINGLE_ARG) != 0 ? 1 : (Integer) arguments.get(argi++); originalValues = arguments.subList(argi, argi + count); argi += count; } @@ -477,11 +499,9 @@ private int addToFingerprint( throws CommandLineExpansionException, InterruptedException { StarlarkCallable mapEach = null; Location location = null; - StarlarkSemantics starlarkSemantics = null; if ((features & HAS_MAP_EACH) != 0) { mapEach = (StarlarkCallable) arguments.get(argi++); location = (Location) arguments.get(argi++); - starlarkSemantics = (StarlarkSemantics) arguments.get(argi++); } // NestedSets and lists never result in the same fingerprint as the @@ -529,7 +549,7 @@ private int addToFingerprint( actionKeyContext.addNestedSetToFingerprint(fingerprint, values); } } else { - int count = (Integer) arguments.get(argi++); + int count = (features & HAS_SINGLE_ARG) != 0 ? 1 : (Integer) arguments.get(argi++); List maybeExpandedValues = maybeExpandDirectories( artifactExpander, @@ -704,17 +724,18 @@ public boolean equals(Object o) { if (this == o) { return true; } - if (o == null || getClass() != o.getClass()) { + if (!(o instanceof VectorArg that)) { return false; } - VectorArg vectorArg = (VectorArg) o; - return features == vectorArg.features - && stringificationType.equals(vectorArg.stringificationType); + return features == that.features + && stringificationType.equals(that.stringificationType) + && Objects.equals(starlarkSemantics, that.starlarkSemantics); } @Override public int hashCode() { - return 31 * Integer.hashCode(features) + stringificationType.hashCode(); + return 31 * HashCodes.hashObjects(stringificationType, starlarkSemantics) + + Integer.hashCode(features); } } @@ -1077,7 +1098,7 @@ private static void applyMapEach( List args = new ArrayList<>(2); args.add(null); // This will be overwritten each iteration. if (wantsDirectoryExpander) { - final DirectoryExpander expander; + DirectoryExpander expander; if (artifactExpander != null) { expander = new FullExpander(artifactExpander); } else {