Skip to content

Commit

Permalink
Disable string interning for label name attribute value
Browse files Browse the repository at this point in the history
Weak interner inside `StringCanonicalizer` does have some memory overhead. Some strings only have one instance during a blaze build. And thus, interning these strings introduces memory overhead from the additional `Map.entry` inside weak interner's `ConcurrentHashMap`.

One obvious example of these strings is the label name. It is rarely that label name duplicate with each other. So this cl avoids interning label name (attribute `name`'s value) in order to get rid of the additional map entries.

PiperOrigin-RevId: 540018375
Change-Id: I6bcfb4349b6f45927de51537c8cc091228d1012e
  • Loading branch information
yuyue730 authored and traversaro committed Jun 24, 2023
1 parent 6fa0237 commit 50872eb
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static com.google.devtools.build.lib.packages.Type.STRING_DICT;
import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
import static com.google.devtools.build.lib.packages.Type.STRING_LIST_DICT;
import static com.google.devtools.build.lib.packages.Type.STRING_NO_INTERN;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
Expand All @@ -59,6 +60,7 @@ public class AttributeFormatter {
private static final ImmutableSet<Type<?>> depTypes =
ImmutableSet.of(
STRING,
STRING_NO_INTERN,
LABEL,
OUTPUT,
STRING_LIST,
Expand Down Expand Up @@ -196,11 +198,11 @@ private static void writeAttributeValueToBuilder(
if (type == INTEGER) {
builder.setIntValue(((StarlarkInt) value).toIntUnchecked());
} else if (type == STRING
|| type == STRING_NO_INTERN
|| type == LABEL
|| type == NODEP_LABEL
|| type == OUTPUT
|| type == GENQUERY_SCOPE_TYPE) {

builder.setStringValue(value.toString());
} else if (type == STRING_LIST
|| type == LABEL_LIST
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,8 @@ private static ImmutableSet<String> attributeValues(AttributeMap rule, String at
// String attributes and lists are easy.
if (Type.STRING == attrType) {
return ImmutableSet.of(rule.get(attrName, Type.STRING));
} else if (Type.STRING_NO_INTERN == attrType) {
return ImmutableSet.of(rule.get(attrName, Type.STRING_NO_INTERN));
} else if (Type.STRING_LIST == attrType) {
return ImmutableSet.copyOf(rule.get(attrName, Type.STRING_LIST));
} else if (BuildType.LABEL == attrType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import static com.google.devtools.build.lib.packages.Type.STRING_DICT;
import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
import static com.google.devtools.build.lib.packages.Type.STRING_LIST_DICT;
import static com.google.devtools.build.lib.packages.Type.STRING_NO_INTERN;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
Expand All @@ -55,6 +56,7 @@ public class ProtoUtils {
.put(GENQUERY_SCOPE_TYPE_LIST, Discriminator.LABEL_LIST)
.put(NODEP_LABEL_LIST, Discriminator.STRING_LIST)
.put(STRING, Discriminator.STRING)
.put(STRING_NO_INTERN, Discriminator.STRING)
.put(STRING_LIST, Discriminator.STRING_LIST)
.put(OUTPUT, Discriminator.OUTPUT)
.put(OUTPUT_LIST, Discriminator.OUTPUT_LIST)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
import static com.google.devtools.build.lib.packages.Type.STRING;
import static com.google.devtools.build.lib.packages.Type.STRING_NO_INTERN;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
Expand Down Expand Up @@ -124,7 +124,7 @@ public class RuleClass {

/** The name attribute, present for all rules at index 0. */
static final Attribute NAME_ATTRIBUTE =
attr("name", STRING)
attr("name", STRING_NO_INTERN)
.nonconfigurable("All rules have a non-customizable \"name\" attribute")
.build();

Expand Down
27 changes: 24 additions & 3 deletions src/main/java/com/google/devtools/build/lib/packages/Type.java
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,23 @@ public Set<String> toTagSet(Object value, String name) {
/** The type of a Starlark integer in the signed 32-bit range. */
@SerializationConstant public static final Type<StarlarkInt> INTEGER = new IntegerType();

/** The type of a string. */
@SerializationConstant public static final Type<String> STRING = new StringType();
/**
* The type of a string which interns the string instance in {@link StringCanonicalizer}'s weak
* interner.
*/
@SerializationConstant
public static final Type<String> STRING = new StringType(/* internString= */ true);

/**
* The type of a string which does not intern the string instance.
*
* <p>When there is only one string instance created in blaze, interning it introduces memory
* overhead of an additional map entry in weak interner's map. So for attribute whose string value
* tends to not duplicate (for example rule name), it is preferable not to intern such string
* values.
*/
@SerializationConstant
public static final Type<String> STRING_NO_INTERN = new StringType(/* internString= */ false);

/** The type of a boolean. */
@SerializationConstant public static final Type<Boolean> BOOLEAN = new BooleanType();
Expand Down Expand Up @@ -438,6 +453,12 @@ public Set<String> toTagSet(Object value, String name) {
}

private static final class StringType extends Type<String> {
private final boolean internString;

public StringType(boolean internString) {
this.internString = internString;
}

@Override
public String cast(Object value) {
return (String) value;
Expand All @@ -462,7 +483,7 @@ public String convert(Object x, Object what, LabelConverter labelConverter)
if (!(x instanceof String)) {
throw new ConversionException(this, x, what);
}
return StringCanonicalizer.intern((String) x);
return internString ? StringCanonicalizer.intern((String) x) : (String) x;
}

@Override
Expand Down

0 comments on commit 50872eb

Please sign in to comment.