Skip to content

Commit

Permalink
Replace some substring() and substr() calls with charAt()
Browse files Browse the repository at this point in the history 
Replace cases like foo.substring(0, 1) with foo.charAt(0). According to https://jsperf.com/substr-or-charat, charAt is about 20x faster on Chrome 54. This also saves a bit on code size.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=138557701
  • Loading branch information
@Dominator008 @blickly
Dominator008 authored and blickly committed Nov 9, 2016
1 parent 13f48ca commit 39184a6
Show file tree
Hide file tree
Showing 7 changed files with 180 additions and 88 deletions.
11 changes: 6 additions & 5 deletions src/com/google/javascript/jscomp/DefaultPassConfig.java
View file
Expand Up @@ -1541,12 +1541,12 @@ protected CompilerPass create(AbstractCompiler compiler) {
/** Various peephole optimizations. */
private static CompilerPass createPeepholeOptimizationsPass(AbstractCompiler compiler) {
final boolean late = false;
final boolean useTypesForOptimization = compiler.getOptions().useTypesForLocalOptimization;
final boolean useTypesForOptimization = compiler.getOptions().useTypesForLocalOptimization;
return new PeepholeOptimizationsPass(compiler,
new MinimizeExitPoints(compiler),
new PeepholeMinimizeConditions(late, useTypesForOptimization),
new PeepholeSubstituteAlternateSyntax(late),
new PeepholeReplaceKnownMethods(late),
new PeepholeReplaceKnownMethods(late, useTypesForOptimization),
new PeepholeRemoveDeadCode(),
new PeepholeFoldConstants(late, useTypesForOptimization),
new PeepholeCollectPropertyAssignments());
Expand Down Expand Up @@ -1576,13 +1576,14 @@ protected CompilerPass create(AbstractCompiler compiler) {
@Override
protected CompilerPass create(AbstractCompiler compiler) {
final boolean late = true;
final boolean useTypesForOptimization = options.useTypesForLocalOptimization;
return new PeepholeOptimizationsPass(compiler,
new StatementFusion(options.aggressiveFusion),
new PeepholeRemoveDeadCode(),
new PeepholeMinimizeConditions(late, options.useTypesForLocalOptimization),
new PeepholeMinimizeConditions(late, useTypesForOptimization),
new PeepholeSubstituteAlternateSyntax(late),
new PeepholeReplaceKnownMethods(late),
new PeepholeFoldConstants(late, options.useTypesForLocalOptimization),
new PeepholeReplaceKnownMethods(late, useTypesForOptimization),
new PeepholeFoldConstants(late, useTypesForOptimization),
new ReorderConstantExpression());
}
};
Expand Down
23 changes: 13 additions & 10 deletions src/com/google/javascript/jscomp/ExpandJqueryAliases.java
View file
Expand Up @@ -24,7 +24,6 @@
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -88,7 +87,7 @@ class ExpandJqueryAliases extends AbstractPostOrderCallback
this.peepholePasses = new PeepholeOptimizationsPass(compiler,
new PeepholeMinimizeConditions(late, useTypesForOptimization),
new PeepholeSubstituteAlternateSyntax(late),
new PeepholeReplaceKnownMethods(late),
new PeepholeReplaceKnownMethods(late, useTypesForOptimization),
new PeepholeRemoveDeadCode(),
new PeepholeFoldConstants(late, useTypesForOptimization),
new PeepholeCollectPropertyAssignments());
Expand All @@ -109,11 +108,13 @@ public static boolean isJqueryExtendCall(Node n, String qname,
}

Node secondArgument = firstArgument.getNext();
if ((firstArgument.isObjectLit() && secondArgument == null) ||
(firstArgument.isName() || NodeUtil.isGet(firstArgument) &&
!NodeUtil.mayHaveSideEffects(firstArgument, compiler) &&
secondArgument != null && secondArgument.isObjectLit() &&
secondArgument.getNext() == null)) {
if ((firstArgument.isObjectLit() && secondArgument == null)
|| (firstArgument.isName()
|| (NodeUtil.isGet(firstArgument)
&& !NodeUtil.mayHaveSideEffects(firstArgument, compiler)
&& secondArgument != null
&& secondArgument.isObjectLit()
&& secondArgument.getNext() == null))) {
return true;
}
}
Expand Down Expand Up @@ -355,7 +356,8 @@ private Node tryExpandJqueryEachCall(NodeTraversal t, Node n,
boolean isValidExpansion = true;

// Expand the jQuery.expandedEach call
Node key = objectToLoopOver.getFirstChild(), val = null;
Node key = objectToLoopOver.getFirstChild();
Node val = null;
for (int i = 0; key != null; key = key.getNext(), i++) {
if (key != null) {
if (objectToLoopOver.isArrayLit()) {
Expand Down Expand Up @@ -535,7 +537,8 @@ static class FindCallbackArgumentReferences extends AbstractPostOrderCallback
List<Node> valueReferences, boolean useArrayMode) {
Preconditions.checkState(functionRoot.isFunction());

String keyString = null, valueString = null;
String keyString = null;
String valueString = null;
Node callbackParams = NodeUtil.getFunctionParameters(functionRoot);
Node param = callbackParams.getFirstChild();
if (param != null) {
Expand Down Expand Up @@ -579,7 +582,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
isThis = n.isThis();
}

if (isThis || n.isName() && !isShadowed(n.getString(), t.getScope())) {
if (isThis || (n.isName() && !isShadowed(n.getString(), t.getScope()))) {
String nodeValue = isThis ? null : n.getString();
if (!isThis && keyName != null && nodeValue.equals(keyName)) {
keyReferences.add(n);
Expand Down
171 changes: 110 additions & 61 deletions src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java
View file
Expand Up @@ -21,7 +21,6 @@
import com.google.common.base.Preconditions;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;

import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
Expand All @@ -34,15 +33,17 @@
class PeepholeReplaceKnownMethods extends AbstractPeepholeOptimization{

private final boolean late;
private final boolean useTypes;

/**
* @param late When late is true, this mean we are currently running after
* most of the other optimizations. In this case we avoid changes that make
* the code larger (but otherwise easier to analyze - such as using string
* splitting).
*/
PeepholeReplaceKnownMethods(boolean late) {
PeepholeReplaceKnownMethods(boolean late, boolean useTypes) {
this.late = late;
this.useTypes = useTypes;
}

@Override
Expand All @@ -67,9 +68,9 @@ private Node tryFoldKnownMethods(Node subtree) {
}

if (NodeUtil.isGet(callTarget)) {
subtree = tryFoldKnownStringMethods(subtree);
} else {
subtree = tryFoldKnownNumericMethods(subtree);
subtree = tryFoldKnownStringMethods(subtree, callTarget);
} else if (callTarget.isName()) {
subtree = tryFoldKnownNumericMethods(subtree, callTarget);
}
}

Expand All @@ -80,74 +81,70 @@ private Node tryFoldKnownMethods(Node subtree) {
* Try to evaluate known String methods
* .indexOf(), .substr(), .substring()
*/
private Node tryFoldKnownStringMethods(Node subtree) {
private Node tryFoldKnownStringMethods(Node subtree, Node callTarget) {
Preconditions.checkArgument(subtree.isCall());

// check if this is a call on a string method
// then dispatch to specific folding method.
Node callTarget = subtree.getFirstChild();
if (callTarget == null) {
return subtree;
}

if (!NodeUtil.isGet(callTarget)) {
return subtree;
}

Node stringNode = callTarget.getFirstChild();
Node functionName = stringNode.getNext();
Node functionName = callTarget.getLastChild();

if ((!stringNode.isString()) ||
(!functionName.isString())) {
if (!functionName.isString()) {
return subtree;
}

boolean isStringLiteral = stringNode.isString();
String functionNameString = functionName.getString();
Node firstArg = callTarget.getNext();
if (functionNameString.equals("split")) {
subtree = tryFoldStringSplit(subtree, stringNode, firstArg);
} else if (firstArg == null) {
if (functionNameString.equals("toLowerCase")) {
subtree = tryFoldStringToLowerCase(subtree, stringNode);
} else if (functionNameString.equals("toUpperCase")) {
subtree = tryFoldStringToUpperCase(subtree, stringNode);
if (isStringLiteral) {
if (functionNameString.equals("split")) {
return tryFoldStringSplit(subtree, stringNode, firstArg);
} else if (firstArg == null) {
switch (functionNameString) {
case "toLowerCase":
return tryFoldStringToLowerCase(subtree, stringNode);
case "toUpperCase":
return tryFoldStringToUpperCase(subtree, stringNode);
}
} else {
if (NodeUtil.isImmutableValue(firstArg)) {
switch (functionNameString) {
case "indexOf":
case "lastIndexOf":
return tryFoldStringIndexOf(subtree, functionNameString, stringNode, firstArg);
case "substr":
return tryFoldStringSubstr(subtree, stringNode, firstArg);
case "substring":
return tryFoldStringSubstring(subtree, stringNode, firstArg);
case "charAt":
return tryFoldStringCharAt(subtree, stringNode, firstArg);
case "charCodeAt":
return tryFoldStringCharCodeAt(subtree, stringNode, firstArg);
}
}
}
return subtree;
} else if (NodeUtil.isImmutableValue(firstArg)) {
if (functionNameString.equals("indexOf") ||
functionNameString.equals("lastIndexOf")) {
subtree = tryFoldStringIndexOf(subtree, functionNameString,
stringNode, firstArg);
} else if (functionNameString.equals("substr")) {
subtree = tryFoldStringSubstr(subtree, stringNode, firstArg);
} else if (functionNameString.equals("substring")) {
subtree = tryFoldStringSubstring(subtree, stringNode, firstArg);
} else if (functionNameString.equals("charAt")) {
subtree = tryFoldStringCharAt(subtree, stringNode, firstArg);
} else if (functionNameString.equals("charCodeAt")) {
subtree = tryFoldStringCharCodeAt(subtree, stringNode, firstArg);
} else if (useTypes && firstArg != null
&& stringNode.getJSType() != null && stringNode.getJSType().isStringValueType()) {
switch (functionNameString) {
case "substr":
return tryReplaceSubstrWithCharAt(subtree, callTarget, firstArg);
case "substring":
return tryReplaceSubstringWithCharAt(subtree, callTarget, firstArg);
}
}

return subtree;
}

/**
* Try to evaluate known Numeric methods
* parseInt(), parseFloat()
*/
private Node tryFoldKnownNumericMethods(Node subtree) {
private Node tryFoldKnownNumericMethods(Node subtree, Node callTarget) {
Preconditions.checkArgument(subtree.isCall());

if (isASTNormalized()) {
// check if this is a call on a string method
// then dispatch to specific folding method.
Node callTarget = subtree.getFirstChild();

if (!callTarget.isName()) {
return subtree;
}

String functionNameString = callTarget.getString();
Node firstArgument = callTarget.getNext();
if ((firstArgument != null) && (firstArgument.isString() || firstArgument.isNumber())
Expand Down Expand Up @@ -191,7 +188,8 @@ private static String normalizeNumericString(String input) {
return input;
}

int startIndex = 0, endIndex = input.length() - 1;
int startIndex = 0;
int endIndex = input.length() - 1;

// Remove leading zeros
while (startIndex < input.length() && input.charAt(startIndex) == '0' &&
Expand Down Expand Up @@ -509,22 +507,24 @@ private Node tryFoldArrayJoin(Node n) {
private Node tryFoldStringSubstr(Node n, Node stringNode, Node arg1) {
Preconditions.checkArgument(n.isCall());
Preconditions.checkArgument(stringNode.isString());
Preconditions.checkArgument(arg1 != null);

int start, length;
int start;
int length;
String stringAsString = stringNode.getString();

// TODO(nicksantos): We really need a NodeUtil.getNumberValue
// function.
if (arg1 != null && arg1.isNumber()) {
start = (int) arg1.getDouble();
Double maybeStart = NodeUtil.getNumberValue(arg1, useTypes);
if (maybeStart != null) {
start = maybeStart.intValue();
} else {
return n;
}

Node arg2 = arg1.getNext();
if (arg2 != null) {
if (arg2.isNumber()) {
length = (int) arg2.getDouble();
Double maybeLength = NodeUtil.getNumberValue(arg2, useTypes);
if (maybeLength != null) {
length = maybeLength.intValue();
} else {
return n;
}
Expand Down Expand Up @@ -563,20 +563,24 @@ private Node tryFoldStringSubstr(Node n, Node stringNode, Node arg1) {
private Node tryFoldStringSubstring(Node n, Node stringNode, Node arg1) {
Preconditions.checkArgument(n.isCall());
Preconditions.checkArgument(stringNode.isString());
Preconditions.checkArgument(arg1 != null);

int start, end;
int start;
int end;
String stringAsString = stringNode.getString();

if (arg1 != null && arg1.isNumber()) {
start = (int) arg1.getDouble();
Double maybeStart = NodeUtil.getNumberValue(arg1, useTypes);
if (maybeStart != null) {
start = maybeStart.intValue();
} else {
return n;
}

Node arg2 = arg1.getNext();
if (arg2 != null) {
if (arg2.isNumber()) {
end = (int) arg2.getDouble();
Double maybeEnd = NodeUtil.getNumberValue(arg2, useTypes);
if (maybeEnd != null) {
end = maybeEnd.intValue();
} else {
return n;
}
Expand Down Expand Up @@ -610,6 +614,50 @@ private Node tryFoldStringSubstring(Node n, Node stringNode, Node arg1) {
return resultNode;
}

private Node tryReplaceSubstrWithCharAt(Node n, Node callTarget, Node firstArg) {
if (n.getChildCount() == 3) {
Double maybeLength = NodeUtil.getNumberValue(firstArg.getNext(), useTypes);
if (maybeLength != null) {
int length = maybeLength.intValue();
if (length == 1) {
return replaceWithCharAt(n, callTarget, firstArg);
}
}
}
return n;
}

private Node tryReplaceSubstringWithCharAt(Node n, Node callTarget, Node firstArg) {
if (n.getChildCount() != 3) {
return n;
}
Double maybeStart = NodeUtil.getNumberValue(firstArg, useTypes);
if (maybeStart == null) {
return n;
}
Node secondArg = firstArg.getNext();
Double maybeEnd = NodeUtil.getNumberValue(secondArg, useTypes);
if (maybeEnd == null) {
return n;
}
int start = maybeStart.intValue();
int end = maybeEnd.intValue();
if (Math.abs(start - end) == 1) {
return replaceWithCharAt(n, callTarget, start < end ? firstArg : secondArg);
}
return n;
}

private Node replaceWithCharAt(Node n, Node callTarget, Node firstArg) {
// TODO(moz): Maybe correct the arity of the function type here.
n.detachChildren();
callTarget.getLastChild().setString("charAt");
n.addChildToFront(callTarget);
n.addChildToBack(firstArg);
reportCodeChange();
return n;
}

/**
* Try to fold .charAt() calls on strings
*/
Expand Down Expand Up @@ -718,7 +766,8 @@ private String[] jsSplit(String stringValue, String separator, int limit) {
splitStrings.add(stringValue.substring(i, i + 1));
}
} else {
int startIndex = 0, matchIndex;
int startIndex = 0;
int matchIndex;
while ((matchIndex =
jsSplitMatch(stringValue, startIndex, separator)) >= 0 &&
splitStrings.size() < limit) {
Expand Down
1 change: 1 addition & 0 deletions src/com/google/javascript/rhino/testing/BaseJSTypeTestCase.java
View file
Expand Up @@ -380,6 +380,7 @@ public static void addNativeProperties(JSTypeRegistry registry) {
addMethod(registry, stringPrototype, "search", numberType);
addMethod(registry, stringPrototype, "slice", stringType);
addMethod(registry, stringPrototype, "split", arrayType);
addMethod(registry, stringPrototype, "substr", stringType);
addMethod(registry, stringPrototype, "substring", stringType);
addMethod(registry, stringPrototype, "toLowerCase", stringType);
addMethod(registry, stringPrototype, "toLocaleLowerCase", stringType);
Expand Down

6 comments on commit 39184a6

@phistuck
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not string[1] instead of string.charAt(1)? Shorter and marginally more performant in Chrome, significantly more performant in Internet Explorer 11 -
https://jsperf.com/substr-or-charat/2

(In Firefox, everything has more or less the same performance characteristics)

@MatrixFrog
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe string[1] doesn't work in some older IEs. If we had a flag to indicate whether you need to support those, we could probably do that.

@phistuck
Copy link
Contributor

@phistuck phistuck commented on 39184a6 Nov 13, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not work in the ancient Internet Explorer 7 and older. Are those really still supported?
http://netmarketshare.com/ desktop by version shows those as lower than a percent (combined).
http://gs.statcounter.com/#browser_version_partially_combined-ww-monthly-201610-201611-bar shows them (export the data to see) as lower than 0.12% (combined).

Anyway, you can use the ECMAScript 5 language output to control it, because it is part of that, according to https://kangax.github.io/compat-table/es5

@ChadKillingsworth
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not work in the ancient Internet Explorer 7 and older. Are those really still supported?

Yes. Google Search and others.

use the ECMAScript 5 language output

That's my suggestion as well

@MatrixFrog
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we just have a separate pass that explicitly converts charAt calls (whether added by this pass, or in the original code) to bracket notation?

@ChadKillingsworth
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems very suited as a peephole optimization.

Please sign in to comment.