diff options
author | nharmata <nharmata@google.com> | 2018-02-27 14:49:17 -0800 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2018-02-27 14:50:51 -0800 |
commit | 701da6ebe8789b18fef42eb4422cb032775a47bb (patch) | |
tree | 8c54cf51ce1c4ada576b2b8157f1800900e7fee0 /src/main/java/com/google/devtools | |
parent | 55f61edec105229daa6e9ed772d0c659530c7b27 (diff) |
Optimize GC churn of funcall evaluation for the common-case where there are no
duplicate named arguments.
RELNOTES: None
PiperOrigin-RevId: 187236124
Diffstat (limited to 'src/main/java/com/google/devtools')
-rw-r--r-- | src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java | 79 |
1 files changed, 45 insertions, 34 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java index 5584dfdea0..68e5006ead 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java @@ -584,25 +584,24 @@ public final class FuncallExpression extends Expression { } /** - * Add one argument to the keyword map, registering a duplicate in case of conflict. + * Add one named argument to the keyword map, and returns whether that name has been encountered + * before. */ - private static void addKeywordArg( + private static boolean addKeywordArgAndCheckIfDuplicate( Map<String, Object> kwargs, String name, - Object value, - ImmutableList.Builder<String> duplicates) { - if (kwargs.put(name, value) != null) { - duplicates.add(name); - } + Object value) { + return kwargs.put(name, value) != null; } /** - * Add multiple arguments to the keyword map (**kwargs), registering duplicates + * Add multiple arguments to the keyword map (**kwargs), and returns all the names of those + * arguments that have been encountered before or {@code null} if there are no such names. */ - private static void addKeywordArgs( + @Nullable + private static ImmutableList<String> addKeywordArgsAndReturnDuplicates( Map<String, Object> kwargs, Object items, - ImmutableList.Builder<String> duplicates, Location location) throws EvalException { if (!(items instanceof Map<?, ?>)) { @@ -610,14 +609,22 @@ public final class FuncallExpression extends Expression { location, "argument after ** must be a dictionary, not '" + EvalUtils.getDataTypeName(items) + "'"); } + ImmutableList.Builder<String> duplicatesBuilder = null; for (Map.Entry<?, ?> entry : ((Map<?, ?>) items).entrySet()) { if (!(entry.getKey() instanceof String)) { throw new EvalException( location, "keywords must be strings, not '" + EvalUtils.getDataTypeName(entry.getKey()) + "'"); } - addKeywordArg(kwargs, (String) entry.getKey(), entry.getValue(), duplicates); + String argName = (String) entry.getKey(); + if (addKeywordArgAndCheckIfDuplicate(kwargs, argName, entry.getValue())) { + if (duplicatesBuilder == null) { + duplicatesBuilder = ImmutableList.builder(); + } + duplicatesBuilder.add(argName); + } } + return duplicatesBuilder == null ? null : duplicatesBuilder.build(); } /** @@ -636,25 +643,6 @@ public final class FuncallExpression extends Expression { } /** - * Check the list from the builder and report an {@link EvalException} if not empty. - */ - private static void checkDuplicates( - ImmutableList.Builder<String> duplicates, Expression function, Location location) - throws EvalException { - List<String> dups = duplicates.build(); - if (!dups.isEmpty()) { - throw new EvalException( - location, - "duplicate keyword" - + (dups.size() > 1 ? "s" : "") - + " '" - + Joiner.on("', '").join(dups) - + "' in call to " - + function); - } - } - - /** * Call a method depending on the type of an object it is called on. * * @param positionals The first object is expected to be the object the method is called on. @@ -728,7 +716,8 @@ public final class FuncallExpression extends Expression { private void evalArguments(ImmutableList.Builder<Object> posargs, Map<String, Object> kwargs, Environment env) throws EvalException, InterruptedException { - ImmutableList.Builder<String> duplicates = new ImmutableList.Builder<>(); + // Optimize allocations for the common case where they are no duplicates. + ImmutableList.Builder<String> duplicatesBuilder = null; // Iterate over the arguments. We assume all positional arguments come before any keyword // or star arguments, because the argument list was already validated by // Argument#validateFuncallArguments, as called by the Parser, @@ -748,12 +737,34 @@ public final class FuncallExpression extends Expression { } posargs.addAll((Iterable<Object>) value); } else if (arg.isStarStar()) { // expand the kwargs - addKeywordArgs(kwargs, value, duplicates, getLocation()); + ImmutableList<String> duplicates = + addKeywordArgsAndReturnDuplicates(kwargs, value, getLocation()); + if (duplicates != null) { + if (duplicatesBuilder == null) { + duplicatesBuilder = ImmutableList.builder(); + } + duplicatesBuilder.addAll(duplicates); + } } else { - addKeywordArg(kwargs, arg.getName(), value, duplicates); + if (addKeywordArgAndCheckIfDuplicate(kwargs, arg.getName(), value)) { + if (duplicatesBuilder == null) { + duplicatesBuilder = ImmutableList.builder(); + } + duplicatesBuilder.add(arg.getName()); + } } } - checkDuplicates(duplicates, function, getLocation()); + if (duplicatesBuilder != null) { + ImmutableList<String> dups = duplicatesBuilder.build(); + throw new EvalException( + getLocation(), + "duplicate keyword" + + (dups.size() > 1 ? "s" : "") + + " '" + + Joiner.on("', '").join(dups) + + "' in call to " + + function); + } } @VisibleForTesting |