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, 67 insertions, 11 deletions

diff --git a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
index 5a8534f8fd..2f442fd924 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.truth.Correspondence;
 import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.util.ArrayList;
 import java.util.List;
@@ -390,9 +391,10 @@ public class OptionsDataTest {
         "bar", "baz", "foo", "qux", "X", "Y", "A", "B", "C").inOrder();
   }
 
-  private List<String> getOptionNames(Iterable<OptionDefinition> fields) {
+  private List<String> getOptionNames(Class<? extends OptionsBase> optionsBase) {
     ArrayList<String> result = new ArrayList<>();
-    for (OptionDefinition optionDefinition : fields) {
+    for (OptionDefinition optionDefinition :
+        OptionsData.getAllOptionDefinitionsForClass(optionsBase)) {
       result.add(optionDefinition.getOptionName());
     }
     return result;
@@ -400,21 +402,75 @@ public class OptionsDataTest {
 
   @Test
   public void getFieldsForClassIsOrdered() throws Exception {
-    IsolatedOptionsData data = construct(
-        FieldNamesDifferOptions.class,
-        EndOfAlphabetOptions.class,
-        ReverseOrderedOptions.class);
-    assertThat(getOptionNames(data.getOptionDefinitionsFromClass(FieldNamesDifferOptions.class)))
+    assertThat(getOptionNames(FieldNamesDifferOptions.class))
         .containsExactly("bar", "baz", "foo", "qux")
         .inOrder();
-    assertThat(getOptionNames(data.getOptionDefinitionsFromClass(EndOfAlphabetOptions.class)))
-        .containsExactly("X", "Y")
-        .inOrder();
-    assertThat(getOptionNames(data.getOptionDefinitionsFromClass(ReverseOrderedOptions.class)))
+    assertThat(getOptionNames(EndOfAlphabetOptions.class)).containsExactly("X", "Y").inOrder();
+    assertThat(getOptionNames(ReverseOrderedOptions.class))
         .containsExactly("A", "B", "C")
         .inOrder();
   }
 
+  private static class ReferenceEqualityCorrespondence extends Correspondence<Object, Object> {
+
+    @Override
+    public boolean compare(Object obj1, Object obj2) {
+      return obj1 == obj2;
+    }
+
+    @Override
+    public String toString() {
+      return "is the same object as";
+    }
+  }
+
+  @Test
+  public void optionsDefinitionsAreSharedBetweenOptionsBases() throws Exception {
+    Class<FieldNamesDifferOptions> class1 = FieldNamesDifferOptions.class;
+    Class<EndOfAlphabetOptions> class2 = EndOfAlphabetOptions.class;
+    Class<ReverseOrderedOptions> class3 = ReverseOrderedOptions.class;
+
+    // Construct the definitions once and accumulate them so we can test that these are not
+    // recomputed during the construction of the options data.
+    ImmutableList<OptionDefinition> optionDefinitions =
+        new ImmutableList.Builder<OptionDefinition>()
+            .addAll(OptionsData.getAllOptionDefinitionsForClass(class1))
+            .addAll(OptionsData.getAllOptionDefinitionsForClass(class2))
+            .addAll(OptionsData.getAllOptionDefinitionsForClass(class3))
+            .build();
+
+    // Construct the data all together.
+    IsolatedOptionsData data = construct(class1, class2, class3);
+    ArrayList<OptionDefinition> optionDefinitionsFromData =
+        new ArrayList<>(optionDefinitions.size());
+    data.getAllNamedFields().forEach(entry -> optionDefinitionsFromData.add(entry.getValue()));
+
+    ReferenceEqualityCorrespondence referenceEquality = new ReferenceEqualityCorrespondence();
+    assertThat(optionDefinitionsFromData)
+        .comparingElementsUsing(referenceEquality)
+        .containsAllIn(optionDefinitions);
+
+    // Construct options data for each class separately, and check again.
+    IsolatedOptionsData data1 = construct(class1);
+    IsolatedOptionsData data2 = construct(class2);
+    IsolatedOptionsData data3 = construct(class3);
+    ArrayList<OptionDefinition> optionDefinitionsFromGroupedData =
+        new ArrayList<>(optionDefinitions.size());
+    data1
+        .getAllNamedFields()
+        .forEach(entry -> optionDefinitionsFromGroupedData.add(entry.getValue()));
+    data2
+        .getAllNamedFields()
+        .forEach(entry -> optionDefinitionsFromGroupedData.add(entry.getValue()));
+    data3
+        .getAllNamedFields()
+        .forEach(entry -> optionDefinitionsFromGroupedData.add(entry.getValue()));
+
+    assertThat(optionDefinitionsFromGroupedData)
+        .comparingElementsUsing(referenceEquality)
+        .containsAllIn(optionDefinitions);
+  }
+
   /** Dummy options class. */
   public static class ValidExpansionOptions extends OptionsBase {
     @Option(