Skip to content

Commit

Permalink
Reduce iterator usage on hot code paths
Browse files Browse the repository at this point in the history
Micro-optimize a few hot code paths which have a habit of iterating over short O(1)-access
lists.

RELNOTES: None
PiperOrigin-RevId: 171347524
  • Loading branch information
michajlo authored and aehlig committed Oct 7, 2017
1 parent 38da0c2 commit 5f39475
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 17 deletions.
21 changes: 12 additions & 9 deletions src/main/java/com/google/devtools/build/lib/syntax/Eval.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.syntax;

import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -58,9 +59,7 @@ void execAugmentedAssignment(AugmentedAssignmentStatement node)

void execIfBranch(IfStatement.ConditionalStatements node)
throws EvalException, InterruptedException {
for (Statement stmt : node.getStatements()) {
exec(stmt);
}
execStatements(node.getStatements());
}

void execFor(ForStatement node) throws EvalException, InterruptedException {
Expand All @@ -72,9 +71,7 @@ void execFor(ForStatement node) throws EvalException, InterruptedException {
node.getVariable().assign(it, env, node.getLocation());

try {
for (Statement stmt : node.getBlock()) {
exec(stmt);
}
execStatements(node.getBlock());
} catch (FlowException ex) {
if (ex == breakException) {
return;
Expand Down Expand Up @@ -123,9 +120,7 @@ void execIf(IfStatement node) throws EvalException, InterruptedException {
return;
}
}
for (Statement stmt : node.getElseBlock()) {
exec(stmt);
}
execStatements(node.getElseBlock());
}

void execLoad(LoadStatement node) throws EvalException, InterruptedException {
Expand Down Expand Up @@ -216,4 +211,12 @@ void execDispatch(Statement st) throws EvalException, InterruptedException {
break;
}
}

private void execStatements(ImmutableList<Statement> statements)
throws EvalException, InterruptedException {
// Hot code path, good chance of short lists which don't justify the iterator overhead.
for (int i = 0; i < statements.size(); i++) {
exec(statements.get(i));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,11 @@ public FuncallException(String msg) {

private final Expression function;

private final List<Argument.Passed> arguments;
private final ImmutableList<Argument.Passed> arguments;

private final int numPositionalArgs;

public FuncallExpression(Expression function, List<Argument.Passed> arguments) {
public FuncallExpression(Expression function, ImmutableList<Argument.Passed> arguments) {
this.function = Preconditions.checkNotNull(function);
this.arguments = Preconditions.checkNotNull(arguments);
this.numPositionalArgs = countPositionalArguments();
Expand Down Expand Up @@ -679,7 +679,10 @@ private void evalArguments(ImmutableList.Builder<Object> posargs, Map<String, Ob
// or star arguments, because the argument list was already validated by
// Argument#validateFuncallArguments, as called by the Parser,
// which should be the only place that build FuncallExpression-s.
for (Argument.Passed arg : arguments) {
// Argument lists are typically short and functions are frequently called, so go by index
// (O(1) for ImmutableList) to avoid the iterator overhead.
for (int i = 0; i < arguments.size(); i++) {
Argument.Passed arg = arguments.get(i);
Object value = arg.getValue().eval(env);
if (arg.isPositional()) {
posargs.add(value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ private Parameter<Expression, Expression> parseFunctionParameter() {

// funcall_suffix ::= '(' arg_list? ')'
private Expression parseFuncallSuffix(int start, Expression function) {
List<Argument.Passed> args = Collections.emptyList();
ImmutableList<Argument.Passed> args = ImmutableList.of();
expect(TokenKind.LPAREN);
int end;
if (token.kind == TokenKind.RPAREN) {
Expand Down Expand Up @@ -509,8 +509,8 @@ private Expression parseSelectorSuffix(int start, Expression receiver) {
}

// arg_list ::= ( (arg ',')* arg ','? )?
private List<Argument.Passed> parseFuncallArguments() {
List<Argument.Passed> arguments = parseFunctionArguments(this::parseFuncallArgument);
private ImmutableList<Argument.Passed> parseFuncallArguments() {
ImmutableList<Argument.Passed> arguments = parseFunctionArguments(this::parseFuncallArgument);
try {
Argument.validateFuncallArguments(arguments);
} catch (Argument.ArgumentException e) {
Expand Down
12 changes: 10 additions & 2 deletions src/main/java/com/google/devtools/build/lib/syntax/Type.java
Original file line number Diff line number Diff line change
Expand Up @@ -568,8 +568,16 @@ public List<ElemT> getDefaultValue() {
@Override
public <T> void visitLabels(LabelVisitor<T> visitor, Object value, T context)
throws InterruptedException {
for (ElemT elem : cast(value)) {
elemType.visitLabels(visitor, elem, context);
List<ElemT> elems = cast(value);
// Hot code path. Optimize for lists with O(1) access to avoid iterator garbage.
if (elems instanceof ImmutableList || elems instanceof ArrayList) {
for (int i = 0; i < elems.size(); i++) {
elemType.visitLabels(visitor, elems.get(i), context);
}
} else {
for (ElemT elem : elems) {
elemType.visitLabels(visitor, elem, context);
}
}
}

Expand Down

0 comments on commit 5f39475

Please sign in to comment.