bazel package loading: Cache, intra-package, `String` -> `Label` conversions during BUILD file evaluation.
For some packages, `LabelType#convert` is a CPU hotspot during BUILD file evaluation, and so the approach in this CL can be a pretty big performance improvement. (I *do* have a plan in mind to address that, but in practice I think this CL here will get rid of the hotspot.)
It's common for there to be duplicate label-strings encountered during evaluation of a single BUILD file. Consider
```
# blah/BUILD
java_library(name = 'a', srcs = ['A.java'], deps = ['//other:a', '//common:dep'])
java_library(name = 'b', srcs = ['B.java'], deps = ['//other:b', '//common:dep'])
```
In this case, we'll do the Starlark-land-String -> Bazel-land-Label conversion for "//common:dep" twice. `Label#LABEL_INTERNER` *is* useful for reducing duplicate retained objects, but we still have to wastefully construct the duplicate `Label` object (paying both the CPU, notably `Label#validateTargetName`, and GC churn costs), and then the CPU + contention cost of the `MapMakerInternalMap` lookup. Instead, by having a package-local cache in front of `Label#LABEL_INTERNER`, we avoid all of this. Therefore, for many BUILD files, this CL here reduces both CPU time and wall time, and also nicely reduces the total number of allocated bytes (even though are introducing a new data structure!).
In some "loading phase" benchmarks of internal Google targets at various points in the BUILD-land-spectrum, this change reduces wall time by between 5% and 50%, and total number of allocated bytes by between 1% and 36%.
PiperOrigin-RevId: 347602162
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
index f328596..b869ace 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -802,6 +802,7 @@
toolsRepository,
configurationFragmentMap,
repoMapping,
+ /*convertedLabelsInPackage=*/ new HashMap<>(),
new SymbolGenerator<>(fileLabel),
/*analysisRuleLabel=*/ null)
.storeInThread(thread);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java
index 28a52a5..a45def6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java
@@ -136,11 +136,13 @@
} else if (defaultValue instanceof StarlarkLateBoundDefault) {
builder.value((StarlarkLateBoundDefault) defaultValue); // unchecked cast
} else {
+ BazelStarlarkContext bazelStarlarkContext = BazelStarlarkContext.from(thread);
builder.defaultValue(
defaultValue,
new BuildType.LabelConversionContext(
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label(),
- BazelStarlarkContext.from(thread).getRepoMapping()),
+ bazelStarlarkContext.getRepoMapping(),
+ bazelStarlarkContext.getConvertedLabelsInPackage()),
DEFAULT_ARG);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java
index 0051f4d..9ce2a4c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java
@@ -52,6 +52,7 @@
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collections;
+import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
@@ -104,6 +105,7 @@
toolsRepository,
/*fragmentNameToClass=*/ null,
ruleContext.getTarget().getPackage().getRepositoryMapping(),
+ /*convertedLabelsInPackage=*/ new HashMap<>(),
ruleContext.getSymbolGenerator(),
ruleContext.getLabel())
.storeInThread(thread);
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
index 5652d97..b87bf6b 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
@@ -45,6 +45,7 @@
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import java.io.IOException;
+import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
@@ -156,6 +157,7 @@
/*toolsRepository=*/ null,
/*fragmentNameToClass=*/ null,
rule.getPackage().getRepositoryMapping(),
+ /*convertedLabelsInPackage=*/ new HashMap<>(),
new SymbolGenerator<>(key),
/*analysisRuleLabel=*/ null)
.storeInThread(thread);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkContext.java b/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkContext.java
index 813837f..80a0d95 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkContext.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkContext.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.analysis.RuleDefinitionContext;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryName;
+import java.util.HashMap;
import javax.annotation.Nullable;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Starlark;
@@ -50,6 +51,7 @@
private final String toolsRepository;
@Nullable private final ImmutableMap<String, Class<?>> fragmentNameToClass;
private final ImmutableMap<RepositoryName, RepositoryName> repoMapping;
+ private final HashMap<String, Label> convertedLabelsInPackage;
private final SymbolGenerator<?> symbolGenerator;
@Nullable private final Label analysisRuleLabel;
@@ -59,6 +61,8 @@
* @param fragmentNameToClass a map from configuration fragment name to configuration fragment
* class, such as "apple" to AppleConfiguration.class
* @param repoMapping a map from RepositoryName to RepositoryName to be used for external
+ * @param convertedLabelsInPackage a mutable map from String to Label, used during package loading
+ * of a single package.
* @param symbolGenerator a {@link SymbolGenerator} to be used when creating objects to be
* compared using reference equality.
* @param analysisRuleLabel is the label of the rule for an analysis phase (rule or aspect
@@ -77,12 +81,14 @@
String toolsRepository,
@Nullable ImmutableMap<String, Class<?>> fragmentNameToClass,
ImmutableMap<RepositoryName, RepositoryName> repoMapping,
+ HashMap<String, Label> convertedLabelsInPackage,
SymbolGenerator<?> symbolGenerator,
@Nullable Label analysisRuleLabel) {
this.phase = phase;
this.toolsRepository = toolsRepository;
this.fragmentNameToClass = fragmentNameToClass;
this.repoMapping = repoMapping;
+ this.convertedLabelsInPackage = convertedLabelsInPackage;
this.symbolGenerator = Preconditions.checkNotNull(symbolGenerator);
this.analysisRuleLabel = analysisRuleLabel;
}
@@ -113,6 +119,16 @@
return repoMapping;
}
+ /**
+ * Returns a String -> Label map of all the Strings that have already been converted to Labels
+ * during package loading of the current package.
+ *
+ * <p>This is used for a performance optimization during package loading, and unused otherwise.
+ */
+ public HashMap<String, Label> getConvertedLabelsInPackage() {
+ return convertedLabelsInPackage;
+ }
+
public SymbolGenerator<?> getSymbolGenerator() {
return symbolGenerator;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
index b3a9953..46f44d4 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
@@ -31,6 +31,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
@@ -199,11 +200,15 @@
public static class LabelConversionContext {
private final Label label;
private final ImmutableMap<RepositoryName, RepositoryName> repositoryMapping;
+ private final HashMap<String, Label> convertedLabelsInPackage;
public LabelConversionContext(
- Label label, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping) {
+ Label label,
+ ImmutableMap<RepositoryName, RepositoryName> repositoryMapping,
+ HashMap<String, Label> convertedLabelsInPackage) {
this.label = label;
this.repositoryMapping = repositoryMapping;
+ this.convertedLabelsInPackage = convertedLabelsInPackage;
}
public Label getLabel() {
@@ -214,6 +219,10 @@
return repositoryMapping;
}
+ HashMap<String, Label> getConvertedLabelsInPackage() {
+ return convertedLabelsInPackage;
+ }
+
@Override
public String toString() {
return label.toString();
@@ -275,9 +284,21 @@
return ((Label) context).getRelativeWithRemapping(str, ImmutableMap.of());
} else if (context instanceof LabelConversionContext) {
LabelConversionContext labelConversionContext = (LabelConversionContext) context;
- return labelConversionContext
- .getLabel()
- .getRelativeWithRemapping(str, labelConversionContext.getRepositoryMapping());
+ HashMap<String, Label> convertedLabelsInPackage =
+ labelConversionContext.getConvertedLabelsInPackage();
+ // Optimization: First check the package-local map, avoiding Label validation, Label
+ // construction, and global Interner lookup. This approach tends to be very profitable
+ // overall, since it's common for the targets in a single package to have duplicate
+ // label-strings across all their attribute values.
+ Label label = convertedLabelsInPackage.get(str);
+ if (label == null) {
+ label =
+ labelConversionContext
+ .getLabel()
+ .getRelativeWithRemapping(str, labelConversionContext.getRepositoryMapping());
+ convertedLabelsInPackage.put(str, label);
+ }
+ return label;
} else {
throw new ConversionException("invalid context '" + context + "' in " + what);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 46c5289..eba918c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -973,6 +973,8 @@
private final Interner<ImmutableList<?>> listInterner = new ThreadCompatibleInterner<>();
+ private final HashMap<String, Label> convertedLabelsInPackage = new HashMap<>();
+
private ImmutableMap<Location, String> generatorMap = ImmutableMap.of();
/** Returns the "generator_name" to use for a given call site location in a BUILD file. */
@@ -1080,6 +1082,10 @@
return listInterner;
}
+ HashMap<String, Label> getConvertedLabelsInPackage() {
+ return convertedLabelsInPackage;
+ }
+
/** Sets the name of this package's BUILD file. */
public Builder setFilename(RootedPath filename) {
this.filename = filename;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 58795a5..d3ca162 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -853,6 +853,7 @@
ruleClassProvider.getToolsRepository(),
/*fragmentNameToClass=*/ null,
pkgBuilder.getRepositoryMapping(),
+ pkgBuilder.getConvertedLabelsInPackage(),
new SymbolGenerator<>(pkgBuilder.getPackageIdentifier()),
/*analysisRuleLabel=*/ null)
.storeInThread(thread);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 682092a..bb6491e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -2081,13 +2081,13 @@
EventHandler eventHandler)
throws InterruptedException, CannotPrecomputeDefaultsException {
-
BitSet definedAttrIndices =
populateDefinedRuleAttributeValues(
rule,
pkgBuilder.getRepositoryMapping(),
attributeValues,
pkgBuilder.getListInterner(),
+ pkgBuilder.getConvertedLabelsInPackage(),
eventHandler);
populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices, eventHandler);
// Now that all attributes are bound to values, collect and store configurable attribute keys.
@@ -2110,6 +2110,7 @@
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping,
AttributeValues<T> attributeValues,
Interner<ImmutableList<?>> listInterner,
+ HashMap<String, Label> convertedLabelsInPackage,
EventHandler eventHandler) {
BitSet definedAttrIndices = new BitSet();
for (T attributeAccessor : attributeValues.getAttributeAccessors()) {
@@ -2142,7 +2143,13 @@
if (attributeValues.valuesAreBuildLanguageTyped()) {
try {
nativeAttributeValue =
- convertFromBuildLangType(rule, attr, attributeValue, repositoryMapping, listInterner);
+ convertFromBuildLangType(
+ rule,
+ attr,
+ attributeValue,
+ repositoryMapping,
+ listInterner,
+ convertedLabelsInPackage);
} catch (ConversionException e) {
rule.reportError(String.format("%s: %s", rule.getLabel(), e.getMessage()), eventHandler);
continue;
@@ -2407,9 +2414,11 @@
Attribute attr,
Object buildLangValue,
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping,
- Interner<ImmutableList<?>> listInterner)
+ Interner<ImmutableList<?>> listInterner,
+ HashMap<String, Label> convertedLabelsInPackage)
throws ConversionException {
- LabelConversionContext context = new LabelConversionContext(rule.getLabel(), repositoryMapping);
+ LabelConversionContext context =
+ new LabelConversionContext(rule.getLabel(), repositoryMapping, convertedLabelsInPackage);
Object converted =
BuildType.selectableConvert(
attr.getType(),
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index b089c74..6e6ee36 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -140,6 +140,7 @@
/*toolsRepository=*/ null,
/*fragmentNameToClass=*/ null,
/*repoMapping=*/ ImmutableMap.of(),
+ /*convertedLabelsInPackage=*/ new HashMap<>(),
new SymbolGenerator<>(workspaceFileKey),
/*analysisRuleLabel=*/ null)
.storeInThread(thread);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkAspectFactory.java
index 68964a0..d035bd4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkAspectFactory.java
@@ -30,6 +30,7 @@
import com.google.devtools.build.lib.packages.StarlarkDefinedAspect;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.StructProvider;
+import java.util.HashMap;
import java.util.Map;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
@@ -79,6 +80,7 @@
toolsRepository,
/*fragmentNameToClass=*/ null,
ruleContext.getRule().getPackage().getRepositoryMapping(),
+ /*convertedLabelsInPackage=*/ new HashMap<>(),
ruleContext.getSymbolGenerator(),
ruleContext.getLabel())
.storeInThread(thread);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
index 5635748..8e0d8db7 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
@@ -28,6 +28,7 @@
import com.google.devtools.build.lib.packages.Type.ConversionException;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
import net.starlark.java.eval.EvalException;
@@ -50,7 +51,10 @@
public final void setCurrentRule() throws Exception {
this.currentRule = Label.parseAbsolute("//quux:baz", ImmutableMap.of());
this.labelConversionContext =
- new LabelConversionContext(currentRule, /* repositoryMapping= */ ImmutableMap.of());
+ new LabelConversionContext(
+ currentRule,
+ /* repositoryMapping= */ ImmutableMap.of(),
+ /* convertedLabelsInPackage= */ new HashMap<>());
}
@Test
@@ -320,13 +324,22 @@
new LabelConversionContext(
currentRule,
ImmutableMap.of(
- RepositoryName.create("@orig_repo"), RepositoryName.create("@new_repo")));
+ RepositoryName.create("@orig_repo"), RepositoryName.create("@new_repo")),
+ /* convertedLabelsInPackage= */ new HashMap<>());
Label label = BuildType.LABEL.convert("@orig_repo//foo:bar", null, context);
assertThat(label)
.isEquivalentAccordingToCompareTo(
Label.parseAbsolute("@new_repo//foo:bar", ImmutableMap.of()));
}
+ @Test
+ public void testLabelConversionContextCaches() throws ConversionException {
+ assertThat(labelConversionContext.getConvertedLabelsInPackage())
+ .doesNotContainKey("//some:label");
+ BuildType.LABEL.convert("//some:label", "doesntmatter", labelConversionContext);
+ assertThat(labelConversionContext.getConvertedLabelsInPackage()).containsKey("//some:label");
+ }
+
/**
* Tests basic {@link Selector} functionality.
*/
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java
index b6a02cf..a2ac2b6 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java
@@ -33,6 +33,7 @@
import com.google.devtools.common.options.Options;
import com.google.devtools.common.options.OptionsParsingException;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Module;
@@ -127,6 +128,7 @@
TestConstants.TOOLS_REPOSITORY,
/*fragmentNameToClass=*/ null,
/*repoMapping=*/ ImmutableMap.of(),
+ /*convertedLabelsInPackage=*/ new HashMap<>(),
new SymbolGenerator<>(new Object()),
/*analysisRuleLabel=*/ null) // dummy value for tests
.storeInThread(thread);
