Skip to content

Commit

Permalink
Intern VectorArgs more aggressively
Browse files Browse the repository at this point in the history 
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 bazelbuild#23638.

PiperOrigin-RevId: 676443873
Change-Id: I9539431011d3cbca1fb53357d00a7d0c0e53f522
  • Loading branch information
@fmeum @copybara-github
fmeum authored and copybara-github committed Sep 19, 2024
1 parent dafa4b5 commit d7c7a70
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 19 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
59 changes: 40 additions & 19 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -147,7 +149,12 @@ private enum StringificationType {
*/
@AutoCodec
static final class VectorArg {
private static final Interner<VectorArg> 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<VectorArg> strongInterner = BlazeInterners.newStrongInterner();
private static final Interner<VectorArg> weakInterner = BlazeInterners.newWeakInterner();

private static final int HAS_MAP_EACH = 1;
private static final int IS_NESTED_SET = 1 << 1;
Expand All @@ -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");
Expand All @@ -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);
}

Expand All @@ -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));
}
Expand Down Expand Up @@ -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<Object> originalValues;
Expand All @@ -291,7 +313,7 @@ private int preprocess(
NestedSet<Object> nestedSet = (NestedSet<Object>) 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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<Object> maybeExpandedValues =
maybeExpandDirectories(
artifactExpander,
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -1077,7 +1098,7 @@ private static void applyMapEach(
List<Object> 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 {
Expand Down

0 comments on commit d7c7a70

Please sign in to comment.