diff options
Diffstat (limited to 'src/main/java/com/google/devtools')
7 files changed, 74 insertions, 95 deletions
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<String, OptionDetails> 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<OptionDefinition> 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<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()); } } 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 @@ -46,6 +46,40 @@ import javax.annotation.concurrent.Immutable; 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { /** 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ 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<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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { + " 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 @@ 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}. + * + * <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 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<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 @@ public class OptionsParser implements OptionsProvider { 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 @@ 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<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 @@ public class OptionsParser implements OptionsProvider { * @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 @@ public class OptionsParser implements OptionsProvider { 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 @@ public class OptionsParser implements OptionsProvider { * 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 @@ public class OptionsParser implements OptionsProvider { 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 @@ public class OptionsParser implements OptionsProvider { 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 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<? 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); |