From 4cf03c7b60f185992968ade7446ce47ab1ae978b Mon Sep 17 00:00:00 2001 From: lukes Date: Mon, 5 Dec 2016 10:39:50 -0800 Subject: [PATCH] Optimize NameAnalyzer by switching the TreeMap to a HashMap also optimize 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 --- .../javascript/jscomp/NameAnalyzer.java | 16 +++++---- .../jscomp/graph/LinkedDirectedGraph.java | 35 +++++++++++-------- 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/src/com/google/javascript/jscomp/NameAnalyzer.java b/src/com/google/javascript/jscomp/NameAnalyzer.java index 9c4a51ed375..ff378873533 100644 --- a/src/com/google/javascript/jscomp/NameAnalyzer.java +++ b/src/com/google/javascript/jscomp/NameAnalyzer.java @@ -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; @@ -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; @@ -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. a) or @@ -88,7 +87,7 @@ final class NameAnalyzer implements CompilerPass { private final AbstractCompiler compiler; /** Map of all JS names found */ - private final Map allNames = new TreeMap<>(); + private final Map allNames = new HashMap<>(); /** Reference dependency graph */ private LinkedDirectedGraph referenceGraph = @@ -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; } @@ -1280,7 +1280,8 @@ String getHtmlReport() { sb.append(""); sb.append("ALL NAMES
    \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("
  • ").append(nameAnchor(node.name)).append("
      "); if (!node.prototypeNames.isEmpty()) { sb.append("
    • PROTOTYPES: "); @@ -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); } @@ -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; } /** @@ -1450,7 +1452,7 @@ private DiGraphNode getGraphNode(JsName name) { private void referenceParentNames() { // Duplicate set of nodes to process so we don't modify set we are // currently iterating over - Set allNamesCopy = new HashSet<>(allNames.values()); + JsName[] allNamesCopy = allNames.values().toArray(new JsName[0]); for (JsName name : allNamesCopy) { String curName = name.name; diff --git a/src/com/google/javascript/jscomp/graph/LinkedDirectedGraph.java b/src/com/google/javascript/jscomp/graph/LinkedDirectedGraph.java index dac34a3b008..946d86ac5a1 100644 --- a/src/com/google/javascript/jscomp/graph/LinkedDirectedGraph.java +++ b/src/com/google/javascript/jscomp/graph/LinkedDirectedGraph.java @@ -237,12 +237,26 @@ public boolean isConnectedInDirection( DiGraphNode dNode2) { // Verify the nodes. List> outEdges = dNode1.getOutEdges(); - int len = outEdges.size(); - for (int i = 0; i < len; i++) { - DiGraphEdge outEdge = outEdges.get(i); - if (outEdge.getDestination() == dNode2 - && edgeMatcher.apply(outEdge.getValue())) { - return true; + int outEdgesLen = outEdges.size(); + List> 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 outEdge = outEdges.get(i); + if (outEdge.getDestination() == dNode2 + && edgeMatcher.apply(outEdge.getValue())) { + return true; + } + } + } else { + for (int i = 0; i < inEdgesLen; i++) { + DiGraphEdge inEdge = inEdges.get(i); + if (inEdge.getSource() == dNode1 + && edgeMatcher.apply(inEdge.getValue())) { + return true; + } } } @@ -253,14 +267,7 @@ private boolean isConnectedInDirection(N n1, Predicate edgeMatcher, N n2) { // Verify the nodes. DiGraphNode dNode1 = getNodeOrFail(n1); DiGraphNode dNode2 = getNodeOrFail(n2); - for (DiGraphEdge outEdge : dNode1.getOutEdges()) { - if (outEdge.getDestination() == dNode2 - && edgeMatcher.apply(outEdge.getValue())) { - return true; - } - } - - return false; + return isConnectedInDirection(dNode1, edgeMatcher, dNode2); } @Override