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
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 754ca2c..34208d0 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 @@
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 @@
/** 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 @@
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 @@
other.optionsClasses,
other.nameToField,
other.abbrevToField,
- other.allOptionsFields,
other.usesOnlyCoreTypes);
}
@@ -130,38 +155,9 @@
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 @@
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 @@
+ " 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 @@
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 1b55a8c..08962bb 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 @@
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 @@
/**
* 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 9d68320..618e0dd 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.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 @@
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 @@
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 @@
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 @@
}
/** 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 @@
* @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 @@
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 @@
* 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 @@
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 @@
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 28aeb22..993fbbe 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 @@
// 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 0ab30da..88da29f 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 @@
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);
