Reverse a RAM regression introduced by bazelbuild@7e53880

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

Author: brandjon

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 @@ public final class MacroInstance extends RuleOrMacroInstance {
   // 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 @@ public final class MacroInstance extends RuleOrMacroInstance {
    * <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 @@ public MacroInstance(
       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 @@ public Package.Metadata getPackageMetadata() {
     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.

src/main/java/com/google/devtools/build/lib/packages/Rule.java

@@ -112,7 +112,7 @@ public class Rule extends RuleOrMacroInstance implements Target {
       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);

src/main/java/com/google/devtools/build/lib/packages/RuleOrMacroInstance.java

@@ -43,9 +43,6 @@ public abstract class RuleOrMacroInstance implements DependencyFilter.AttributeI
   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 @@ public abstract class RuleOrMacroInstance implements DependencyFilter.AttributeI
    */
   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 @@ Object getAttrWithIndex(int attrIndex) {
    */
   @Nullable
   Object getAttrIfStored(int attrIndex) {
+    int attrCount = getAttributeProvider().getAttributeCount();
     checkPositionIndex(attrIndex, attrCount - 1);
     return switch (getAttrState()) {
       case MUTABLE -> attrValues[attrIndex];
@@ -271,7 +267,7 @@ Object getAttrIfStored(int attrIndex) {
         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 @@ void freeze() {
       indicesToStore.set(i);
     }
 
-    if (attrCount < ATTR_SIZE_THRESHOLD) {
+    if (getAttributeProvider().getAttributeCount() < ATTR_SIZE_THRESHOLD) {
       freezeSmall(indicesToStore);
     } else {
       freezeLarge(indicesToStore);
@@ -464,15 +460,16 @@ enum AttrState {
 
   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;
     }
     return attrCount < ATTR_SIZE_THRESHOLD ? AttrState.FROZEN_SMALL : AttrState.FROZEN_LARGE;
   }
 
   /** 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 @@ public List<Label> getVisibilityDeclaredLabels() {
     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);