Memoize the OptionsData per BlazeCommand. This saves the cost of (1) collecting all Options classes and (2) getting all their @Option annotations. Note that there is no savings on reflection costs, since that's already memoized internally by OptionsParser. This saves ~250us per Blaze invocation. -- MOS_MIGRATED_REVID=121153156
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java index 5abddfe..3e17b47 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -18,12 +18,14 @@ import com.google.common.base.Joiner; import com.google.common.base.Predicates; import com.google.common.base.Verify; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; -import com.google.common.collect.Lists; import com.google.common.io.Flushables; import com.google.devtools.build.lib.Constants; import com.google.devtools.build.lib.events.Event; @@ -40,8 +42,8 @@ import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.common.options.OpaqueOptionsData; import com.google.devtools.common.options.OptionPriority; -import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; @@ -146,6 +148,17 @@ private final Object commandLock; private String currentClientDescription = null; private OutputStream logOutputStream = null; + private final LoadingCache<BlazeCommand, OpaqueOptionsData> optionsDataCache = + CacheBuilder.newBuilder().build( + new CacheLoader<BlazeCommand, OpaqueOptionsData>() { + @Override + public OpaqueOptionsData load(BlazeCommand command) { + return OptionsParser.getOptionsData(BlazeCommandUtils.getOptions( + command.getClass(), + runtime.getBlazeModules(), + runtime.getRuleClassProvider())); + } + }); /** * Create a Blaze dispatcher that uses the specified {@code BlazeRuntime} instance, but overrides @@ -655,10 +668,7 @@ protected OptionsParser createOptionsParser(BlazeCommand command) throws OptionsParsingException { Command annotation = command.getClass().getAnnotation(Command.class); - List<Class<? extends OptionsBase>> allOptions = Lists.newArrayList(); - allOptions.addAll(BlazeCommandUtils.getOptions( - command.getClass(), getRuntime().getBlazeModules(), getRuntime().getRuleClassProvider())); - OptionsParser parser = OptionsParser.newOptionsParser(allOptions); + OptionsParser parser = OptionsParser.newOptionsParser(optionsDataCache.getUnchecked(command)); parser.setAllowResidue(annotation.allowResidue()); return parser; }
diff --git a/src/main/java/com/google/devtools/common/options/OpaqueOptionsData.java b/src/main/java/com/google/devtools/common/options/OpaqueOptionsData.java new file mode 100644 index 0000000..befc122 --- /dev/null +++ b/src/main/java/com/google/devtools/common/options/OpaqueOptionsData.java
@@ -0,0 +1,26 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.common.options; + +/** + * Opaque options data type, returned by {@link OptionsParser#getOptionsData} and consumed by + * {@link OptionsParser#newOptionsParser(OpaqueOptionsData)}. + */ +public abstract class OpaqueOptionsData { + // Package-protected so the existence of subclasses is under our control. + OpaqueOptionsData() { + } +} +
diff --git a/src/main/java/com/google/devtools/common/options/OptionsData.java b/src/main/java/com/google/devtools/common/options/OptionsData.java index 0038cad..ac23d63 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsData.java +++ b/src/main/java/com/google/devtools/common/options/OptionsData.java
@@ -37,7 +37,7 @@ * Therefore this class can be used internally to cache the results. */ @Immutable -final class OptionsData { +final class OptionsData extends OpaqueOptionsData { /** * These are the options-declaring classes which are annotated with
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 e47d219..fd6d915 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -72,7 +72,20 @@ private static final Map<ImmutableList<Class<? extends OptionsBase>>, OptionsData> optionsData = Maps.newHashMap(); - private static synchronized OptionsData getOptionsData( + /** + * Returns {@link OpaqueOptionsData} suitable for passing along to + * {@link #newOptionsParser(OpaqueOptionsData optionsData)}. + * + * This is useful when you want to do the work of analyzing the given {@code optionsClasses} + * exactly once, but you want to parse lots of different lists of strings (and thus need to + * construct lots of different {@link OptionsParser} instances). + */ + public static OpaqueOptionsData getOptionsData( + ImmutableList<Class<? extends OptionsBase>> optionsClasses) { + return getOptionsDataInternal(optionsClasses); + } + + private static synchronized OptionsData getOptionsDataInternal( ImmutableList<Class<? extends OptionsBase>> optionsClasses) { OptionsData result = optionsData.get(optionsClasses); if (result == null) { @@ -87,7 +100,8 @@ * ones. */ static Collection<Field> getAllAnnotatedFields(Class<? extends OptionsBase> optionsClass) { - OptionsData data = getOptionsData(ImmutableList.<Class<? extends OptionsBase>>of(optionsClass)); + OptionsData data = getOptionsDataInternal( + ImmutableList.<Class<? extends OptionsBase>>of(optionsClass)); return data.getFieldsForClass(optionsClass); } @@ -111,8 +125,16 @@ */ public static OptionsParser newOptionsParser( Iterable<? extends Class<? extends OptionsBase>> optionsClasses) { - return new OptionsParser( - getOptionsData(ImmutableList.<Class<? extends OptionsBase>>copyOf(optionsClasses))); + return newOptionsParser( + getOptionsDataInternal(ImmutableList.<Class<? extends OptionsBase>>copyOf(optionsClasses))); + } + + /** + * Create a new {@link OptionsParser}, using {@link OpaqueOptionsData} previously returned from + * {@link #getOptionsData}. + */ + public static OptionsParser newOptionsParser(OpaqueOptionsData optionsData) { + return new OptionsParser((OptionsData) optionsData); } private final OptionsParserImpl impl;