Skip to content

Commit

Permalink
Makes DefinitionUseSiteFinder incrementally updateable.
Browse files Browse the repository at this point in the history 
Does not yet wrap it into the the persistent datastructure
framework, that comes in the following CL.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=161996924
  • Loading branch information
@stalcup @blickly
stalcup authored and blickly committed Jul 18, 2017
1 parent a643b8c commit 6c7c763
Show file tree
Hide file tree
Showing 9 changed files with 298 additions and 127 deletions.
12 changes: 6 additions & 6 deletions src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java
View file
Expand Up @@ -24,7 +24,7 @@
import com.google.common.collect.PeekingIterator;
import com.google.common.collect.Sets;
import com.google.javascript.jscomp.NodeTraversal.Callback;
import com.google.javascript.jscomp.NodeTraversal.FunctionCallback;
import com.google.javascript.jscomp.NodeTraversal.ChangeScopeRootCallback;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token;
import java.util.HashMap;
Expand Down Expand Up @@ -88,7 +88,7 @@ public void process(Node externs, Node root) {
NodeTraversal.traverseChangedFunctions(compiler, new FunctionVisitor(blacklistedPropNames));
}

private static class FunctionVisitor implements FunctionCallback {
private static class FunctionVisitor implements ChangeScopeRootCallback {

/**
* A set of properties names that are potentially unsafe to remove duplicate writes to.
Expand All @@ -100,18 +100,18 @@ private static class FunctionVisitor implements FunctionCallback {
}

@Override
public void enterFunction(AbstractCompiler compiler, Node n) {
if (!n.isFunction()) {
public void enterChangeScopeRoot(AbstractCompiler compiler, Node root) {
if (!root.isFunction()) {
return;
}

Node body = NodeUtil.getFunctionBody(n);
Node body = NodeUtil.getFunctionBody(root);
if (!body.hasChildren() || NodeUtil.containsFunction(body)) {
return;
}

FindCandidateAssignmentTraversal traversal =
new FindCandidateAssignmentTraversal(blacklistedPropNames, NodeUtil.isConstructor(n));
new FindCandidateAssignmentTraversal(blacklistedPropNames, NodeUtil.isConstructor(root));
NodeTraversal.traverseEs6(compiler, body, traversal);

// Any candidate property assignment can have a write removed if that write is never read
Expand Down
102 changes: 67 additions & 35 deletions src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java
View file
Expand Up @@ -20,40 +20,56 @@
import static com.google.common.base.Preconditions.checkState;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.Multimap;
import com.google.javascript.jscomp.DefinitionsRemover.Definition;
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
import com.google.javascript.rhino.Node;
import java.util.Collection;
import java.util.List;

/**
* Built on top of the {@link NameBasedDefinitionProvider}, this class additionally collects the
* use sites for each definition. It is useful for constructing a full reference graph of the entire
* Built on top of the {@link NameBasedDefinitionProvider}, this class additionally collects the use
* sites for each definition. It is useful for constructing a full reference graph of the entire
* ast.
*
*/
// TODO(stalcup): track useSites in lhs of GET_ELEM nodes as well.
public class DefinitionUseSiteFinder extends NameBasedDefinitionProvider {

private final Multimap<String, UseSite> nameUseSiteMultimap;
private static class NameAndUseSite {
final String name;
final UseSite useSite;

NameAndUseSite(String name, UseSite useSite) {
this.name = name;
this.useSite = useSite;
}
}

private final Multimap<String, UseSite> useSitesByName;
// Remember which UseSite instances are in which scopes, so that the knowledge about a changing
// scope can be rebuilt later.
private final Multimap<Node, NameAndUseSite> useSitesByScopeNode;

@VisibleForTesting
Multimap<String, UseSite> getNameUseSiteMultimap() {
Multimap<String, UseSite> getUseSitesByName() {
// Defensive copy.
return LinkedHashMultimap.create(nameUseSiteMultimap);
return LinkedHashMultimap.create(useSitesByName);
}

public DefinitionUseSiteFinder(AbstractCompiler compiler) {
super(compiler, false);
this.nameUseSiteMultimap = LinkedHashMultimap.create();
this.useSitesByName = LinkedHashMultimap.create();
this.useSitesByScopeNode = HashMultimap.create();
}

@Override
public void process(Node externs, Node source) {
super.process(externs, source);
NodeTraversal.traverseEs6(
compiler, source, new UseSiteGatheringCallback());
NodeTraversal.traverseEs6(compiler, source, new UseSiteGatheringCallback());
}

/**
Expand All @@ -64,9 +80,8 @@ public void process(Node externs, Node source) {
* @return use site collection.
*/
public Collection<UseSite> getUseSites(Definition definition) {
checkState(hasProcessBeenRun, "The process was not run");
String name = getSimplifiedName(definition.getLValue());
return nameUseSiteMultimap.get(name);
checkState(hasProcessBeenRun, "Hasn't been initialized with process() yet.");
return useSitesByName.get(definition.getSimplifiedName());
}

private class UseSiteGatheringCallback extends AbstractPostOrderCallback {
Expand All @@ -85,9 +100,10 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {

String name = getSimplifiedName(first.getLValue());
checkNotNull(name);
nameUseSiteMultimap.put(
name,
new UseSite(node, traversal.getScope(), traversal.getModule()));
UseSite useSite = new UseSite(node, traversal.getScope(), traversal.getModule());
useSitesByName.put(name, useSite);
useSitesByScopeNode.put(
NodeUtil.getEnclosingChangeScopeRoot(node), new NameAndUseSite(name, useSite));
}
}

Expand Down Expand Up @@ -129,8 +145,7 @@ boolean canModifyDefinition(Definition definition) {
// mechanisms.

Node nameNode = site.node;
Collection<Definition> singleSiteDefinitions =
getDefinitionsReferencedAt(nameNode);
Collection<Definition> singleSiteDefinitions = getDefinitionsReferencedAt(nameNode);
if (singleSiteDefinitions.size() > 1) {
return false;
}
Expand All @@ -142,9 +157,7 @@ boolean canModifyDefinition(Definition definition) {
return true;
}

/**
* @return Whether the definition is directly exported.
*/
/** @return Whether the definition is directly exported. */
private boolean isExported(Definition definition) {
// Assume an exported method result is used.
Node lValue = definition.getLValue();
Expand All @@ -167,32 +180,51 @@ private boolean isExported(Definition definition) {
return codingConvention.isExported(partialName);
}

/**
* Traverse a node and its children and remove any references to from
* the structures.
*/
@Override
public void rebuildScopeRoots(List<Node> changedScopeRoots, List<Node> deletedScopeRoots) {
super.rebuildScopeRoots(changedScopeRoots, deletedScopeRoots);

for (Node scopeRoot : Iterables.concat(deletedScopeRoots, changedScopeRoots)) {
for (NameAndUseSite nameAndUseSite : useSitesByScopeNode.removeAll(scopeRoot)) {
useSitesByName.remove(nameAndUseSite.name, nameAndUseSite.useSite);
}
}

NodeTraversal.traverseEs6ScopeRoots(
compiler, null, changedScopeRoots, new UseSiteGatheringCallback(), false);
}

/** Traverse a node and its children and remove any references to from the structures. */
void removeReferences(Node node) {
if (DefinitionsRemover.isDefinitionNode(node)) {
DefinitionSite defSite = definitionNodeByDefinitionSite.get(node);
if (defSite != null) {
Definition def = defSite.definition;
String name = getSimplifiedName(def.getLValue());
Node definitionSiteNode = node;
DefinitionSite definitionSite = definitionSitesByDefinitionSiteNode.get(definitionSiteNode);
if (definitionSite != null) {
Definition definition = definitionSite.definition;
String name = definition.getSimplifiedName();
if (name != null) {
this.definitionNodeByDefinitionSite.remove(node);
this.nameDefinitionMultimap.remove(name, def);
Node definitionNode = definition.getLValue();
definitionNodes.remove(definitionNode);
definitionsByName.remove(name, definition);
definitionSitesByDefinitionSiteNode.remove(definitionSiteNode);
Node scopeNode = NodeUtil.getEnclosingChangeScopeRoot(definitionSiteNode);
definitionSitesByScopeNode.remove(scopeNode, definitionSite);
}
}
} else {
Node useSite = node;
if (useSite.isGetProp()) {
String propName = useSite.getLastChild().getString();
Node useSiteNode = node;
if (useSiteNode.isGetProp()) {
String propName = useSiteNode.getLastChild().getString();
if (propName.equals("apply") || propName.equals("call")) {
useSite = useSite.getFirstChild();
useSiteNode = useSiteNode.getFirstChild();
}
}
String name = getSimplifiedName(useSite);
String name = getSimplifiedName(useSiteNode);
if (name != null) {
this.nameUseSiteMultimap.remove(name, new UseSite(useSite, null, null));
UseSite useSite = new UseSite(useSiteNode, null, null);
useSitesByName.remove(name, useSite);
useSitesByScopeNode.remove(
NodeUtil.getEnclosingChangeScopeRoot(useSiteNode), new NameAndUseSite(name, useSite));
}
}

Expand Down
24 changes: 17 additions & 7 deletions src/com/google/javascript/jscomp/DefinitionsRemover.java
View file
Expand Up @@ -130,9 +130,11 @@ static boolean isDefinitionNode(Node n) {
abstract static class Definition {

private final boolean isExtern;
private final String simplifiedName;

Definition(boolean isExtern) {
Definition(boolean isExtern, String simplifiedName) {
this.isExtern = isExtern;
this.simplifiedName = simplifiedName;
}

/**
Expand All @@ -154,6 +156,10 @@ public void remove(AbstractCompiler compiler) {
*/
protected abstract void performRemove(AbstractCompiler compiler);

public String getSimplifiedName() {
return simplifiedName;
}

/**
* Variable or property name represented by this definition.
* For example, in the case of assignments this method would
Expand Down Expand Up @@ -194,7 +200,7 @@ abstract static class IncompleteDefinition extends Definition {
private final Node lValue;

IncompleteDefinition(Node lValue, boolean inExterns) {
super(inExterns);
super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(lValue));
checkNotNull(lValue);

Preconditions.checkArgument(
Expand Down Expand Up @@ -273,7 +279,7 @@ abstract static class FunctionDefinition extends Definition {
protected final Node function;

FunctionDefinition(Node node, boolean inExterns) {
super(inExterns);
super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(node.getFirstChild()));
checkArgument(node.isFunction());
function = node;
}
Expand Down Expand Up @@ -357,7 +363,7 @@ abstract static class ClassDefinition extends Definition {
protected final Node c;

ClassDefinition(Node node, boolean inExterns) {
super(inExterns);
super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(node.getFirstChild()));
Preconditions.checkArgument(node.isClass());
c = node;
}
Expand Down Expand Up @@ -414,7 +420,7 @@ static final class AssignmentDefinition extends Definition {
private final Node assignment;

AssignmentDefinition(Node node, boolean inExterns) {
super(inExterns);
super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(node.getFirstChild()));
checkArgument(node.isAssign());
assignment = node;
}
Expand Down Expand Up @@ -470,7 +476,7 @@ static final class ObjectLiteralPropertyDefinition extends Definition {

ObjectLiteralPropertyDefinition(Node lit, Node name, Node value,
boolean isExtern) {
super(isExtern);
super(isExtern, NameBasedDefinitionProvider.getSimplifiedName(getLValue(name)));

this.literal = lit;
this.name = name;
Expand All @@ -485,6 +491,10 @@ public void performRemove(AbstractCompiler compiler) {

@Override
public Node getLValue() {
return getLValue(name);
}

private static Node getLValue(Node name) {
// TODO(user) revisit: object literal definitions are an example
// of definitions whose LHS doesn't correspond to a node that
// exists in the AST. We will have to change the return type of
Expand Down Expand Up @@ -516,7 +526,7 @@ public Node getRValue() {
static final class VarDefinition extends Definition {
private final Node name;
VarDefinition(Node node, boolean inExterns) {
super(inExterns);
super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(node));
checkArgument(NodeUtil.isVarDeclaration(node));
Preconditions.checkArgument(inExterns || node.hasChildren(),
"VAR Declaration of %s must be assigned a value.", node.getString());
Expand Down
6 changes: 3 additions & 3 deletions src/com/google/javascript/jscomp/J2clClinitPrunerPass.java
View file
Expand Up @@ -20,7 +20,7 @@
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.jscomp.NodeTraversal.ChangeScopeRootCallback;
import com.google.javascript.rhino.Node;
import java.util.ArrayDeque;
import java.util.Deque;
Expand Down Expand Up @@ -76,10 +76,10 @@ private List<Node> getNonNestedParentScopeNodes() {
}

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

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

0 comments on commit 6c7c763

Please sign in to comment.