From 2a4317fa7b2f5b1704e4adee9870e0e94c13318e Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Wed, 31 May 2017 08:46:43 -0700 Subject: [PATCH] Move definitions to smallest dependent subtree. Improve the selection of where to move code by always picking the smallest module subtree that contains all references to the moved definition. Before this change the preferred module was recalculated as each reference was found, and this piecemeal approach can lead to sub-optimal choices. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157590940 --- .../jscomp/CrossModuleCodeMotion.java | 95 ++++++++++++------- .../javascript/jscomp/JSModuleGraph.java | 9 +- .../javascript/jscomp/JSModuleGraphTest.java | 12 ++- 3 files changed, 74 insertions(+), 42 deletions(-) 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)