Reverse a RAM regression introduced by https://github.com/bazelbuild/bazel/commit/7e538803d775929beb3c9af1cd2e87667d373bbe
That CL factored some content of `Rule` into a new base class, `RuleOrMacroInstance`. In doing so, it added two new fields to `Rule` instances, causing a 1% RAM regression on a large `query` invocation.
This CL eliminates the two extra fields.
`attrCount`, the number of attributes in the rule or macro's schema (not to be confused with the number of attributes actually stored on the instance, which may change after freezing/compacting), can be obtained from the schema via the subclass's `ruleClass` or `macroClass` field without storing it as an additional field on the base class. We have to take care not to do this retrieval of the subclass field while executing the super constructor (i.e. before the subclass field is initialized). That's solved by threading `attrCount` into `bitSetSize` as a parameter.
(A prior draft of this CL went through some indirect initialization logic whereby the super constructor called an abstract method to initialized the subclass field. Thanks to arostovtsev@ for pointing out that this was unnecessary.)
The second field, `packageDeclarations`, is replaced by an abstract getter that already exists on `Rule` and which is added to `MacroInstance` in this CL. It's probably not really needed on `MacroInstance`, but we can eliminate that latter; the cost of an extra field on a macro is nothing like the cost of an extra field on a rule.
PiperOrigin-RevId: 740793903
Change-Id: Iec65f20ff459e33c3961b16d9d51d0d4a22f0e00
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
index 03f216b..7b2fd1c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
@@ -27,7 +27,6 @@
import java.util.List;
import java.util.function.Consumer;
import javax.annotation.Nullable;
-import net.starlark.java.eval.EvalException;
/**
* Represents a use of a symbolic macro in a package.
@@ -45,6 +44,11 @@
// single field of type Object, and walk up the parent hierarchy to answer getPackage() queries.
private final Package.Metadata packageMetadata;
+ // TODO(bazel-team): This is only needed for RuleOrMacroInstance#getPackageDeclarations(), which
+ // is used by the attribute mapper logic. That might only be needed for rules rather than macros.
+ // Consider removing it and pushing getPackageDeclarations() down to Rule.
+ private final Package.Declarations packageDeclarations;
+
@Nullable private final MacroInstance parent;
private final MacroClass macroClass;
@@ -57,9 +61,6 @@
* <p>{@code sameNameDepth} is the number of macro instances that this one is inside of that share
* its name. For most instances it is 1, but for the main submacro of a parent macro it is one
* more than the parent's depth.
- *
- * @throws EvalException if there's a problem with the attribute values (currently, only thrown if
- * the {@code visibility} value is invalid)
*/
// TODO: #19922 - Better encapsulate the invariant around attrValues, by either transitioning to
// storing internal-typed values (the way Rules do) instead of Starlark-typed values, or else just
@@ -71,8 +72,9 @@
MacroClass macroClass,
Label label,
int sameNameDepth) {
- super(label, packageDeclarations, macroClass.getAttributeProvider().getAttributeCount());
+ super(label, macroClass.getAttributeProvider().getAttributeCount());
this.packageMetadata = packageMetadata;
+ this.packageDeclarations = packageDeclarations;
this.parent = parent;
this.macroClass = macroClass;
Preconditions.checkArgument(sameNameDepth > 0);
@@ -84,6 +86,11 @@
return packageMetadata;
}
+ @Override
+ Declarations getPackageDeclarations() {
+ return packageDeclarations;
+ }
+
/**
* Returns the macro instance that instantiated this one, or null if this was created directly
* during BUILD evaluation.
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 e140b8c..9eb15fb 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
@@ -112,7 +112,7 @@
RuleClass ruleClass,
Location location,
@Nullable CallStack.Node interiorCallStack) {
- super(label, pkg.getDeclarations(), ruleClass.getAttributeProvider().getAttributeCount());
+ super(label, ruleClass.getAttributeProvider().getAttributeCount());
this.pkg = checkNotNull(pkg);
this.ruleClass = checkNotNull(ruleClass);
this.location = checkNotNull(location);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleOrMacroInstance.java b/src/main/java/com/google/devtools/build/lib/packages/RuleOrMacroInstance.java
index 1c87f8d..cf33020 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleOrMacroInstance.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleOrMacroInstance.java
@@ -43,9 +43,6 @@
static final String GENERATOR_LOCATION = "generator_location";
private static final int ATTR_SIZE_THRESHOLD = 126;
- private final int attrCount;
-
- @Nullable private final Declarations packageDeclarations;
/**
* For {@link Rule}s, the length of this instance's generator name if it is a prefix of its name,
@@ -68,12 +65,10 @@
*/
int generatorNamePrefixLength = 0;
- RuleOrMacroInstance(Label label, Declarations packageDeclarations, int attrCount) {
+ RuleOrMacroInstance(Label label, int attrCount) {
this.label = checkNotNull(label);
- this.packageDeclarations = packageDeclarations;
this.attrValues = new Object[attrCount];
- this.attrCount = attrCount;
- this.attrBytes = new byte[bitSetSize()];
+ this.attrBytes = new byte[bitSetSize(attrCount)];
}
/**
@@ -260,6 +255,7 @@
*/
@Nullable
Object getAttrIfStored(int attrIndex) {
+ int attrCount = getAttributeProvider().getAttributeCount();
checkPositionIndex(attrIndex, attrCount - 1);
return switch (getAttrState()) {
case MUTABLE -> attrValues[attrIndex];
@@ -271,7 +267,7 @@
if (attrBytes.length == 0) {
yield null;
}
- int bitSetSize = bitSetSize();
+ int bitSetSize = bitSetSize(attrCount);
int index = binarySearchAttrBytes(bitSetSize, attrIndex, 0xff);
yield index < 0 ? null : attrValues[index - bitSetSize];
}
@@ -408,7 +404,7 @@
indicesToStore.set(i);
}
- if (attrCount < ATTR_SIZE_THRESHOLD) {
+ if (getAttributeProvider().getAttributeCount() < ATTR_SIZE_THRESHOLD) {
freezeSmall(indicesToStore);
} else {
freezeLarge(indicesToStore);
@@ -464,7 +460,8 @@
AttrState getAttrState() {
// This check works because the name attribute is never stored, so the compact representation
- // of attrValues will always have length < attrCount.
+ // of attrValues will always have length < attr count.
+ int attrCount = getAttributeProvider().getAttributeCount();
if (attrValues.length == attrCount) {
return AttrState.MUTABLE;
}
@@ -472,7 +469,7 @@
}
/** Calculates the number of bytes necessary to have an explicit bit for each attribute. */
- private int bitSetSize() {
+ private static int bitSetSize(int attrCount) {
// ceil(attrCount / 8)
return (attrCount + 7) / 8;
}
@@ -548,9 +545,11 @@
return rawLabels != null ? rawLabels : getDefaultVisibility().getDeclaredLabels();
}
+ abstract Declarations getPackageDeclarations();
+
@Nullable
public PackageArgs getPackageArgs() {
- return packageDeclarations.getPackageArgs();
+ return getPackageDeclarations().getPackageArgs();
}
abstract void reportError(String message, EventHandler eventHandler);
