Skip to content

Commit

Permalink
Optimize NameAnalyzer by switching the TreeMap to a HashMap also opti…
Browse files Browse the repository at this point in the history
…mize

LinkDirectedGraph.isDirectlyConnected

The ordering property of the treemap appears to only be useful when generating
the html report, but presumably that isn't common, whereas get() and .put()
are.  So instead we do an explicit sort operation in the report generation.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=141069570
  • Loading branch information
lukesandberg authored and blickly committed Dec 6, 2016
1 parent b00f7ef commit 4cf03c7
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 21 deletions.
16 changes: 9 additions & 7 deletions src/com/google/javascript/jscomp/NameAnalyzer.java
Expand Up @@ -26,6 +26,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Ordering;
import com.google.common.io.Files;
import com.google.javascript.jscomp.CodingConvention.SubclassRelationship;
import com.google.javascript.jscomp.GatherSideEffectSubexpressionsCallback.GetReplacementSideEffectSubexpressions;
Expand All @@ -37,7 +38,6 @@
import com.google.javascript.jscomp.graph.LinkedDirectedGraph;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;

import java.io.File;
import java.io.IOException;
import java.util.ArrayDeque;
Expand All @@ -50,7 +50,6 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;

/**
* This pass identifies all global names, simple (e.g. <code>a</code>) or
Expand Down Expand Up @@ -88,7 +87,7 @@ final class NameAnalyzer implements CompilerPass {
private final AbstractCompiler compiler;

/** Map of all JS names found */
private final Map<String, JsName> allNames = new TreeMap<>();
private final Map<String, JsName> allNames = new HashMap<>();

/** Reference dependency graph */
private LinkedDirectedGraph<JsName, RefType> referenceGraph =
Expand Down Expand Up @@ -204,6 +203,7 @@ private static class NameInformation {
/** Whether this is a call that only affects the class definition */
boolean onlyAffectsClassDef = false;

@Override
public String toString() {
return "NameInformation:" + name;
}
Expand Down Expand Up @@ -1280,7 +1280,8 @@ String getHtmlReport() {
sb.append("</ul>");

sb.append("ALL NAMES<ul>\n");
for (JsName node : allNames.values()) {
// Sort before generating to ensure a consistent stable order
for (JsName node : Ordering.natural().sortedCopy(allNames.values())) {
sb.append("<li>").append(nameAnchor(node.name)).append("<ul>");
if (!node.prototypeNames.isEmpty()) {
sb.append("<li>PROTOTYPES: ");
Expand Down Expand Up @@ -1353,7 +1354,7 @@ private static String nameAnchor(String name) {
*/
private JsName getName(String name, boolean canCreate) {
if (canCreate) {
createName(name);
return createName(name);
}
return allNames.get(name);
}
Expand All @@ -1364,13 +1365,14 @@ private JsName getName(String name, boolean canCreate) {
*
* @param name A fully qualified name
*/
private void createName(String name) {
private JsName createName(String name) {
JsName jsn = allNames.get(name);
if (jsn == null) {
jsn = new JsName();
jsn.name = name;
allNames.put(name, jsn);
}
return jsn;
}

/**
Expand Down Expand Up @@ -1450,7 +1452,7 @@ private DiGraphNode<JsName, RefType> getGraphNode(JsName name) {
private void referenceParentNames() {
// Duplicate set of nodes to process so we don't modify set we are
// currently iterating over
Set<JsName> allNamesCopy = new HashSet<>(allNames.values());
JsName[] allNamesCopy = allNames.values().toArray(new JsName[0]);

for (JsName name : allNamesCopy) {
String curName = name.name;
Expand Down
35 changes: 21 additions & 14 deletions src/com/google/javascript/jscomp/graph/LinkedDirectedGraph.java
Expand Up @@ -237,12 +237,26 @@ public boolean isConnectedInDirection(
DiGraphNode<N, E> dNode2) {
// Verify the nodes.
List<DiGraphEdge<N, E>> outEdges = dNode1.getOutEdges();
int len = outEdges.size();
for (int i = 0; i < len; i++) {
DiGraphEdge<N, E> outEdge = outEdges.get(i);
if (outEdge.getDestination() == dNode2
&& edgeMatcher.apply(outEdge.getValue())) {
return true;
int outEdgesLen = outEdges.size();
List<DiGraphEdge<N, E>> inEdges = dNode2.getInEdges();
int inEdgesLen = inEdges.size();
// It is possible that there is a large assymmetry between the nodes, so pick the direction
// to search based on the shorter list since the edge lists should be symmetric.
if (outEdgesLen < inEdgesLen) {
for (int i = 0; i < outEdgesLen; i++) {
DiGraphEdge<N, E> outEdge = outEdges.get(i);
if (outEdge.getDestination() == dNode2
&& edgeMatcher.apply(outEdge.getValue())) {
return true;
}
}
} else {
for (int i = 0; i < inEdgesLen; i++) {
DiGraphEdge<N, E> inEdge = inEdges.get(i);
if (inEdge.getSource() == dNode1
&& edgeMatcher.apply(inEdge.getValue())) {
return true;
}
}
}

Expand All @@ -253,14 +267,7 @@ private boolean isConnectedInDirection(N n1, Predicate<E> edgeMatcher, N n2) {
// Verify the nodes.
DiGraphNode<N, E> dNode1 = getNodeOrFail(n1);
DiGraphNode<N, E> dNode2 = getNodeOrFail(n2);
for (DiGraphEdge<N, E> outEdge : dNode1.getOutEdges()) {
if (outEdge.getDestination() == dNode2
&& edgeMatcher.apply(outEdge.getValue())) {
return true;
}
}

return false;
return isConnectedInDirection(dNode1, edgeMatcher, dNode2);
}

@Override
Expand Down

0 comments on commit 4cf03c7

Please sign in to comment.