From 987f09f0cf3c5bf2fc5157c20fe0f7979978a40b Mon Sep 17 00:00:00 2001 From: ccalvarin Date: Thu, 31 Aug 2017 19:50:39 +0200 Subject: 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 --- .../analysis/config/TransitiveOptionDetails.java | 16 ++--- .../devtools/build/lib/runtime/BlazeRuntime.java | 23 +++---- .../common/options/IsolatedOptionsData.java | 78 ++++++++++------------ .../devtools/common/options/OptionDefinition.java | 9 ++- .../devtools/common/options/OptionsParser.java | 39 +++++------ .../devtools/common/options/OptionsParserImpl.java | 2 +- .../devtools/common/options/OptionsUsage.java | 2 +- 7 files changed, 74 insertions(+), 95 deletions(-) (limited to 'src/main/java') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java index 6aa27b22d2..f1a2329403 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java @@ -17,11 +17,10 @@ package com.google.devtools.build.lib.analysis.config; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.common.options.OptionDefinition; -import com.google.devtools.common.options.OptionDefinition.NotAnOptionException; import com.google.devtools.common.options.OptionMetadataTag; import com.google.devtools.common.options.OptionsBase; +import com.google.devtools.common.options.OptionsParser; import java.io.Serializable; -import java.lang.reflect.Field; import java.util.Map; import javax.annotation.Nullable; @@ -43,20 +42,15 @@ public final class TransitiveOptionDetails implements Serializable { ImmutableMap.Builder map = ImmutableMap.builder(); try { for (OptionsBase options : buildOptions) { - for (Field field : options.getClass().getFields()) { - OptionDefinition optionDefinition; - try { - optionDefinition = OptionDefinition.extractOptionDefinition(field); - } catch (NotAnOptionException e) { - // Skip non @Option fields. - continue; - } + ImmutableList optionDefinitions = + OptionsParser.getOptionDefinitions(options.getClass()); + for (OptionDefinition optionDefinition : optionDefinitions) { if (ImmutableList.copyOf(optionDefinition.getOptionMetadataTags()) .contains(OptionMetadataTag.INTERNAL)) { // ignore internal options continue; } - Object value = field.get(options); + Object value = optionDefinition.getField().get(options); if (value == null) { if (lateBoundDefaults.containsKey(optionDefinition.getOptionName())) { value = lateBoundDefaults.get(optionDefinition.getOptionName()); 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 a1602342e8..cbd0a4e9d6 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.build.lib.windows.WindowsSubprocessFactory; 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 com.google.devtools.common.options.TriState; 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 @@ public final class BlazeRuntime { static CommandLineOptions splitStartupOptions( Iterable modules, String... args) { List prefixes = new ArrayList<>(); - List startupFields = Lists.newArrayList(); + List startupOptions = Lists.newArrayList(); for (Class 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()); } } 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 754ca2caa0..34208d01a5 100644 --- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java +++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java @@ -45,6 +45,40 @@ import javax.annotation.concurrent.Immutable; @Immutable public class IsolatedOptionsData extends OpaqueOptionsData { + /** + * Cache for the options in an OptionsBase. + * + *

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, ImmutableList> + allOptionsFields = new HashMap<>(); + + /** Returns all {@code optionDefinitions}, ordered by their option name (not their field name). */ + public static synchronized ImmutableList getAllOptionDefinitionsForClass( + Class 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { /** Mapping from option abbreviation to {@code OptionDefinition} (unordered). */ private final ImmutableMap 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, ImmutableList> - allOptionsFields; /** * Mapping from each options class to whether or not it has the {@link UsesOnlyCoreTypes} @@ -82,12 +110,10 @@ public class IsolatedOptionsData extends OpaqueOptionsData { Map, Constructor> optionsClasses, Map nameToField, Map abbrevToField, - Map, ImmutableList> allOptionsFields, Map, 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { other.optionsClasses, other.nameToField, other.abbrevToField, - other.allOptionsFields, other.usesOnlyCoreTypes); } @@ -130,38 +155,9 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 getOptionDefinitionsFromClass( - Class optionsClass) { - return allOptionsFields.get(optionsClass); - } - public boolean getUsesOnlyCoreTypes(Class optionsClass) { return usesOnlyCoreTypes.get(optionsClass); } - - /** Returns all {@code optionDefinitions}, ordered by their option name (not their field name). */ - private static ImmutableList getAllOptionDefinitionsSorted( - Class 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 void checkForCollisions( Map aFieldMap, A optionName, String description) { if (aFieldMap.containsKey(optionName)) { @@ -204,8 +200,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData { static IsolatedOptionsData from(Collection> classes) { // Mind which fields have to preserve order. Map, Constructor> constructorBuilder = new LinkedHashMap<>(); - Map, ImmutableList> allOptionsFieldsBuilder = - new HashMap<>(); Map nameToFieldBuilder = new LinkedHashMap<>(); Map abbrevToFieldBuilder = new HashMap<>(); @@ -225,8 +219,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData { + " lacks an accessible default constructor"); } ImmutableList optionDefinitions = - getAllOptionDefinitionsSorted(parsedOptionsClass); - allOptionsFieldsBuilder.put(parsedOptionsClass, optionDefinitions); + getAllOptionDefinitionsForClass(parsedOptionsClass); for (OptionDefinition optionDefinition : optionDefinitions) { String optionName = optionDefinition.getOptionName(); @@ -297,7 +290,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 1b55a8cef2..08962bba0d 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 @@ import java.util.Comparator; 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 @@ public class OptionDefinition { /** * If the {@code field} is annotated with the appropriate @{@link Option} annotation, returns the * {@code OptionDefinition} for that option. Otherwise, throws a {@link NotAnOptionException}. + * + *

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 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 allFields = new ArrayList<>(); for (Class 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 allFields = new ArrayList<>(); for (Class 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 getFields(Class optionsClass) { - OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass); - return data.getOptionDefinitionsFromClass(optionsClass); + public static ImmutableList getOptionDefinitions( + Class 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 Map toMap(Class optionsClass, O options) { - OptionsData data = getOptionsDataInternal(optionsClass); - // Alphabetized due to getOptionDefinitionsFromClass()'s order. + // Alphabetized due to getAllOptionDefinitionsForClass()'s order. Map 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 optionDefinitions = data.getOptionDefinitionsFromClass(optionsClass); + List optionDefinitions = + OptionsData.getAllOptionDefinitionsForClass(optionsClass); // Ensure all fields are covered, no extraneous fields. - validateFieldsSets(data, optionsClass, new LinkedHashSet(map.keySet())); + validateFieldsSets(optionsClass, new LinkedHashSet(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 optionsClass, LinkedHashSet fieldsFromMap) { - ImmutableList optionFieldsFromClasses = - data.getOptionDefinitionsFromClass(optionsClass); + ImmutableList optionDefsFromClasses = + OptionsData.getAllOptionDefinitionsForClass(optionsClass); Set 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 extraNamesFromClass = new ArrayList<>(); List 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(""); } 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 28aeb22b70..993fbbe500 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 @@ class OptionsParserImpl { // 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 0ab30da2b5..88da29f6cc 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 @@ class OptionsUsage { static void getUsage(Class optionsClass, StringBuilder usage) { OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass); List 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); -- cgit v1.2.3