Skip to content

Commit

Permalink
Improves j2clOptBundlePass perf by running on just changed scopes.
Browse files Browse the repository at this point in the history 
In [] j2clOptBundlePass accounts for 7.15% of execution time. 4.0% of that time is the PureFunctionIdentifier and the other 3.15% is J2CL pass visitors.

Though this CL does not migrate *all* J2CL pass visitors to the new scope limited traversal, it does migrate most (and documents what is necessary to migrate the rest) and reduces that 3.15% cost to 1.5%.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=159268542
  • Loading branch information
@stalcup @blickly
stalcup authored and blickly committed Jun 16, 2017
1 parent a35aa3d commit a9f0ce0
Show file tree
Hide file tree
Showing 16 changed files with 230 additions and 66 deletions.
9 changes: 6 additions & 3 deletions src/com/google/javascript/jscomp/DefaultPassConfig.java
View file
Expand Up @@ -3111,11 +3111,14 @@ protected FeatureSet featureSet() {
new PassFactory("j2clOptBundlePass", false) {
@Override
protected CompilerPass create(AbstractCompiler compiler) {
final J2clClinitPrunerPass j2clClinitPrunerPass = new J2clClinitPrunerPass(compiler);
List<Node> changedScopeNodes = compiler.getChangedScopeNodesForPass(getName());

final J2clClinitPrunerPass j2clClinitPrunerPass =
new J2clClinitPrunerPass(compiler, changedScopeNodes);
final J2clConstantHoisterPass j2clConstantHoisterPass =
(new J2clConstantHoisterPass(compiler));
new J2clConstantHoisterPass(compiler);
final J2clEqualitySameRewriterPass j2clEqualitySameRewriterPass =
(new J2clEqualitySameRewriterPass(compiler));
new J2clEqualitySameRewriterPass(compiler, changedScopeNodes);
return new CompilerPass() {

@Override
Expand Down
15 changes: 6 additions & 9 deletions src/com/google/javascript/jscomp/FunctionToBlockMutator.java
View file
Expand Up @@ -216,7 +216,6 @@ private static void fixUninitializedVarDeclarations(Node n, Node containingBlock
}
}


/**
* Fix-up all local names to be unique for this subtree.
* @param fnNode A mutable instance of the function to be inlined.
Expand All @@ -225,16 +224,14 @@ private void makeLocalNamesUnique(Node fnNode, boolean isCallInLoop) {
Supplier<String> idSupplier = compiler.getUniqueNameIdSupplier();
// Make variable names unique to this instance.
NodeTraversal.traverseEs6(
compiler, fnNode, new MakeDeclaredNamesUnique(
compiler,
fnNode,
new MakeDeclaredNamesUnique(
new InlineRenamer(
compiler.getCodingConvention(),
idSupplier,
"inline_",
isCallInLoop,
true,
null)));
compiler.getCodingConvention(), idSupplier, "inline_", isCallInLoop, true, null),
false));
// Make label names unique to this instance.
new RenameLabels(compiler, new LabelNameSupplier(idSupplier), false)
new RenameLabels(compiler, new LabelNameSupplier(idSupplier), false, false)
.process(null, fnNode);
}

Expand Down
1 change: 1 addition & 0 deletions src/com/google/javascript/jscomp/InlineFunctions.java
View file
Expand Up @@ -776,6 +776,7 @@ void removeInlinedFunctions() {
Preconditions.checkState(fn != null);
verifyAllReferencesInlined(functionState);
fn.remove();
NodeUtil.markFunctionsDeleted(fn.getFunctionNode(), compiler);
}
}
}
Expand Down
42 changes: 34 additions & 8 deletions src/com/google/javascript/jscomp/J2clClinitPrunerPass.java
View file
Expand Up @@ -19,10 +19,12 @@
import com.google.common.base.Strings;
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
import com.google.javascript.jscomp.NodeTraversal.Callback;
import com.google.javascript.jscomp.NodeTraversal.FunctionCallback;
import com.google.javascript.rhino.Node;
import java.util.ArrayDeque;
import java.util.Deque;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import javax.annotation.Nullable;

Expand All @@ -33,9 +35,11 @@ public class J2clClinitPrunerPass implements CompilerPass {

private final AbstractCompiler compiler;
private boolean madeChange = false;
private final List<Node> changedScopeNodes;

J2clClinitPrunerPass(AbstractCompiler compiler) {
J2clClinitPrunerPass(AbstractCompiler compiler, List<Node> changedScopeNodes) {
this.compiler = compiler;
this.changedScopeNodes = changedScopeNodes;
}

@Override
Expand All @@ -44,19 +48,41 @@ public void process(Node externs, Node root) {
return;
}

NodeTraversal.traverseEs6(compiler, root, new RedundantClinitPruner());
NodeTraversal.traverseEs6(compiler, root, new LookAheadRedundantClinitPruner());
NodeTraversal.traverseEs6(compiler, root, new EmptyClinitPruner());
RedundantClinitPruner redundantClinitPruner = new RedundantClinitPruner();
NodeTraversal.traverseEs6ScopeRoots(
compiler,
root,
getNonNestedParentScopeNodes(),
redundantClinitPruner,
redundantClinitPruner, // FunctionCallback
true);
NodeTraversal.traverseEs6ScopeRoots(
compiler, root, changedScopeNodes, new LookAheadRedundantClinitPruner(), false);
NodeTraversal.traverseEs6ScopeRoots(
compiler, root, changedScopeNodes, new EmptyClinitPruner(), false);

if (madeChange) {
// This invocation is ~70% of ALL the cost of j2clOptBundlePass :(
new PureFunctionIdentifier.Driver(compiler, null).process(externs, root);
}
}

/**
* Removes redundant clinit calls inside method body if it is guaranteed to be called earlier.
*/
private final class RedundantClinitPruner implements Callback {
private List<Node> getNonNestedParentScopeNodes() {
return changedScopeNodes == null
? null
: NodeUtil.removeNestedChangeScopeNodes(
NodeUtil.getParentChangeScopeNodes(changedScopeNodes));
}

/** Removes redundant clinit calls inside method body if it is guaranteed to be called earlier. */
private final class RedundantClinitPruner implements Callback, FunctionCallback {

@Override
public void enterFunction(AbstractCompiler compiler, Node fnRoot) {
// Reset the clinit call tracking when starting over on a new scope.
clinitsCalledAtBranch = new HierarchicalSet<>(null);
stateStack.clear();
}

private final Deque<HierarchicalSet<String>> stateStack = new ArrayDeque<>();
private HierarchicalSet<String> clinitsCalledAtBranch = new HierarchicalSet<>(null);
Expand Down
29 changes: 18 additions & 11 deletions src/com/google/javascript/jscomp/J2clConstantHoisterPass.java
View file
Expand Up @@ -46,17 +46,24 @@ public void process(Node externs, Node root) {

final Multimap<String, Node> fieldAssignments = ArrayListMultimap.create();
final Set<Node> hoistableFunctions = new HashSet<>();
NodeTraversal.traverseEs6(compiler, root, new AbstractPostOrderCallback() {
@Override
public void visit(NodeTraversal t, Node node, Node parent) {
if (parent != null && NodeUtil.isLValue(node)) {
fieldAssignments.put(node.getQualifiedName(), parent);
}
if (isHoistableFunction(t, node)) {
hoistableFunctions.add(node);
}
}
});
NodeTraversal.traverseEs6(
compiler,
root,
new AbstractPostOrderCallback() {
@Override
public void visit(NodeTraversal t, Node node, Node parent) {
// TODO(stalcup): don't gather assignments ourselves, switch to a persistent
// DefinitionUseSiteFinder instead.
if (parent != null && NodeUtil.isLValue(node)) {
fieldAssignments.put(node.getQualifiedName(), parent);
}

// TODO(stalcup): convert to a persistent index of hoistable functions.
if (isHoistableFunction(t, node)) {
hoistableFunctions.add(node);
}
}
});

for (Collection<Node> assignments : fieldAssignments.asMap().values()) {
maybeHoistClassField(assignments, hoistableFunctions);
Expand Down
7 changes: 5 additions & 2 deletions src/com/google/javascript/jscomp/J2clEqualitySameRewriterPass.java
View file
Expand Up @@ -18,6 +18,7 @@
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;
import java.util.List;

/** An optimization pass to re-write J2CL Equality.$same. */
public class J2clEqualitySameRewriterPass extends AbstractPostOrderCallback
Expand All @@ -30,9 +31,11 @@ private static enum Eq {
}

private final AbstractCompiler compiler;
private final List<Node> changedScopeNodes;

J2clEqualitySameRewriterPass(AbstractCompiler compiler) {
J2clEqualitySameRewriterPass(AbstractCompiler compiler, List<Node> changedScopeNodes) {
this.compiler = compiler;
this.changedScopeNodes = changedScopeNodes;
}

@Override
Expand All @@ -41,7 +44,7 @@ public void process(Node externs, Node root) {
return;
}

NodeTraversal.traverseEs6(compiler, root, this);
NodeTraversal.traverseEs6ScopeRoots(compiler, root, changedScopeNodes, this, false);
}

@Override
Expand Down
44 changes: 30 additions & 14 deletions src/com/google/javascript/jscomp/MakeDeclaredNamesUnique.java
View file
Expand Up @@ -25,6 +25,7 @@
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Multiset;
import com.google.javascript.jscomp.MakeDeclaredNamesUnique.ContextualRenameInverter;
import com.google.javascript.jscomp.NodeTraversal.ScopedCallback;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;
Expand Down Expand Up @@ -61,17 +62,23 @@ class MakeDeclaredNamesUnique implements NodeTraversal.ScopedCallback {
// In addition, ES6 introduced block scopes, which we also need to handle.
private final Deque<Renamer> nameStack = new ArrayDeque<>();
private final Renamer rootRenamer;
private final boolean markChanges;

MakeDeclaredNamesUnique() {
this(new ContextualRenamer());
this(new ContextualRenamer(), true);
}

MakeDeclaredNamesUnique(Renamer renamer) {
this(renamer, true);
}

MakeDeclaredNamesUnique(Renamer renamer, boolean markChanges) {
this.rootRenamer = renamer;
this.markChanges = markChanges;
}

static CompilerPass getContextualRenameInverter(AbstractCompiler compiler) {
return new ContextualRenameInverter(compiler);
return new ContextualRenameInverter(compiler, true);
}

@Override
Expand Down Expand Up @@ -186,6 +193,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
break;
}
}

private void visitName(NodeTraversal t, Node n, Node parent) {
String newName = getReplacementName(n.getString());
if (newName != null) {
Expand All @@ -195,11 +203,13 @@ private void visitName(NodeTraversal t, Node n, Node parent) {
n.removeProp(Node.IS_CONSTANT_NAME);
}
n.setString(newName);
t.reportCodeChange();
// If we are renaming a function declaration, make sure the containing scope
// has the opporunity to act on the change.
if (parent.isFunction() && NodeUtil.isFunctionDeclaration(parent)) {
t.getCompiler().reportChangeToEnclosingScope(parent);
if (markChanges) {
t.reportCodeChange();
// If we are renaming a function declaration, make sure the containing scope
// has the opporunity to act on the change.
if (parent.isFunction() && NodeUtil.isFunctionDeclaration(parent)) {
t.getCompiler().reportChangeToEnclosingScope(parent);
}
}
}
}
Expand Down Expand Up @@ -301,8 +311,12 @@ static class ContextualRenameInverter
private final ListMultimap<String, Node> nameMap =
MultimapBuilder.hashKeys().arrayListValues().build();

private ContextualRenameInverter(AbstractCompiler compiler) {
// Whether to report changes to the compiler.
private final boolean markChanges;

private ContextualRenameInverter(AbstractCompiler compiler, boolean markChanges) {
this.compiler = compiler;
this.markChanges = markChanges;
}

@Override
Expand Down Expand Up @@ -379,12 +393,14 @@ void handleScopeVar(Var v) {
for (Node n : references) {
checkState(n.isName(), n);
n.setString(newName);
compiler.reportChangeToEnclosingScope(n);
Node parent = n.getParent();
// If we are renaming a function declaration, make sure the containing scope
// has the opportunity to act on the change.
if (parent.isFunction() && NodeUtil.isFunctionDeclaration(parent)) {
compiler.reportChangeToEnclosingScope(parent);
if (markChanges) {
compiler.reportChangeToEnclosingScope(n);
Node parent = n.getParent();
// If we are renaming a function declaration, make sure the containing scope
// has the opportunity to act on the change.
if (parent.isFunction() && NodeUtil.isFunctionDeclaration(parent)) {
compiler.reportChangeToEnclosingScope(parent);
}
}
}
nameMap.removeAll(name);
Expand Down
25 changes: 23 additions & 2 deletions src/com/google/javascript/jscomp/NodeTraversal.java
View file
Expand Up @@ -432,22 +432,43 @@ private void traverseScopeRoot(Node scopeRoot) {
}

/**
* Traverses *just* the contents of provided scope nodes (but not scopes nested within them) but
* will fall back on traversing the entire AST from root if a null scope nodes list is provided.
* Traverses *just* the contents of provided scope nodes (and optionally scopes nested within
* them) but will fall back on traversing the entire AST from root if a null scope nodes list is
* provided.
*/
public static void traverseEs6ScopeRoots(
AbstractCompiler compiler,
Node root,
List<Node> scopeNodes,
final Callback cb,
final boolean traverseNested) {
traverseEs6ScopeRoots(compiler, root, scopeNodes, cb, null, traverseNested);
}

/**
* Traverses *just* the contents of provided scope nodes (and optionally scopes nested within
* them) but will fall back on traversing the entire AST from root if a null scope nodes list is
* provided. Also allows for a callback to notify when starting on one of the provided scope
* nodes.
*/
public static void traverseEs6ScopeRoots(
AbstractCompiler compiler,
Node root,
List<Node> scopeNodes,
final Callback cb,
final FunctionCallback fcb,
final boolean traverseNested) {
if (scopeNodes == null) {
NodeTraversal.traverseEs6(compiler, root, cb);
} else {
MemoizedScopeCreator scopeCreator =
new MemoizedScopeCreator(new Es6SyntacticScopeCreator(compiler));

for (final Node scopeNode : scopeNodes) {
if (fcb != null) {
fcb.enterFunction(compiler, scopeNode);
}

Callback scb =
new Callback() {
@Override
Expand Down
40 changes: 40 additions & 0 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -43,6 +43,7 @@
import java.util.Collections;
import java.util.EnumSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -2647,6 +2648,17 @@ static boolean isInSyntheticScript(Node n) {
return n.getSourceFileName() != null && n.getSourceFileName().startsWith(" [synthetic:");
}

/**
* Removes all children from this node, isolates the children from each other and reports any
* nested functions as deleted.
*/
public static void detachChildren(Node n, AbstractCompiler compiler) {
for (Node child = n.getFirstChild(); child != null; child = child.getNext()) {
NodeUtil.markFunctionsDeleted(child, compiler);
}
n.detachChildren();
}

/**
* Safely remove children while maintaining a valid node structure.
* In some cases, this is done by removing the parent from the AST as well.
Expand Down Expand Up @@ -4931,4 +4943,32 @@ static void markFunctionsDeleted(Node node, AbstractCompiler compiler) {
markFunctionsDeleted(child, compiler);
}
}

/** Returns the list of scope nodes which are parents of the provided list of scope nodes. */
public static List<Node> getParentChangeScopeNodes(List<Node> scopeNodes) {
Set<Node> parentScopeNodes = new LinkedHashSet<>(scopeNodes);
for (Node scopeNode : scopeNodes) {
parentScopeNodes.add(getEnclosingChangeScopeRoot(scopeNode));
}
return new ArrayList<>(parentScopeNodes);
}

/**
* Removes any scope nodes from the provided list that are nested within some other scope node
* also in the list. Returns the modified list.
*/
public static List<Node> removeNestedChangeScopeNodes(List<Node> scopeNodes) {
Set<Node> uniqueScopeNodes = new LinkedHashSet<>(scopeNodes);
for (Node scopeNode : scopeNodes) {
for (Node ancestor = scopeNode.getParent();
ancestor != null;
ancestor = ancestor.getParent()) {
if (isChangeScopeRoot(ancestor) && uniqueScopeNodes.contains(ancestor)) {
uniqueScopeNodes.remove(scopeNode);
break;
}
}
}
return new ArrayList<>(uniqueScopeNodes);
}
}

0 comments on commit a9f0ce0

Please sign in to comment.