Skip to content

Commit

Permalink
SimpleDefinitionFinder is *not* simple and does more than it needs to…
Browse files Browse the repository at this point in the history
… in some cases.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=128513771
  • Loading branch information
tadeegan authored and blickly committed Jul 26, 2016
1 parent b36981e commit df94bcc
Show file tree
Hide file tree
Showing 23 changed files with 560 additions and 562 deletions.
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/AbstractCompiler.java
Expand Up @@ -186,9 +186,9 @@ public abstract class AbstractCompiler implements SourceExcerptProvider {
* optimize-parameters, remove-unused-variables), to avoid having them
* recompute it independently.
*/
abstract SimpleDefinitionFinder getSimpleDefinitionFinder();
abstract DefinitionUseSiteFinder getSimpleDefinitionFinder();

abstract void setSimpleDefinitionFinder(SimpleDefinitionFinder defFinder);
abstract void setSimpleDefinitionFinder(DefinitionUseSiteFinder defFinder);

/**
* Parses code for injecting.
Expand Down
10 changes: 5 additions & 5 deletions src/com/google/javascript/jscomp/CallGraph.java
Expand Up @@ -138,12 +138,12 @@ public CallGraph(AbstractCompiler compiler) {
public void process(Node externsRoot, Node jsRoot) {
Preconditions.checkState(!alreadyRun);

SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler);
defFinder.process(externsRoot, jsRoot);
DefinitionUseSiteFinder definitionProvider = new DefinitionUseSiteFinder(compiler);
definitionProvider.process(externsRoot, jsRoot);

createFunctionsAndCallsites(jsRoot, defFinder);
createFunctionsAndCallsites(jsRoot, definitionProvider);

fillInFunctionInformation(defFinder);
fillInFunctionInformation(definitionProvider);

alreadyRun = true;
}
Expand Down Expand Up @@ -338,7 +338,7 @@ private void connectCallsiteToTargets(Callsite callsite,
* <p>We do this here, rather than when connecting the callgraph, to make sure that we have
* correct information for all functions, rather than just functions that are actually called.
*/
private void fillInFunctionInformation(SimpleDefinitionFinder finder) {
private void fillInFunctionInformation(DefinitionUseSiteFinder finder) {
for (DefinitionSite definitionSite : finder.getDefinitionSites()) {
Definition definition = definitionSite.definition;

Expand Down
5 changes: 2 additions & 3 deletions src/com/google/javascript/jscomp/ChainCalls.java
Expand Up @@ -21,7 +21,6 @@
import com.google.javascript.jscomp.NodeTraversal.ScopedCallback;
import com.google.javascript.jscomp.graph.DiGraph.DiGraphEdge;
import com.google.javascript.rhino.Node;

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
Expand All @@ -37,7 +36,7 @@ class ChainCalls implements CompilerPass {
private final Set<Node> badFunctionNodes = new HashSet<>();
private final Set<Node> goodFunctionNodes = new HashSet<>();
private final List<CallSite> callSites = new ArrayList<>();
private SimpleDefinitionFinder defFinder;
private NameBasedDefinitionProvider defFinder;
private GatherFunctions gatherFunctions = new GatherFunctions();

ChainCalls(AbstractCompiler compiler) {
Expand All @@ -46,7 +45,7 @@ class ChainCalls implements CompilerPass {

@Override
public void process(Node externs, Node root) {
defFinder = new SimpleDefinitionFinder(compiler);
defFinder = new NameBasedDefinitionProvider(compiler);
defFinder.process(externs, root);

NodeTraversal.traverseEs6(compiler, root, new GatherCallSites());
Expand Down
6 changes: 3 additions & 3 deletions src/com/google/javascript/jscomp/Compiler.java
Expand Up @@ -210,7 +210,7 @@ public SourceFile apply(String filename) {
public PerformanceTracker tracker;

// Used by optimize-returns, optimize-parameters and remove-unused-variables
private SimpleDefinitionFinder defFinder = null;
private DefinitionUseSiteFinder defFinder = null;

// For use by the new type inference
private GlobalTypeInfo symbolTable;
Expand Down Expand Up @@ -1346,12 +1346,12 @@ void setSymbolTable(CompilerPass symbolTable) {
}

@Override
SimpleDefinitionFinder getSimpleDefinitionFinder() {
DefinitionUseSiteFinder getSimpleDefinitionFinder() {
return this.defFinder;
}

@Override
void setSimpleDefinitionFinder(SimpleDefinitionFinder defFinder) {
void setSimpleDefinitionFinder(DefinitionUseSiteFinder defFinder) {
this.defFinder = defFinder;
}

Expand Down
194 changes: 194 additions & 0 deletions src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java
@@ -0,0 +1,194 @@
/*
* Copyright 2009 The Closure Compiler Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.javascript.jscomp;

import com.google.common.base.Preconditions;
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;

/**
* 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.
*
*/
public class DefinitionUseSiteFinder extends NameBasedDefinitionProvider {


private final Multimap<String, UseSite> nameUseSiteMultimap;

public DefinitionUseSiteFinder(AbstractCompiler compiler) {
super(compiler);
this.nameUseSiteMultimap = LinkedHashMultimap.create();
}

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

/**
* Returns a collection of use sites that may refer to provided definition. Returns an empty
* collection if the definition is not used anywhere.
*
* @param definition Definition of interest.
* @return use site collection.
*/
public Collection<UseSite> getUseSites(Definition definition) {
Preconditions.checkState(hasProcessBeenRun, "The process was not run");
String name = getSimplifiedName(definition.getLValue());
return nameUseSiteMultimap.get(name);
}

private class UseSiteGatheringCallback extends AbstractPostOrderCallback {
@Override
public void visit(NodeTraversal traversal, Node node, Node parent) {

Collection<Definition> defs = getDefinitionsReferencedAt(node);
if (defs == null) {
return;
}

Definition first = defs.iterator().next();

String name = getSimplifiedName(first.getLValue());
Preconditions.checkNotNull(name);
nameUseSiteMultimap.put(
name,
new UseSite(node, traversal.getScope(), traversal.getModule()));
}
}

/**
* @param use A use site to check.
* @return Whether the use is a call or new.
*/
static boolean isCallOrNewSite(UseSite use) {
Node call = use.node.getParent();
if (call == null) {
// The node has been removed from the AST.
return false;
}
// We need to make sure we're dealing with a call to the function we're
// optimizing. If the the first child of the parent is not the site, this
// is a nested call and it's a call to another function.
return NodeUtil.isCallOrNew(call) && call.getFirstChild() == use.node;
}

boolean canModifyDefinition(Definition definition) {
if (isExported(definition)) {
return false;
}

// Don't modify unused definitions for two reasons:
// 1) It causes unnecessary churn
// 2) Other definitions might be used to reflect on this one using
// goog.reflect.object (the check for definitions with uses is below).
Collection<UseSite> useSites = getUseSites(definition);
if (useSites.isEmpty()) {
return false;
}

for (UseSite site : useSites) {
// This catches the case where an object literal in goog.reflect.object
// and a prototype method have the same property name.

// NOTE(nicksantos): Maps and trogedit both do this by different
// mechanisms.

Node nameNode = site.node;
Collection<Definition> singleSiteDefinitions =
getDefinitionsReferencedAt(nameNode);
if (singleSiteDefinitions.size() > 1) {
return false;
}

Preconditions.checkState(!singleSiteDefinitions.isEmpty());
Preconditions.checkState(singleSiteDefinitions.contains(definition));
}

return true;
}

/**
* @return Whether the definition is directly exported.
*/
private boolean isExported(Definition definition) {
// Assume an exported method result is used.
Node lValue = definition.getLValue();
if (lValue == null) {
return true;
}

String partialName;
if (lValue.isGetProp()) {
partialName = lValue.getLastChild().getString();
} else if (lValue.isName()) {
partialName = lValue.getString();
} else {
// GETELEM is assumed to be an export or other expression are unknown
// uses.
return true;
}

CodingConvention codingConvention = compiler.getCodingConvention();
return codingConvention.isExported(partialName);
}

/**
* Traverse a node and its children and remove any references to from
* the structures.
*/
void removeReferences(Node node) {
if (DefinitionsRemover.isDefinitionNode(node)) {
DefinitionSite defSite = definitionSiteMap.get(node);
if (defSite != null) {
Definition def = defSite.definition;
String name = getSimplifiedName(def.getLValue());
if (name != null) {
this.definitionSiteMap.remove(node);
this.nameDefinitionMultimap.remove(name, node);
}
}
} else {
Node useSite = node;
if (useSite.isGetProp()) {
String propName = useSite.getLastChild().getString();
if (propName.equals("apply") || propName.equals("call")) {
useSite = useSite.getFirstChild();
}
}
String name = getSimplifiedName(useSite);
if (name != null) {
this.nameUseSiteMultimap.remove(name, new UseSite(useSite, null, null));
}
}

for (Node child : node.children()) {
removeReferences(child);
}
}
}
Expand Up @@ -68,14 +68,14 @@ class DevirtualizePrototypeMethods

@Override
public void process(Node externs, Node root) {
SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler);
DefinitionUseSiteFinder defFinder = new DefinitionUseSiteFinder(compiler);
defFinder.process(externs, root);
process(externs, root, defFinder);
}

@Override
public void process(
Node externs, Node root, SimpleDefinitionFinder definitions) {
Node externs, Node root, DefinitionUseSiteFinder definitions) {
for (DefinitionSite defSite : definitions.getDefinitionSites()) {
rewriteDefinitionIfEligible(defSite, definitions);
}
Expand Down Expand Up @@ -171,14 +171,14 @@ private static String getRewrittenMethodName(String originalMethodName) {
* defined in the global scope exactly once.
*
* Definition and use site information is provided by the
* {@link SimpleDefinitionFinder} passed in as an argument.
* {@link DefinitionUseSiteFinder} passed in as an argument.
*
* @param defSite definition site to process.
* @param defFinder structure that hold Node -> Definition and
* Definition -> [UseSite] maps.
*/
private void rewriteDefinitionIfEligible(DefinitionSite defSite,
SimpleDefinitionFinder defFinder) {
DefinitionUseSiteFinder defFinder) {
if (defSite.inExterns ||
!defSite.inGlobalScope ||
!isEligibleDefinition(defFinder, defSite)) {
Expand Down Expand Up @@ -220,7 +220,7 @@ private void rewriteDefinitionIfEligible(DefinitionSite defSite,
* choice at each call site.
* - Definition must happen in a module loaded before the first use.
*/
private boolean isEligibleDefinition(SimpleDefinitionFinder defFinder,
private boolean isEligibleDefinition(DefinitionUseSiteFinder defFinder,
DefinitionSite definitionSite) {

Definition definition = definitionSite.definition;
Expand Down Expand Up @@ -305,7 +305,7 @@ private boolean isEligibleDefinition(SimpleDefinitionFinder defFinder,
* After:
* foo(o, a, b, c)
*/
private void rewriteCallSites(SimpleDefinitionFinder defFinder,
private void rewriteCallSites(DefinitionUseSiteFinder defFinder,
Definition definition,
String newMethodName) {
Collection<UseSite> useSites = defFinder.getUseSites(definition);
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/InlineFunctions.java
Expand Up @@ -590,7 +590,7 @@ private void checkNameUsage(Node n, Node parent) {
if (parent.isNew()) {
Node target = parent.getFirstChild();
if (target.isName() && target.getString().equals(
SimpleDefinitionFinder.EXTERN_OBJECT_PROPERTY_STRING)) {
NodeUtil.EXTERN_OBJECT_PROPERTY_STRING)) {
// This method is going to be replaced so don't inline it anywhere.
fs.setInline(false);
}
Expand Down
7 changes: 3 additions & 4 deletions src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java
Expand Up @@ -21,7 +21,6 @@
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node;

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
Expand Down Expand Up @@ -50,7 +49,7 @@ class MarkNoSideEffectCalls implements CompilerPass {

@Override
public void process(Node externs, Node root) {
SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler);
NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler);
defFinder.process(externs, root);

// Gather the list of function nodes that have @nosideeffects annotations.
Expand Down Expand Up @@ -158,9 +157,9 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
* refer to function names that are known to have no side effects.
*/
private class SetNoSideEffectCallProperty extends AbstractPostOrderCallback {
private final SimpleDefinitionFinder defFinder;
private final NameBasedDefinitionProvider defFinder;

SetNoSideEffectCallProperty(SimpleDefinitionFinder defFinder) {
SetNoSideEffectCallProperty(NameBasedDefinitionProvider defFinder) {
this.defFinder = defFinder;
}

Expand Down

0 comments on commit df94bcc

Please sign in to comment.