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

1 files changed, 18 insertions, 21 deletions

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) {