Improve performance of OptionsBase#asMap.
Much of the improvement comes from avoiding calls to OptionDefinition#extractOptionDefinition which is expensive due to its reflective annotation access. The previous methodology for obtaining an option's value was going from OptionsBase -> OptionDefinition (cached) -> Field -> OptionDefinition (uncached) -> Field -> Object. Now it simply goes OptionsBase -> OptionDefinition (cached) -> Field -> Object.
PiperOrigin-RevId: 243304970
diff --git a/src/main/java/com/google/devtools/common/options/OptionsBase.java b/src/main/java/com/google/devtools/common/options/OptionsBase.java
index ce66e66..fd684c6 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsBase.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsBase.java
@@ -14,10 +14,9 @@
package com.google.devtools.common.options;
+import com.google.common.collect.Maps;
import com.google.common.escape.CharEscaperBuilder;
import com.google.common.escape.Escaper;
-import java.lang.reflect.Field;
-import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
@@ -63,23 +62,23 @@
* inherited ones. The mapping is a copy, so subsequent mutations to it or to this object are
* independent. Entries are sorted alphabetically.
*/
- public final <O extends OptionsBase> Map<String, Object> asMap() {
- // Generic O is needed to tell the type system that the toMap() call is safe.
- // The casts are safe because "this" is an instance of "getClass()"
- // which subclasses OptionsBase.
- @SuppressWarnings("unchecked")
- O castThis = (O) this;
- @SuppressWarnings("unchecked")
- Class<O> castClass = (Class<O>) getClass();
-
- Map<String, Object> map = new LinkedHashMap<>();
- for (Map.Entry<Field, Object> entry : OptionsParser.toMap(castClass, castThis).entrySet()) {
- OptionDefinition optionDefinition = OptionDefinition.extractOptionDefinition(entry.getKey());
- map.put(optionDefinition.getOptionName(), entry.getValue());
+ public final Map<String, Object> asMap() {
+ List<OptionDefinition> definitions = OptionsData.getAllOptionDefinitionsForClass(getClass());
+ Map<String, Object> map = Maps.newLinkedHashMapWithExpectedSize(definitions.size());
+ for (OptionDefinition definition : definitions) {
+ map.put(definition.getOptionName(), getValueFromDefinition(definition));
}
return map;
}
+ private Object getValueFromDefinition(OptionDefinition definition) {
+ try {
+ return definition.getField().get(this);
+ } catch (IllegalAccessException e) {
+ throw new IllegalStateException("All options fields of options classes should be public", e);
+ }
+ }
+
@Override
public final String toString() {
return getClass().getName() + asMap();
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 78f37e0..4f26562 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -23,19 +23,14 @@
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MoreCollectors;
import com.google.common.escape.Escaper;
-import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
import com.google.devtools.common.options.OptionsParserImpl.ResidueAndPriority;
-import java.lang.reflect.Constructor;
-import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;
@@ -796,132 +791,5 @@
OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
return data.getUsesOnlyCoreTypes(optionsClass);
}
-
- /**
- * Returns a mapping from each option {@link Field} in {@code optionsClass} (including inherited
- * ones) to its value in {@code options}.
- *
- * <p>To save space, the map directly stores {@code Fields} instead of the {@code
- * OptionDefinitions}.
- *
- * <p>The map is a mutable copy; changing the map won't affect {@code options} and vice versa. The
- * map entries appear sorted alphabetically by option name.
- *
- * <p>If {@code options} is an instance of a subclass of {@link OptionsBase}, any options defined
- * by the subclass are not included in the map, only the options declared in the provided class
- * are included.
- *
- * @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) {
- // Alphabetized due to getAllOptionDefinitionsForClass()'s order.
- Map<Field, Object> map = new LinkedHashMap<>();
- 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));
- } catch (IllegalAccessException e) {
- // All options fields of options classes should be public.
- throw new IllegalStateException(e);
- } catch (IllegalArgumentException e) {
- // This would indicate an inconsistency in the cached OptionsData.
- throw new IllegalStateException(e);
- }
- }
- return map;
- }
-
- /**
- * Given a mapping as returned by {@link #toMap}, and the options class it that its entries
- * correspond to, this constructs the corresponding instance of the options class.
- *
- * @param map Field to Object, expecting an entry for each field in the optionsClass. This
- * directly refers to the Field, without wrapping it in an OptionDefinition, see {@link
- * #toMap}.
- * @throws IllegalArgumentException if {@code map} does not contain exactly the fields of {@code
- * optionsClass}, with values of the appropriate type
- */
- public static <O extends OptionsBase> O fromMap(Class<O> optionsClass, Map<Field, Object> map) {
- // Instantiate the options class.
- OptionsData data = getOptionsDataInternal(optionsClass);
- O optionsInstance;
- try {
- Constructor<O> constructor = data.getConstructor(optionsClass);
- Preconditions.checkNotNull(constructor, "No options class constructor available");
- optionsInstance = constructor.newInstance();
- } catch (ReflectiveOperationException e) {
- throw new IllegalStateException("Error while instantiating options class", e);
- }
-
- List<OptionDefinition> optionDefinitions =
- OptionsData.getAllOptionDefinitionsForClass(optionsClass);
- // Ensure all fields are covered, no extraneous fields.
- validateFieldsSets(optionsClass, new LinkedHashSet<Field>(map.keySet()));
- // Populate the instance.
- for (OptionDefinition optionDefinition : optionDefinitions) {
- // Non-null as per above check.
- Object value = map.get(optionDefinition.getField());
- try {
- optionDefinition.getField().set(optionsInstance, value);
- } catch (IllegalAccessException e) {
- throw new IllegalStateException(e);
- }
- // May also throw IllegalArgumentException if map value is ill typed.
- }
- return optionsInstance;
- }
-
- /**
- * Raises a pretty {@link IllegalArgumentException} if the provided set of fields is a complete
- * set for the optionsClass.
- *
- * <p>The entries in {@code fieldsFromMap} may be ill formed by being null or lacking an {@link
- * Option} annotation.
- */
- private static void validateFieldsSets(
- Class<? extends OptionsBase> optionsClass, LinkedHashSet<Field> fieldsFromMap) {
- ImmutableList<OptionDefinition> optionDefsFromClasses =
- OptionsData.getAllOptionDefinitionsForClass(optionsClass);
- Set<Field> fieldsFromClass =
- optionDefsFromClasses.stream().map(OptionDefinition::getField).collect(Collectors.toSet());
-
- if (fieldsFromClass.equals(fieldsFromMap)) {
- // They are already equal, avoid additional checks.
- return;
- }
-
- List<String> extraNamesFromClass = new ArrayList<>();
- List<String> extraNamesFromMap = new ArrayList<>();
- for (OptionDefinition optionDefinition : optionDefsFromClasses) {
- if (!fieldsFromMap.contains(optionDefinition.getField())) {
- extraNamesFromClass.add("'" + optionDefinition.getOptionName() + "'");
- }
- }
- for (Field field : fieldsFromMap) {
- // Extra validation on the map keys since they don't come from OptionsData.
- if (!fieldsFromClass.contains(field)) {
- 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) {
- extraNamesFromMap.add("<non-Option field>");
- }
- }
- }
- }
- throw new IllegalArgumentException(
- "Map keys do not match fields of options class; extra map keys: {"
- + Joiner.on(", ").join(extraNamesFromMap)
- + "}; extra options class options: {"
- + Joiner.on(", ").join(extraNamesFromClass)
- + "}");
- }
}
diff --git a/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java b/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java
index b96e57a..9065912 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java
@@ -15,12 +15,7 @@
package com.google.devtools.common.options;
import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
-import com.google.common.collect.ImmutableMap;
-import java.lang.reflect.Field;
-import java.util.LinkedHashMap;
-import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -29,90 +24,44 @@
@RunWith(JUnit4.class)
public class OptionsMapConversionTest {
- private static Map<String, Object> keysToStrings(Map<Field, Object> map) {
- Map<String, Object> result = new LinkedHashMap<>();
- for (Map.Entry<Field, Object> entry : map.entrySet()) {
- OptionDefinition optionDefinition = OptionDefinition.extractOptionDefinition(entry.getKey());
- result.put(optionDefinition.getOptionName(), entry.getValue());
- }
- return result;
- }
-
- private static Map<Field, Object> keysToFields(
- Class<? extends OptionsBase> optionsClass, Map<String, Object> map) {
- OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
- Map<Field, Object> result = new LinkedHashMap<>();
- for (Map.Entry<String, Object> entry : map.entrySet()) {
- OptionDefinition optionDefinition = data.getOptionDefinitionFromName(entry.getKey());
- result.put(optionDefinition.getField(), entry.getValue());
- }
- return result;
- }
-
/** Dummy options base class. */
public static class FooOptions extends OptionsBase {
@Option(
- name = "foo",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "false"
- )
+ name = "foo",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "false")
public boolean foo;
}
/** Dummy options derived class. */
public static class BazOptions extends FooOptions {
@Option(
- name = "bar",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "true"
- )
+ name = "bar",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "true")
public boolean bar;
@Option(
- name = "baz",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "5"
- )
+ name = "baz",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "5")
public int baz;
}
@Test
- public void toMap_Basic() {
- FooOptions foo = Options.getDefaults(FooOptions.class);
- assertThat(keysToStrings(OptionsParser.toMap(FooOptions.class, foo)))
- .containsExactly("foo", false);
- }
-
- @Test
public void asMap_Basic() {
FooOptions foo = Options.getDefaults(FooOptions.class);
- assertThat(foo.asMap())
- .containsExactly("foo", false);
- }
-
- @Test
- public void toMap_Inheritance() {
- BazOptions baz = Options.getDefaults(BazOptions.class);
- assertThat(keysToStrings(OptionsParser.toMap(BazOptions.class, baz)))
- .containsExactly("foo", false, "bar", true, "baz", 5);
+ assertThat(foo.asMap()).containsExactly("foo", false);
}
@Test
public void asMap_Inheritance() {
// Static type is base class, dynamic type is derived. We still get the derived fields.
FooOptions foo = Options.getDefaults(BazOptions.class);
- assertThat(foo.asMap())
- .containsExactly("foo", false, "bar", true, "baz", 5);
- }
-
- @Test
- public void toMap_InheritanceBaseFieldsOnly() {
- BazOptions baz = Options.getDefaults(BazOptions.class);
- assertThat(keysToStrings(OptionsParser.toMap(FooOptions.class, baz)))
- .containsExactly("foo", false);
+ assertThat(foo.asMap()).containsExactly("foo", false, "bar", true, "baz", 5);
}
/**
@@ -123,135 +72,44 @@
public static class AlphaOptions extends OptionsBase {
@Option(
- name = "c",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "0"
- )
+ name = "c",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "0")
public int v;
@Option(
- name = "d",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "0"
- )
+ name = "d",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "0")
public int w;
@Option(
- name = "a",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "0"
- )
+ name = "a",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "0")
public int x;
@Option(
- name = "e",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "0"
- )
+ name = "e",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "0")
public int y;
@Option(
- name = "b",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "0"
- )
+ name = "b",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "0")
public int z;
}
@Test
- public void toMap_AlphabeticalOrder() {
- AlphaOptions alpha = Options.getDefaults(AlphaOptions.class);
- assertThat(keysToStrings(OptionsParser.toMap(AlphaOptions.class, alpha)))
- .containsExactly("a", 0, "b", 0, "c", 0, "d", 0, "e", 0).inOrder();
- }
-
- @Test
public void asMap_AlphabeticalOrder() {
AlphaOptions alpha = Options.getDefaults(AlphaOptions.class);
- assertThat(alpha.asMap())
- .containsExactly("a", 0, "b", 0, "c", 0, "d", 0, "e", 0).inOrder();
- }
-
- @Test
- public void fromMap_Basic() {
- Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true);
- Map<Field, Object> fieldMap = keysToFields(FooOptions.class, map);
- FooOptions foo = OptionsParser.fromMap(FooOptions.class, fieldMap);
- assertThat(foo.foo).isTrue();
- }
-
- /** Dummy subclass of foo. */
- public static class SubFooAOptions extends FooOptions {
- @Option(
- name = "a",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "false"
- )
- public boolean a;
- }
-
- /** Dummy subclass of foo. */
- public static class SubFooBOptions extends FooOptions {
- @Option(
- name = "b1",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "false"
- )
- public boolean b1;
-
- @Option(
- name = "b2",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "false"
- )
- public boolean b2;
- }
-
- @Test
- public void fromMap_FailsOnWrongKeys() {
- Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true, "a", false);
- Map<Field, Object> fieldMap = keysToFields(SubFooAOptions.class, map);
- try {
- OptionsParser.fromMap(SubFooBOptions.class, fieldMap);
- fail("Should have failed due to the given map's fields not matching the ones on the class");
- } catch (IllegalArgumentException e) {
- assertThat(e)
- .hasMessageThat()
- .contains(
- "Map keys do not match fields of options class; extra map keys: {'a'}; "
- + "extra options class options: {'b1', 'b2'}");
- }
- }
-
- @Test
- public void fromMap_FailsOnWrongTypes() {
- Map<String, Object> map = ImmutableMap.<String, Object>of("foo", 5);
- Map<Field, Object> fieldMap = keysToFields(SubFooAOptions.class, map);
- try {
- OptionsParser.fromMap(FooOptions.class, fieldMap);
- fail("Should have failed due to trying to assign a field value with the wrong type");
- } catch (IllegalArgumentException e) {
- assertThat(e)
- .hasMessageThat()
- .matches("Can not set boolean field .*\\.foo to java\\.lang\\.Integer");
- }
- }
-
- @Test
- public void fromMap_Inheritance() {
- Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true, "bar", true, "baz", 3);
- Map<Field, Object> fieldMap = keysToFields(BazOptions.class, map);
- BazOptions baz = OptionsParser.fromMap(BazOptions.class, fieldMap);
- assertThat(baz.foo).isTrue();
- assertThat(baz.bar).isTrue();
- assertThat(baz.baz).isEqualTo(3);
+ assertThat(alpha.asMap()).containsExactly("a", 0, "b", 0, "c", 0, "d", 0, "e", 0).inOrder();
}
}
