commit 987f09f0cf3c5bf2fc5157c20fe0f7979978a40b
author ccalvarin <ccalvarin@google.com> Thu Aug 31 19:50:39 2017 +0200
committer Vladimir Moskva <vladmos@google.com> Fri Sep 01 12:27:46 2017 +0200
tree 797b052ac007f948912adef9f3689cc852af7d35
parent ca58a3e431b003bde02be043bfca74226ac4a238

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

src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java

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 a160234..cbd0a4e 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.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 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 @@
   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());
       }
     }