diff --git a/src/com/google/javascript/jscomp/CrossModuleCodeMotion.java b/src/com/google/javascript/jscomp/CrossModuleCodeMotion.java index 9f4ca5b167f..4e4b7ddb6a2 100644 --- a/src/com/google/javascript/jscomp/CrossModuleCodeMotion.java +++ b/src/com/google/javascript/jscomp/CrossModuleCodeMotion.java @@ -19,12 +19,12 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; -import com.google.common.collect.ImmutableList; import com.google.javascript.jscomp.CodingConvention.SubclassRelationship; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import java.util.ArrayDeque; +import java.util.BitSet; import java.util.Deque; import java.util.HashMap; import java.util.Iterator; @@ -105,10 +105,11 @@ private void moveCode() { if (info.shouldBeMoved()) { Iterator it = info.declarationIterator(); // Find the appropriate spot to move it to - Node destParent = moduleVarParentMap.get(info.preferredModule); + JSModule preferredModule = info.getPreferredModule(); + Node destParent = moduleVarParentMap.get(preferredModule); if (destParent == null) { - destParent = compiler.getNodeForCodeInsertion(info.preferredModule); - moduleVarParentMap.put(info.preferredModule, destParent); + destParent = compiler.getNodeForCodeInsertion(preferredModule); + moduleVarParentMap.put(preferredModule, destParent); } while (it.hasNext()) { Declaration decl = it.next(); @@ -131,7 +132,7 @@ private void moveCode() { } // Update variable declaration location. info.wasMoved = true; - info.declModule = info.preferredModule; + info.declModule = preferredModule; } } } @@ -140,9 +141,11 @@ private void moveCode() { private class NamedInfo { boolean allowMove = true; - // All modules containing references to this global variable directly or transitively depend - // on this module, which is potentially further down the tree than the module containing the - // variable's declaration statements. + // If movement is allowed, this will be filled with the indices of all modules referring to + // the global name. + private BitSet modulesWithReferences = null; + + // A place to stash the results of getPreferredModule() to avoid recalculating it unnecessarily. private JSModule preferredModule = null; // The module where declarations appear @@ -154,21 +157,51 @@ private class NamedInfo { private final Deque declarations = new ArrayDeque<>(); + boolean isAllowedToMove() { + return allowMove; + } + + void disallowMovement() { + allowMove = false; + // If we cannot move it, there's no point tracking where it's used. + modulesWithReferences = null; + preferredModule = declModule; + } + // Add a Module where it is used void addUsedModule(JSModule m) { - // If we are not allowed to move it, all bets are off - if (!allowMove) { - return; + // If we are not allowed to move it, don't waste time and space tracking modules with + // references. + if (allowMove) { + if (modulesWithReferences == null) { + // first call to this method + modulesWithReferences = new BitSet(graph.getModuleCount()); + } + modulesWithReferences.set(m.getIndex()); + // invalidate preferredModule, so it will be recalculated next time getPreferredModule() is + // called. + preferredModule = null; } + } - // If we have no preferred module yet, set this one + /** + * Returns the root module of a dependency subtree that contains all of the modules which refer + * to this global name. + */ + JSModule getPreferredModule() { + // It doesn't even make sense to call this method if the declarations cannot be moved. + checkState(allowMove); if (preferredModule == null) { - preferredModule = m; - } else { - // Find the deepest common dependency - preferredModule = - graph.getSmallestCoveringDependency(ImmutableList.of(m, preferredModule)); + if (modulesWithReferences == null) { + // If we saw no references, we must at least have seen a declaration. + preferredModule = checkNotNull(declModule); + } else { + // Note that getSmallestCoveringDependency() will do this: + // checkState(!modulesWithReferences.isEmpty()) + preferredModule = graph.getSmallestCoveringDependency(modulesWithReferences); + } } + return preferredModule; } /** @@ -190,13 +223,8 @@ boolean shouldBeMoved() { // Only move if all are true: // a) allowMove is true // b) it is declared somewhere (declModule != null) - // c) it was used somewhere (preferredModule != null) - // [if not, then it will be removed as dead or invalid code elsewhere] - // d) the all usages depend on the declModule by way of preferredModule - return allowMove - && preferredModule != null - && declModule != null - && graph.dependsOn(preferredModule, declModule); + // c) the all usages depend on the declModule by way of a different, preferred module + return allowMove && declModule != null && graph.dependsOn(getPreferredModule(), declModule); } /** @@ -310,12 +338,11 @@ private void collectReferences(Node root) { for (Var v : collector.getAllSymbols()) { NamedInfo info = getNamedInfo(v); - if (!info.allowMove) { - continue; - } - ReferenceCollection refCollection = collector.getReferences(v); - for (Reference ref : refCollection) { - processReference(collector, ref, info, v); + if (info.isAllowedToMove()) { + ReferenceCollection refCollection = collector.getReferences(v); + for (Reference ref : refCollection) { + processReference(collector, ref, info, v); + } } } } @@ -335,7 +362,7 @@ private void processReference( // we would need to skip the parent in this check as the name could // just be a function itself. if (hasConditionalAncestor(parent.getParent())) { - info.allowMove = false; + info.disallowMovement(); } } else { if (!parentModuleCanSeeSymbolsDeclaredInChildren) { @@ -584,9 +611,9 @@ private static boolean canMoveValue( } /** - * Transforms instanceof usages into an expression that short circuits to - * false if tested with a constructor that is undefined. This allows ignoring - * instanceof with respect to cross module code motion. + * Transforms instanceof usages into an expression that short circuits to false if tested with a + * constructor that is undefined. This allows ignoring instanceof with respect to cross module + * code motion. */ private void addInstanceofGuards() { Node tmp = IR.block(); diff --git a/src/com/google/javascript/jscomp/JSModuleGraph.java b/src/com/google/javascript/jscomp/JSModuleGraph.java index 2234558ba5e..6dd35e184b2 100644 --- a/src/com/google/javascript/jscomp/JSModuleGraph.java +++ b/src/com/google/javascript/jscomp/JSModuleGraph.java @@ -268,10 +268,10 @@ public boolean dependsOn(JSModule src, JSModule m) { *

If multiple candidates have the same number of dependents, the module farthest down in the * total ordering of modules will be chosen. * - * @param dependentModules to consider + * @param dependentModules indices of modules to consider * @return A module on which all of the argument modules depend */ - public JSModule getSmallestCoveringDependency(Collection dependentModules) { + public JSModule getSmallestCoveringDependency(BitSet dependentModules) { checkState(!dependentModules.isEmpty()); // Candidate modules are those that all of the given dependent modules depend on, including @@ -280,8 +280,9 @@ public JSModule getSmallestCoveringDependency(Collection dependentModu int minDependentModuleIndex = modules.length; final BitSet candidates = new BitSet(modules.length); candidates.set(0, modules.length, true); - for (JSModule module : dependentModules) { - int dependentIndex = module.getIndex(); + for (int dependentIndex = dependentModules.nextSetBit(0); + dependentIndex >= 0; + dependentIndex = dependentModules.nextSetBit(dependentIndex + 1)) { minDependentModuleIndex = Math.min(minDependentModuleIndex, dependentIndex); candidates.and(selfPlusTransitiveDeps[dependentIndex]); } diff --git a/test/com/google/javascript/jscomp/JSModuleGraphTest.java b/test/com/google/javascript/jscomp/JSModuleGraphTest.java index 9599a667fd7..3dbf7b9f710 100644 --- a/test/com/google/javascript/jscomp/JSModuleGraphTest.java +++ b/test/com/google/javascript/jscomp/JSModuleGraphTest.java @@ -26,7 +26,7 @@ import com.google.gson.JsonObject; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; +import java.util.BitSet; import java.util.List; import junit.framework.TestCase; @@ -394,16 +394,20 @@ private List requires(String ... strings) { } private void assertSmallestCoveringDependency(JSModule expected, JSModule... modules) { - assertSmallestCoveringDependency(expected, graph, Arrays.asList(modules)); + assertSmallestCoveringDependency(expected, graph, modules); } private void assertSmallestCoveringDependency( JSModule expected, JSModuleGraph graph, JSModule... modules) { - assertSmallestCoveringDependency(expected, graph, Arrays.asList(modules)); + BitSet modulesBitSet = new BitSet(); + for (JSModule m : modules) { + modulesBitSet.set(m.getIndex()); + } + assertSmallestCoveringDependency(expected, graph, modulesBitSet); } private void assertSmallestCoveringDependency( - JSModule expected, JSModuleGraph graph, Collection modules) { + JSModule expected, JSModuleGraph graph, BitSet modules) { JSModule actual = graph.getSmallestCoveringDependency(modules); assertWithMessage( "Smallest covering dep of %s should be %s but was %s", modules, expected, actual)