Optimize GC churn of funcall evaluation for the common-case where there are no
duplicate named arguments.
RELNOTES: None
PiperOrigin-RevId: 187236124
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 5584dfd..68e5006 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 @@
}
/**
- * 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 @@
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 @@
}
/**
- * 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 @@
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 @@
}
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
