Rephrase AbstractAttributeMapper#visitLabels such that we can avoid creating a temporary Type.LabelVisitor instance per Attribute being visited. Instead, we can now create one temporary object per visitation. Getting rid of this dimension of scaling reduces the amount of garbage created.

RELNOTES: None

PiperOrigin-RevId: 152161836

8 files changed, 107 insertions, 101 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
index 33f7d348f6..bc24ae9862 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java
@@ -823,15 +823,15 @@ public class ConstraintSemantics {
    */
   private static void addSelectValuesToSet(BuildType.Selector<?> select, final Set<Label> set) {
     Type<?> type = select.getOriginalType();
-    LabelVisitor visitor = new LabelVisitor() {
+    LabelVisitor<?> visitor = new LabelVisitor<Object>() {
       @Override
-      public void visit(Label label) {
+      public void visit(Label label, Object dummy) {
         set.add(label);
       }
     };
     for (Object value : select.getEntries().values()) {
       try {
-        type.visitLabels(visitor, value);
+        type.visitLabels(visitor, value, /*context=*/ null);
       } catch (InterruptedException ex) {
         // Because the LabelVisitor does not throw InterruptedException, it should not be thrown
         // by visitLabels here.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
index 24d1080232..8b99d8182b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
@@ -135,33 +135,34 @@ public abstract class AbstractAttributeMapper implements AttributeMap {
   }
 
   @Override
-  public void visitLabels(AcceptsLabelAttribute observer) throws InterruptedException {
+  public void visitLabels(final AcceptsLabelAttribute observer) throws InterruptedException {
+    Type.LabelVisitor<Attribute> visitor = new Type.LabelVisitor<Attribute>() {
+      @Override
+      public void visit(@Nullable Label label, Attribute attribute) throws InterruptedException {
+        if (label != null) {
+          Label absoluteLabel = ruleLabel.resolveRepositoryRelative(label);
+          observer.acceptLabelAttribute(absoluteLabel, attribute);
+        }
+      }
+    };
     for (Attribute attribute : ruleClass.getAttributes()) {
       Type<?> type = attribute.getType();
       // TODO(bazel-team): clean up the typing / visitation interface so we don't have to
       // special-case these types.
       if (type != BuildType.OUTPUT && type != BuildType.OUTPUT_LIST
           && type != BuildType.NODEP_LABEL && type != BuildType.NODEP_LABEL_LIST) {
-        visitLabels(attribute, observer);
+        visitLabels(attribute, visitor);
       }
     }
   }
 
   /** Visits all labels reachable from the given attribute. */
-  protected void visitLabels(final Attribute attribute, final AcceptsLabelAttribute observer)
+  protected void visitLabels(Attribute attribute, Type.LabelVisitor<Attribute> visitor)
       throws InterruptedException {
     Type<?> type = attribute.getType();
     Object value = get(attribute.getName(), type);
     if (value != null) { // null values are particularly possible for computed defaults.
-      type.visitLabels(new Type.LabelVisitor() {
-        @Override
-        public void visit(@Nullable Label label) throws InterruptedException {
-          if (label != null) {
-            Label absoluteLabel = ruleLabel.resolveRepositoryRelative(label);
-            observer.acceptLabelAttribute(absoluteLabel, attribute);
-          }
-        }
-      }, value);
+      type.visitLabels(visitor, value, attribute);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
index e3a390e958..73d786bf5a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
@@ -25,6 +25,7 @@ import com.google.devtools.build.lib.packages.Attribute.ComputationLimiter;
 import com.google.devtools.build.lib.packages.BuildType.Selector;
 import com.google.devtools.build.lib.packages.BuildType.SelectorList;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.syntax.Type.LabelVisitor;
 import com.google.devtools.build.lib.util.Preconditions;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -71,13 +72,13 @@ public class AggregatingAttributeMapper extends AbstractAttributeMapper {
    * path whenever actual value iteration isn't specifically needed.
    */
   @Override
-  protected void visitLabels(Attribute attribute, AcceptsLabelAttribute observer)
+  protected void visitLabels(Attribute attribute, Type.LabelVisitor<Attribute> visitor)
       throws InterruptedException {
-    visitLabels(attribute, true, observer);
+    visitLabels(attribute, true, visitor);
   }
 
   private void visitLabels(
-      final Attribute attribute, boolean includeSelectKeys, final AcceptsLabelAttribute observer)
+      Attribute attribute, boolean includeSelectKeys, Type.LabelVisitor<Attribute> visitor)
       throws InterruptedException {
     Type<?> type = attribute.getType();
     SelectorList<?> selectorList = getSelectorList(attribute.getName(), type);
@@ -87,39 +88,22 @@ public class AggregatingAttributeMapper extends AbstractAttributeMapper {
         // (computed) values and look for labels.
         for (Object value : visitAttribute(attribute.getName(), attribute.getType())) {
           if (value != null) {
-            type.visitLabels(new Type.LabelVisitor() {
-              @Override
-              public void visit(@Nullable Label label) throws InterruptedException {
-                if (label != null) {
-                  observer.acceptLabelAttribute(
-                      getLabel().resolveRepositoryRelative(label), attribute);
-                }
-              }
-            }, value);
+            type.visitLabels(visitor, value, attribute);
           }
         }
       } else {
-        super.visitLabels(attribute, observer);
+        super.visitLabels(attribute, visitor);
       }
     } else {
       for (Selector<?> selector : selectorList.getSelectors()) {
         for (Map.Entry<Label, ?> selectorEntry : selector.getEntries().entrySet()) {
           if (includeSelectKeys && !BuildType.Selector.isReservedLabel(selectorEntry.getKey())) {
-            observer.acceptLabelAttribute(
-                getLabel().resolveRepositoryRelative(selectorEntry.getKey()), attribute);
+            visitor.visit(selectorEntry.getKey(), attribute);
           }
           Object value = selector.isValueSet(selectorEntry.getKey())
               ? selectorEntry.getValue()
               : attribute.getDefaultValue(null);
-          type.visitLabels(new Type.LabelVisitor() {
-            @Override
-            public void visit(@Nullable Label label) throws InterruptedException {
-              if (label != null) {
-                observer.acceptLabelAttribute(
-                    getLabel().resolveRepositoryRelative(label), attribute);
-              }
-            }
-          }, value);
+          type.visitLabels(visitor, value, attribute);
         }
       }
     }
@@ -133,10 +117,12 @@ public class AggregatingAttributeMapper extends AbstractAttributeMapper {
   public Set<Label> getReachableLabels(String attributeName, boolean includeSelectKeys)
       throws InterruptedException {
     final ImmutableSet.Builder<Label> builder = ImmutableSet.<Label>builder();
-    visitLabels(getAttributeDefinition(attributeName), includeSelectKeys,
-        new AcceptsLabelAttribute() {
+    visitLabels(
+        getAttributeDefinition(attributeName),
+        includeSelectKeys,
+        new LabelVisitor<Attribute>() {
           @Override
-          public void acceptLabelAttribute(Label label, Attribute attribute) {
+          public void visit(Label label, Attribute attribute) {
             builder.add(label);
           }
         });
@@ -538,14 +524,17 @@ public class AggregatingAttributeMapper extends AbstractAttributeMapper {
   private static ImmutableList<Label> extractLabels(Type<?> type, Object value) {
     try {
       final ImmutableList.Builder<Label> result = ImmutableList.builder();
-      type.visitLabels(new Type.LabelVisitor() {
-        @Override
-        public void visit(@Nullable Label label) {
-          if (label != null) {
-            result.add(label);
-          }
-        }
-      }, value);
+      type.visitLabels(
+          new Type.LabelVisitor<Object>() {
+            @Override
+            public void visit(@Nullable Label label, Object dummy) {
+              if (label != null) {
+                result.add(label);
+              }
+            }
+          },
+          value,
+          /*context=*/ null);
       return result.build();
     } catch (InterruptedException e) {
       throw new IllegalStateException("Unexpected InterruptedException", e);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
index 8e2ae52fcb..118b2be6e1 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
@@ -195,28 +195,27 @@ public final class AspectDefinition {
       final Multimap<Attribute, Label> labelBuilder,
       Aspect aspect,
       DependencyFilter dependencyFilter) {
+    LabelVisitor<Attribute> labelVisitor = new LabelVisitor<Attribute>() {
+      @Override
+      public void visit(Label label, Attribute aspectAttribute) {
+        Label repositoryRelative = maybeGetRepositoryRelativeLabel(from, label);
+        if (repositoryRelative == null) {
+          return;
+        }
+        labelBuilder.put(aspectAttribute, repositoryRelative);
+      }
+    };
     ImmutableMap<String, Attribute> attributes = aspect.getDefinition().getAttributes();
     for (final Attribute aspectAttribute : attributes.values()) {
       if (!dependencyFilter.apply(aspect, aspectAttribute)) {
         continue;
       }
-      Type type = aspectAttribute.getType();
+      Type<?> type = aspectAttribute.getType();
       if (type.getLabelClass() != LabelClass.DEPENDENCY) {
         continue;
       }
       try {
-        type.visitLabels(
-            new LabelVisitor() {
-              @Override
-              public void visit(Label label) {
-                Label repositoryRelative = maybeGetRepositoryRelativeLabel(from, label);
-                if (repositoryRelative == null) {
-                  return;
-                }
-                labelBuilder.put(aspectAttribute, repositoryRelative);
-              }
-            },
-            aspectAttribute.getDefaultValue(from));
+        type.visitLabels(labelVisitor, aspectAttribute.getDefaultValue(from), aspectAttribute);
       } catch (InterruptedException ex) {
         // Because the LabelVisitor does not throw InterruptedException, it should not be thrown
         // by visitLabels here.
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 263fe63e79..5df9d83625 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
@@ -30,6 +30,7 @@ import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
 import com.google.devtools.build.lib.syntax.Type.DictType;
 import com.google.devtools.build.lib.syntax.Type.LabelClass;
+import com.google.devtools.build.lib.syntax.Type.LabelVisitor;
 import com.google.devtools.build.lib.syntax.Type.ListType;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -103,7 +104,7 @@ public final class BuildType {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context) {
     }
 
     @Override
@@ -200,9 +201,10 @@ public final class BuildType {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) throws InterruptedException {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context)
+        throws InterruptedException {
       for (Label label : cast(value).getLabels()) {
-        visitor.visit(label);
+        visitor.visit(label, context);
       }
     }
   }
@@ -225,8 +227,9 @@ public final class BuildType {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) throws InterruptedException {
-      visitor.visit(cast(value));
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context)
+        throws InterruptedException {
+      visitor.visit(cast(value), context);
     }
 
     @Override
@@ -345,7 +348,7 @@ public final class BuildType {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context) {
     }
 
     @Override
@@ -384,7 +387,7 @@ public final class BuildType {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context) {
     }
 
     @Override
@@ -410,8 +413,9 @@ public final class BuildType {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) throws InterruptedException {
-      visitor.visit(cast(value));
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context)
+        throws InterruptedException {
+      visitor.visit(cast(value), context);
     }
 
     @Override
@@ -676,7 +680,7 @@ public final class BuildType {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context) {
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Type.java b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
index 6a0a76b4bf..c83a72bd09 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Type.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
@@ -124,13 +124,17 @@ public abstract class Type<T> {
    */
   public abstract T getDefaultValue();
 
-  /** Function accepting a (potentially null) object value. See {@link #visitLabels}. */
-  public static interface LabelVisitor {
-    void visit(@Nullable Label label) throws InterruptedException;
+  /**
+   * Function accepting a (potentially null) {@link Label} and an arbitrary context object. Used by
+   * {@link #visitLabels}.
+   */
+  public static interface LabelVisitor<C> {
+    void visit(@Nullable Label label, @Nullable C context) throws InterruptedException;
   }
 
   /**
-   * Extracts all labels associated with the instance of the type to visitor.
+   * Invokes {@code visitor.visit(label, context)} for each {@link Label} {@code label} associated
+   * with {@code value}, which is assumed an instance of this {@link Type}.
    *
    * <p>This is used to support reliable label visitation in
    * {@link com.google.devtools.build.lib.packages.AbstractAttributeMapper#visitLabels}. To preserve
@@ -138,7 +142,8 @@ public abstract class Type<T> {
    * words, be careful about defining default instances in base types that get auto-inherited by
    * their children. Keep all definitions as explicit as possible.
    */
-  public abstract void visitLabels(LabelVisitor visitor, Object value) throws InterruptedException;
+  public abstract <C> void visitLabels(LabelVisitor<C> visitor, Object value, @Nullable C context)
+      throws InterruptedException;
 
   /** Classifications of labels by their usage. */
   public enum LabelClass {
@@ -283,7 +288,7 @@ public abstract class Type<T> {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context) {
     }
 
     @Override
@@ -309,7 +314,7 @@ public abstract class Type<T> {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context) {
     }
 
     @Override
@@ -348,7 +353,7 @@ public abstract class Type<T> {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context) {
     }
 
     @Override
@@ -398,7 +403,7 @@ public abstract class Type<T> {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context) {
     }
 
     @Override
@@ -446,10 +451,11 @@ public abstract class Type<T> {
     private final LabelClass labelClass;
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) throws InterruptedException {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context)
+        throws InterruptedException {
       for (Entry<KeyT, ValueT> entry : cast(value).entrySet()) {
-        keyType.visitLabels(visitor, entry.getKey());
-        valueType.visitLabels(visitor, entry.getValue());
+        keyType.visitLabels(visitor, entry.getKey(), context);
+        valueType.visitLabels(visitor, entry.getValue(), context);
       }
     }
 
@@ -559,9 +565,10 @@ public abstract class Type<T> {
     }
 
     @Override
-    public void visitLabels(LabelVisitor visitor, Object value) throws InterruptedException {
+    public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context)
+        throws InterruptedException {
       for (ElemT elem : cast(value)) {
-        elemType.visitLabels(visitor, elem);
+        elemType.visitLabels(visitor, elem, context);
       }
     }
 
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 c5b0d31544..63c15d9ef4 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
@@ -592,13 +592,16 @@ public class BuildTypeTest {
   private static ImmutableList<Label> collectLabels(Type<?> type, Object value)
       throws InterruptedException {
     final ImmutableList.Builder<Label> result = ImmutableList.builder();
-    type.visitLabels(new Type.LabelVisitor() {
-      @SuppressWarnings("unchecked")
-      @Override
-      public void visit(Label label) throws InterruptedException {
-        result.add(label);
-      }
-    }, value);
+    type.visitLabels(
+        new Type.LabelVisitor<Object>() {
+          @SuppressWarnings("unchecked")
+          @Override
+          public void visit(Label label, Object dummy) throws InterruptedException {
+            result.add(label);
+          }
+        },
+        value,
+        /*context=*/ null);
     return result.build();
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java b/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
index 4efe9d0c9c..43cb4d47df 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
@@ -450,13 +450,16 @@ public class TypeTest {
   private static ImmutableList<Label> collectLabels(Type<?> type, Object value)
       throws InterruptedException {
     final ImmutableList.Builder<Label> result = ImmutableList.builder();
-    type.visitLabels(new Type.LabelVisitor() {
-      @SuppressWarnings("unchecked")
-      @Override
-      public void visit(Label label) throws InterruptedException {
-        result.add(label);
-      }
-    }, value);
+    type.visitLabels(
+        new Type.LabelVisitor<Object>() {
+          @SuppressWarnings("unchecked")
+          @Override
+          public void visit(Label label, Object dummy) throws InterruptedException {
+            result.add(label);
+          }
+         },
+        value,
+        /*context=*/ null);
     return result.build();
   }
 }