Move caching of OptionDefinitions to be static, and remove uncached extractions of OptionDefinitions.

We already had caching of OptionsData objects, for a list of OptionsBases, but repeated the reflective work for the same OptionsBase if it appeared in different lists. Now that the @Option-annotation specific state is isolated to the OptionDefinition object, this can be trivially cached by OptionsBase.

There are a few additional convenient side effects to this change. This should slightly decrease the memory use of the OptionsParser, since it already cached this map per options-base, and now only requires a single copy. It also means that parts of the code base that needed details of an option's definition no longer need to either obtain an option definition themselves or need access to an OptionsData object, which should be private to the OptionsParser anyway.

PiperOrigin-RevId: 167158902

7 files changed, 74 insertions, 95 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
index 6aa27b22d2..f1a2329403 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
@@ -17,11 +17,10 @@ package com.google.devtools.build.lib.analysis.config;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.common.options.OptionDefinition;
-import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
 import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsBase;
+import com.google.devtools.common.options.OptionsParser;
 import java.io.Serializable;
-import java.lang.reflect.Field;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -43,20 +42,15 @@ public final class TransitiveOptionDetails implements Serializable {
     ImmutableMap.Builder<String, OptionDetails> map = ImmutableMap.builder();
     try {
       for (OptionsBase options : buildOptions) {
-        for (Field field : options.getClass().getFields()) {
-          OptionDefinition optionDefinition;
-          try {
-            optionDefinition = OptionDefinition.extractOptionDefinition(field);
-          } catch (NotAnOptionException e) {
-            // Skip non @Option fields.
-            continue;
-          }
+        ImmutableList<OptionDefinition> optionDefinitions =
+            OptionsParser.getOptionDefinitions(options.getClass());
+        for (OptionDefinition optionDefinition : optionDefinitions) {
           if (ImmutableList.copyOf(optionDefinition.getOptionMetadataTags())
               .contains(OptionMetadataTag.INTERNAL)) {
               // ignore internal options
               continue;
             }
-            Object value = field.get(options);
+          Object value = optionDefinition.getField().get(options);
             if (value == null) {
             if (lateBoundDefaults.containsKey(optionDefinition.getOptionName())) {
               value = lateBoundDefaults.get(optionDefinition.getOptionName());
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
index a1602342e8..cbd0a4e9d6 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -76,7 +76,6 @@ import com.google.devtools.build.lib.windows.WindowsSubprocessFactory;
 import com.google.devtools.common.options.CommandNameCache;
 import com.google.devtools.common.options.InvocationPolicyParser;
 import com.google.devtools.common.options.OptionDefinition;
-import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
 import com.google.devtools.common.options.OptionPriority;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsClassProvider;
@@ -87,7 +86,7 @@ import com.google.devtools.common.options.TriState;
 import java.io.BufferedOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
-import java.lang.reflect.Field;
+import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Date;
@@ -646,23 +645,17 @@ public final class BlazeRuntime {
   static CommandLineOptions splitStartupOptions(
       Iterable<BlazeModule> modules, String... args) {
     List<String> prefixes = new ArrayList<>();
-    List<Field> startupFields = Lists.newArrayList();
+    List<OptionDefinition> startupOptions = Lists.newArrayList();
     for (Class<? extends OptionsBase> defaultOptions
       : BlazeCommandUtils.getStartupOptions(modules)) {
-      startupFields.addAll(ImmutableList.copyOf(defaultOptions.getFields()));
+      startupOptions.addAll(OptionsParser.getOptionDefinitions(defaultOptions));
     }
 
-    for (Field field : startupFields) {
-      try {
-        OptionDefinition optionDefinition = OptionDefinition.extractOptionDefinition(field);
-        prefixes.add("--" + optionDefinition.getOptionName());
-        if (field.getType() == boolean.class || field.getType() == TriState.class) {
-          prefixes.add("--no" + optionDefinition.getOptionName());
-        }
-      } catch (NotAnOptionException e) {
-        // Do nothing, just ignore fields that are not actually options. OptionsBases technically
-        // shouldn't have fields that are not @Options, but this is a requirement that isn't yet
-        // being enforced, so this should not cause a failure here.
+    for (OptionDefinition optionDefinition : startupOptions) {
+      Type optionType = optionDefinition.getField().getType();
+      prefixes.add("--" + optionDefinition.getOptionName());
+      if (optionType == boolean.class || optionType == TriState.class) {
+        prefixes.add("--no" + optionDefinition.getOptionName());
       }
     }
 
diff --git a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
index 754ca2caa0..34208d01a5 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -46,6 +46,40 @@ import javax.annotation.concurrent.Immutable;
 public class IsolatedOptionsData extends OpaqueOptionsData {
 
   /**
+   * Cache for the options in an OptionsBase.
+   *
+   * <p>Mapping from options class to a list of all {@code OptionFields} in that class. The map
+   * entries are unordered, but the fields in the lists are ordered alphabetically. This caches the
+   * work of reflection done for the same {@code optionsBase} across multiple {@link OptionsData}
+   * instances, and must be used through the thread safe {@link
+   * #getAllOptionDefinitionsForClass(Class)}
+   */
+  private static final Map<Class<? extends OptionsBase>, ImmutableList<OptionDefinition>>
+      allOptionsFields = new HashMap<>();
+
+  /** Returns all {@code optionDefinitions}, ordered by their option name (not their field name). */
+  public static synchronized ImmutableList<OptionDefinition> getAllOptionDefinitionsForClass(
+      Class<? extends OptionsBase> optionsClass) {
+    return allOptionsFields.computeIfAbsent(
+        optionsClass,
+        optionsBaseClass ->
+            Arrays.stream(optionsBaseClass.getFields())
+                .map(
+                    field -> {
+                      try {
+                        return OptionDefinition.extractOptionDefinition(field);
+                      } catch (NotAnOptionException e) {
+                        // Ignore non-@Option annotated fields. Requiring all fields in the
+                        // OptionsBase to be @Option-annotated requires a depot cleanup.
+                        return null;
+                      }
+                    })
+                .filter(Objects::nonNull)
+                .sorted(OptionDefinition.BY_OPTION_NAME)
+                .collect(ImmutableList.toImmutableList()));
+  }
+
+  /**
    * Mapping from each options class to its no-arg constructor. Entries appear in the same order
    * that they were passed to {@link #from(Collection)}.
    */
@@ -61,12 +95,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
   /** Mapping from option abbreviation to {@code OptionDefinition} (unordered). */
   private final ImmutableMap<Character, OptionDefinition> abbrevToField;
 
-  /**
-   * Mapping from options class to a list of all {@code OptionFields} in that class. The map entries
-   * are unordered, but the fields in the lists are ordered alphabetically.
-   */
-  private final ImmutableMap<Class<? extends OptionsBase>, ImmutableList<OptionDefinition>>
-      allOptionsFields;
 
   /**
    * Mapping from each options class to whether or not it has the {@link UsesOnlyCoreTypes}
@@ -82,12 +110,10 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
       Map<Class<? extends OptionsBase>, Constructor<?>> optionsClasses,
       Map<String, OptionDefinition> nameToField,
       Map<Character, OptionDefinition> abbrevToField,
-      Map<Class<? extends OptionsBase>, ImmutableList<OptionDefinition>> allOptionsFields,
       Map<Class<? extends OptionsBase>, Boolean> usesOnlyCoreTypes) {
     this.optionsClasses = ImmutableMap.copyOf(optionsClasses);
     this.nameToField = ImmutableMap.copyOf(nameToField);
     this.abbrevToField = ImmutableMap.copyOf(abbrevToField);
-    this.allOptionsFields = ImmutableMap.copyOf(allOptionsFields);
     this.usesOnlyCoreTypes = ImmutableMap.copyOf(usesOnlyCoreTypes);
   }
 
@@ -96,7 +122,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
         other.optionsClasses,
         other.nameToField,
         other.abbrevToField,
-        other.allOptionsFields,
         other.usesOnlyCoreTypes);
   }
 
@@ -130,38 +155,9 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     return abbrevToField.get(abbrev);
   }
 
-  /**
-   * Returns a list of all {@link Field} objects for options in the given options class, ordered
-   * alphabetically by option name.
-   */
-  public ImmutableList<OptionDefinition> getOptionDefinitionsFromClass(
-      Class<? extends OptionsBase> optionsClass) {
-    return allOptionsFields.get(optionsClass);
-  }
-
   public boolean getUsesOnlyCoreTypes(Class<? extends OptionsBase> optionsClass) {
     return usesOnlyCoreTypes.get(optionsClass);
   }
-
-  /** Returns all {@code optionDefinitions}, ordered by their option name (not their field name). */
-  private static ImmutableList<OptionDefinition> getAllOptionDefinitionsSorted(
-      Class<? extends OptionsBase> optionsClass) {
-    return Arrays.stream(optionsClass.getFields())
-        .map(
-            field -> {
-              try {
-                return OptionDefinition.extractOptionDefinition(field);
-              } catch (NotAnOptionException e) {
-                // Ignore non-@Option annotated fields. Requiring all fields in the OptionsBase to
-                // be @Option-annotated requires a depot cleanup.
-                return null;
-              }
-            })
-        .filter(Objects::nonNull)
-        .sorted(OptionDefinition.BY_OPTION_NAME)
-        .collect(ImmutableList.toImmutableList());
-  }
-
   private static <A> void checkForCollisions(
       Map<A, OptionDefinition> aFieldMap, A optionName, String description) {
     if (aFieldMap.containsKey(optionName)) {
@@ -204,8 +200,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
   static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes) {
     // Mind which fields have to preserve order.
     Map<Class<? extends OptionsBase>, Constructor<?>> constructorBuilder = new LinkedHashMap<>();
-    Map<Class<? extends OptionsBase>, ImmutableList<OptionDefinition>> allOptionsFieldsBuilder =
-        new HashMap<>();
     Map<String, OptionDefinition> nameToFieldBuilder = new LinkedHashMap<>();
     Map<Character, OptionDefinition> abbrevToFieldBuilder = new HashMap<>();
 
@@ -225,8 +219,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
             + " lacks an accessible default constructor");
       }
       ImmutableList<OptionDefinition> optionDefinitions =
-          getAllOptionDefinitionsSorted(parsedOptionsClass);
-      allOptionsFieldsBuilder.put(parsedOptionsClass, optionDefinitions);
+          getAllOptionDefinitionsForClass(parsedOptionsClass);
 
       for (OptionDefinition optionDefinition : optionDefinitions) {
         String optionName = optionDefinition.getOptionName();
@@ -297,7 +290,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
         constructorBuilder,
         nameToFieldBuilder,
         abbrevToFieldBuilder,
-        allOptionsFieldsBuilder,
         usesOnlyCoreTypesBuilder);
   }
 
diff --git a/src/main/java/com/google/devtools/common/options/OptionDefinition.java b/src/main/java/com/google/devtools/common/options/OptionDefinition.java
index 1b55a8cef2..08962bba0d 100644
--- a/src/main/java/com/google/devtools/common/options/OptionDefinition.java
+++ b/src/main/java/com/google/devtools/common/options/OptionDefinition.java
@@ -32,8 +32,8 @@ import java.util.Comparator;
 public class OptionDefinition {
 
   // TODO(b/65049598) make ConstructionException checked, which will make this checked as well.
-  public static class NotAnOptionException extends ConstructionException {
-    public NotAnOptionException(Field field) {
+  static class NotAnOptionException extends ConstructionException {
+    NotAnOptionException(Field field) {
       super(
           "The field "
               + field.getName()
@@ -44,8 +44,11 @@ public class OptionDefinition {
   /**
    * If the {@code field} is annotated with the appropriate @{@link Option} annotation, returns the
    * {@code OptionDefinition} for that option. Otherwise, throws a {@link NotAnOptionException}.
+   *
+   * <p>These values are cached in the {@link OptionsData} layer and should be accessed through
+   * {@link OptionsParser#getOptionDefinitions(Class)}.
    */
-  public static OptionDefinition extractOptionDefinition(Field field) {
+  static OptionDefinition extractOptionDefinition(Field field) {
     Option annotation = field == null ? null : field.getAnnotation(Option.class);
     if (annotation == null) {
       throw new NotAnOptionException(field);
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index 9d683208d1..618e0ddafa 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -26,7 +26,6 @@ import java.lang.reflect.Field;
 import java.nio.file.FileSystem;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
@@ -519,7 +518,7 @@ public class OptionsParser implements OptionsProvider {
     if (!data.getOptionsClasses().isEmpty()) {
       List<OptionDefinition> allFields = new ArrayList<>();
       for (Class<? extends OptionsBase> optionsClass : data.getOptionsClasses()) {
-        allFields.addAll(data.getOptionDefinitionsFromClass(optionsClass));
+        allFields.addAll(OptionsData.getAllOptionDefinitionsForClass(optionsClass));
       }
       Collections.sort(allFields, OptionDefinition.BY_CATEGORY);
       String prevCategory = null;
@@ -563,7 +562,7 @@ public class OptionsParser implements OptionsProvider {
     if (!data.getOptionsClasses().isEmpty()) {
       List<OptionDefinition> allFields = new ArrayList<>();
       for (Class<? extends OptionsBase> optionsClass : data.getOptionsClasses()) {
-        allFields.addAll(data.getOptionDefinitionsFromClass(optionsClass));
+        allFields.addAll(OptionsData.getAllOptionDefinitionsForClass(optionsClass));
       }
       Collections.sort(allFields, OptionDefinition.BY_CATEGORY);
       String prevCategory = null;
@@ -620,7 +619,7 @@ public class OptionsParser implements OptionsProvider {
     data.getOptionsClasses()
         // List all options
         .stream()
-        .flatMap(optionsClass -> data.getOptionDefinitionsFromClass(optionsClass).stream())
+        .flatMap(optionsClass -> OptionsData.getAllOptionDefinitionsForClass(optionsClass).stream())
         // Sort field for deterministic ordering
         .sorted(OptionDefinition.BY_OPTION_NAME)
         .filter(predicate)
@@ -772,9 +771,9 @@ public class OptionsParser implements OptionsProvider {
   }
 
   /** Returns all options fields of the given options class, in alphabetic order. */
-  public static Collection<OptionDefinition> getFields(Class<? extends OptionsBase> optionsClass) {
-    OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
-    return data.getOptionDefinitionsFromClass(optionsClass);
+  public static ImmutableList<OptionDefinition> getOptionDefinitions(
+      Class<? extends OptionsBase> optionsClass) {
+    return OptionsData.getAllOptionDefinitionsForClass(optionsClass);
   }
 
   /**
@@ -803,10 +802,10 @@ public class OptionsParser implements OptionsProvider {
    * @throws IllegalArgumentException if {@code options} is not an instance of {@link OptionsBase}
    */
   public static <O extends OptionsBase> Map<Field, Object> toMap(Class<O> optionsClass, O options) {
-    OptionsData data = getOptionsDataInternal(optionsClass);
-    // Alphabetized due to getOptionDefinitionsFromClass()'s order.
+    // Alphabetized due to getAllOptionDefinitionsForClass()'s order.
     Map<Field, Object> map = new LinkedHashMap<>();
-    for (OptionDefinition optionDefinition : data.getOptionDefinitionsFromClass(optionsClass)) {
+    for (OptionDefinition optionDefinition :
+        OptionsData.getAllOptionDefinitionsForClass(optionsClass)) {
       try {
         // Get the object value of the optionDefinition and place in map.
         map.put(optionDefinition.getField(), optionDefinition.getField().get(options));
@@ -843,9 +842,10 @@ public class OptionsParser implements OptionsProvider {
       throw new IllegalStateException("Error while instantiating options class", e);
     }
 
-    List<OptionDefinition> optionDefinitions = data.getOptionDefinitionsFromClass(optionsClass);
+    List<OptionDefinition> optionDefinitions =
+        OptionsData.getAllOptionDefinitionsForClass(optionsClass);
     // Ensure all fields are covered, no extraneous fields.
-    validateFieldsSets(data, optionsClass, new LinkedHashSet<Field>(map.keySet()));
+    validateFieldsSets(optionsClass, new LinkedHashSet<Field>(map.keySet()));
     // Populate the instance.
     for (OptionDefinition optionDefinition : optionDefinitions) {
       // Non-null as per above check.
@@ -868,16 +868,12 @@ public class OptionsParser implements OptionsProvider {
    * Option} annotation.
    */
   private static void validateFieldsSets(
-      OptionsData data,
       Class<? extends OptionsBase> optionsClass,
       LinkedHashSet<Field> fieldsFromMap) {
-    ImmutableList<OptionDefinition> optionFieldsFromClasses =
-        data.getOptionDefinitionsFromClass(optionsClass);
+    ImmutableList<OptionDefinition> optionDefsFromClasses =
+        OptionsData.getAllOptionDefinitionsForClass(optionsClass);
     Set<Field> fieldsFromClass =
-        optionFieldsFromClasses
-            .stream()
-            .map(optionField -> optionField.getField())
-            .collect(Collectors.toSet());
+        optionDefsFromClasses.stream().map(OptionDefinition::getField).collect(Collectors.toSet());
 
     if (fieldsFromClass.equals(fieldsFromMap)) {
       // They are already equal, avoid additional checks.
@@ -886,7 +882,7 @@ public class OptionsParser implements OptionsProvider {
 
     List<String> extraNamesFromClass = new ArrayList<>();
     List<String> extraNamesFromMap = new ArrayList<>();
-    for (OptionDefinition optionDefinition : optionFieldsFromClasses) {
+    for (OptionDefinition optionDefinition : optionDefsFromClasses) {
       if (!fieldsFromMap.contains(optionDefinition.getField())) {
         extraNamesFromClass.add("'" + optionDefinition.getOptionName() + "'");
       }
@@ -897,9 +893,10 @@ public class OptionsParser implements OptionsProvider {
         if (field == null) {
           extraNamesFromMap.add("<null field>");
         } else {
-
           OptionDefinition optionDefinition = null;
           try {
+            // TODO(ccalvarin) This shouldn't be necessary, no option definitions should be found in
+            // this optionsClass that weren't in the cache.
             optionDefinition = OptionDefinition.extractOptionDefinition(field);
             extraNamesFromMap.add("'" + optionDefinition.getOptionName() + "'");
           } catch (NotAnOptionException e) {
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index 28aeb22b70..993fbbe500 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -700,7 +700,7 @@ class OptionsParserImpl {
 
     // Set the fields
     for (OptionDefinition optionDefinition :
-        optionsData.getOptionDefinitionsFromClass(optionsClass)) {
+        OptionsData.getAllOptionDefinitionsForClass(optionsClass)) {
       Object value;
       OptionValueDescription entry = parsedValues.get(optionDefinition);
       if (entry == null) {
diff --git a/src/main/java/com/google/devtools/common/options/OptionsUsage.java b/src/main/java/com/google/devtools/common/options/OptionsUsage.java
index 0ab30da2b5..88da29f6cc 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsUsage.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsUsage.java
@@ -39,7 +39,7 @@ class OptionsUsage {
   static void getUsage(Class<? extends OptionsBase> optionsClass, StringBuilder usage) {
     OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
     List<OptionDefinition> optionDefinitions =
-        new ArrayList<>(data.getOptionDefinitionsFromClass(optionsClass));
+        new ArrayList<>(OptionsData.getAllOptionDefinitionsForClass(optionsClass));
     optionDefinitions.sort(OptionDefinition.BY_OPTION_NAME);
     for (OptionDefinition optionDefinition : optionDefinitions) {
       getUsage(optionDefinition, usage, OptionsParser.HelpVerbosity.LONG, data);