bazel packages: improve space efficiency of generator_{function,location} attributes by leveraging the recently introduced sparse representation of AttributeContainer.
Before this change, generator_* attributes were recorded explicitly, costing about 10 bytes (ignoring space to store the values) per generated rule.
When callstack is recorded, this is redundant information.
This change derives the attributes from the rule's instantiation call stack,
if and only if the --record_rule_instantiation_callstack flag is enabled.
When returning attribute value: If attribute is not explicitly stored, infer it from callstack.
During rule creation: If callstack recording is enabled, do not write generator_{location,function} attributes.
Consequences:
- This change should be a functional no-op, irrespective of whether callstack recording is enabled or not.
- If callstack recording is enabled, memory consumption is reduced
- In some corner cases generator_function returned is different, and likely fixes existing bug.
NOTE: No changes to generator_name, since that is NOT available in the callstack.
PiperOrigin-RevId: 329996187
diff --git a/src/main/java/com/google/devtools/build/lib/packages/CallStack.java b/src/main/java/com/google/devtools/build/lib/packages/CallStack.java
index 4ba6410..65a1880 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/CallStack.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/CallStack.java
@@ -84,14 +84,33 @@
StarlarkThread.CallStackEntry[] array = new StarlarkThread.CallStackEntry[size];
int i = size;
for (Node n = node; n != null; n = n.parent) {
- String file = strings.get(n.file);
- String name = strings.get(n.name);
- Location loc = Location.fromFileLineColumn(file, n.line, n.col);
- array[--i] = new StarlarkThread.CallStackEntry(name, loc);
+ array[--i] = nodeFrame(n);
}
return array;
}
+ /** Returns a single frame, like {@code toArray()[i]} but more efficient. */
+ public StarlarkThread.CallStackEntry getFrame(int i) {
+ for (Node n = node; n != null; n = n.parent) {
+ if (++i == size) {
+ return nodeFrame(n);
+ }
+ }
+ throw new IndexOutOfBoundsException(); // !(0 <= i < size)
+ }
+
+ /** Returns the number of frames in the call stack. */
+ public int size() {
+ return size;
+ }
+
+ private StarlarkThread.CallStackEntry nodeFrame(Node n) {
+ String file = strings.get(n.file);
+ String name = strings.get(n.name);
+ Location loc = Location.fromFileLineColumn(file, n.line, n.col);
+ return new StarlarkThread.CallStackEntry(name, loc);
+ }
+
// A Node is a node in a linked tree representing a prefix of the call stack.
// file and line are indices of strings in the builder's shared table.
private static class Node {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index 726aa85..81d8899 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -63,6 +63,9 @@
/** Label predicate that allows every label. */
public static final Predicate<Label> ALL_LABELS = Predicates.alwaysTrue();
+ private static final String GENERATOR_FUNCTION = "generator_function";
+ private static final String GENERATOR_LOCATION = "generator_location";
+
private final Label label;
private final Package pkg;
@@ -347,7 +350,30 @@
// the value for the attribute is actually a function to compute the value.
return null;
}
- return attr.getDefaultValue(null);
+ switch (attr.getName()) {
+ case GENERATOR_FUNCTION:
+ return callstack.size() > 1 ? callstack.getFrame(1).name : "";
+ case GENERATOR_LOCATION:
+ return callstack.size() > 1 ? relativeLocation(callstack.getFrame(0).location) : "";
+ default:
+ return attr.getDefaultValue(null);
+ }
+ }
+
+ @Nullable
+ private String relativeLocation(Location location) {
+ // Determining the workspace root only works reliably if both location and label point to files
+ // in the same package.
+ // It would be preferable to construct the path from the label itself, but this doesn't work for
+ // rules created from function calls in a subincluded file, even if both files share a path
+ // prefix (for example, when //a/package:BUILD subincludes //a/package/with/a/subpackage:BUILD).
+ // We can revert to that approach once subincludes aren't supported anymore.
+ //
+ // TODO(b/151165647): this logic has always been wrong:
+ // it spuriously matches occurrences of the package name earlier in the path.
+ String absolutePath = location.toString();
+ int pos = absolutePath.indexOf(getLabel().getPackageName());
+ return (pos < 0) ? null : absolutePath.substring(pos);
}
/**
@@ -434,6 +460,9 @@
* with the given name.
*/
public boolean isAttributeValueExplicitlySpecified(String attrName) {
+ if (attrName.equals(GENERATOR_FUNCTION) || attrName.equals(GENERATOR_LOCATION)) {
+ return wasCreatedByMacro();
+ }
Integer attrIndex = ruleClass.getAttributeIndex(attrName);
return attrIndex != null && attributes.isAttributeValueExplicitlySpecified(attrIndex);
}
@@ -442,12 +471,12 @@
* Returns whether this rule was created by a macro.
*/
public boolean wasCreatedByMacro() {
- return hasStringAttribute("generator_name") || hasStringAttribute("generator_function");
+ return hasStringAttribute("generator_name") || hasStringAttribute(GENERATOR_FUNCTION);
}
/** Returns the macro that generated this rule, or an empty string. */
public String getGeneratorFunction() {
- Object value = getAttr("generator_function");
+ Object value = getAttr(GENERATOR_FUNCTION);
if (value instanceof String) {
return (String) value;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index 0668a78..eda405c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.syntax.Location;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
+import com.google.devtools.build.lib.syntax.StarlarkThread.CallStackEntry;
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
@@ -107,7 +108,12 @@
}
AttributesAndLocation generator =
- generatorAttributesForMacros(pkgBuilder, attributeValues, callstack, label);
+ generatorAttributesForMacros(
+ pkgBuilder,
+ attributeValues,
+ callstack,
+ label,
+ semantics.recordRuleInstantiationCallstack());
// The raw stack is of the form [<toplevel>@BUILD:1, macro@lib.bzl:1, cc_library@<builtin>].
// If we're recording it (--record_rule_instantiation_callstack),
@@ -182,8 +188,6 @@
* rule, and have a build-language-typed value which can be converted to the appropriate
* native type of the attribute (i.e. via {@link BuildType#selectableConvert}). There must be
* a map entry for each non-optional attribute of this class of rule.
- * @param loc the location of the rule expression
- * @param thread the lexical environment of the function call which declared this rule (optional)
* @throws InvalidRuleException if the rule could not be constructed for any reason (e.g. no
* {@code name} attribute is defined)
* @throws NameConflictException if the rule's name or output files conflict with others in this
@@ -294,8 +298,9 @@
private static AttributesAndLocation generatorAttributesForMacros(
Package.Builder pkgBuilder,
BuildLangTypedAttributeValuesMap args,
- ImmutableList<StarlarkThread.CallStackEntry> stack,
- Label label) {
+ ImmutableList<CallStackEntry> stack,
+ Label label,
+ boolean recordRuleInstantiationCallstack) {
// For a callstack [BUILD <toplevel>, .bzl <function>, <rule>],
// location is that of the caller of 'rule' (the .bzl function).
Location location = stack.size() < 2 ? Location.BUILTIN : stack.get(stack.size() - 2).location;
@@ -312,6 +317,8 @@
// The stack must contain at least two entries:
// 0: the outermost function (e.g. a BUILD file),
// 1: the function called by it (e.g. a "macro" in a .bzl file).
+ // optionally followed by other Starlark or built-in functions,
+ // and finally the rule instantiation function.
if (stack.size() < 2 || !stack.get(1).location.file().endsWith(".bzl")) {
return new AttributesAndLocation(args, location); // macro is not a Starlark function
}
@@ -327,10 +334,14 @@
generatorName = (String) args.getAttributeValue("name");
}
builder.put("generator_name", generatorName);
- builder.put("generator_function", generatorFunction);
- String relativePath = maybeGetRelativeLocation(generatorLocation, label);
- if (relativePath != null) {
- builder.put("generator_location", relativePath);
+ if (!recordRuleInstantiationCallstack) {
+ // When we are recording the callstack, we can materialize the value from callstack
+ // as needed. So save memory by not recording it.
+ builder.put("generator_function", generatorFunction);
+ String relativePath = maybeGetRelativeLocation(generatorLocation, label);
+ if (relativePath != null) {
+ builder.put("generator_location", relativePath);
+ }
}
try {
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
index 24333d8..d1c9f1a 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
@@ -253,6 +253,31 @@
}
@Test
+ public void testGeneratorAttributesWhenCallstackEnabled_macro() throws Exception {
+ // generator_* attributes are derived using alternative logic from the call stack when
+ // --record_rule_instantiation_callstack is enabled. This test exercises that.
+ scratch.file(
+ "mypkg/inc.bzl",
+ "def _impl(ctx):",
+ " pass",
+ "",
+ "myrule = rule(implementation = _impl)",
+ "",
+ "def f(name):",
+ " g()",
+ "",
+ "def g():",
+ " myrule(name='a')",
+ "");
+ scratch.file("mypkg/BUILD", "load(':inc.bzl', 'f')", "f(name='foo')");
+ setStarlarkSemanticsOptions("--record_rule_instantiation_callstack");
+ Rule rule = (Rule) getTarget("//mypkg:a");
+ assertThat(rule.getAttr("generator_function")).isEqualTo("f");
+ assertThat(rule.getAttr("generator_location")).isEqualTo("mypkg/BUILD:2:2");
+ assertThat(rule.getAttr("generator_name")).isEqualTo("foo");
+ }
+
+ @Test
public void sanityCheckUserDefinedTestRule() throws Exception {
scratch.file(
"test/starlark/test_rule.bzl",
