diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java index 51701516abf..9c86d8aec52 100644 --- a/src/com/google/javascript/jscomp/Compiler.java +++ b/src/com/google/javascript/jscomp/Compiler.java @@ -104,8 +104,9 @@ public class Compiler extends AbstractCompiler implements ErrorHandler, SourceFi static final String READING_PASS_NAME = "readInputs"; static final String PARSING_PASS_NAME = "parseInputs"; static final String CROSS_MODULE_CODE_MOTION_NAME = "crossModuleCodeMotion"; - static final String CROSS_MODULE_METHOD_MOTION_NAME = - "crossModuleMethodMotion"; + static final String CROSS_MODULE_METHOD_MOTION_NAME = "crossModuleMethodMotion"; + static final String PEEPHOLE_PASS_NAME = "peepholeOptimizations"; + static final String UNREACHABLE_CODE_ELIM_NAME = "removeUnreachableCode"; private static final String CONFIG_RESOURCE = "com.google.javascript.jscomp.parsing.ParserConfig"; diff --git a/src/com/google/javascript/jscomp/CompilerOptions.java b/src/com/google/javascript/jscomp/CompilerOptions.java index ca45b8032cf..f1f9c32d8b8 100644 --- a/src/com/google/javascript/jscomp/CompilerOptions.java +++ b/src/com/google/javascript/jscomp/CompilerOptions.java @@ -534,6 +534,8 @@ public enum ExtractPrototypeMemberDeclarationsMode { /** Use type information to enable additional optimization opportunities. */ boolean useTypesForLocalOptimization; + boolean useSizeHeuristicToStopOptimizationLoop = true; + //-------------------------------- // Renaming //-------------------------------- @@ -2204,6 +2206,10 @@ public void setUseTypesForLocalOptimization(boolean useTypesForLocalOptimization this.useTypesForLocalOptimization = useTypesForLocalOptimization; } + public void setUseSizeHeuristicToStopOptimizationLoop(boolean mayStopEarly) { + this.useSizeHeuristicToStopOptimizationLoop = mayStopEarly; + } + @Deprecated public void setUseTypesForOptimization(boolean useTypesForOptimization) { if (useTypesForOptimization) { diff --git a/src/com/google/javascript/jscomp/DefaultPassConfig.java b/src/com/google/javascript/jscomp/DefaultPassConfig.java index 3a2b5e2934d..7c1a9499227 100644 --- a/src/com/google/javascript/jscomp/DefaultPassConfig.java +++ b/src/com/google/javascript/jscomp/DefaultPassConfig.java @@ -1548,7 +1548,7 @@ private static CompilerPass createPeepholeOptimizationsPass(AbstractCompiler com /** Various peephole optimizations. */ private final PassFactory peepholeOptimizations = - new PassFactory("peepholeOptimizations", false /* oneTimePass */) { + new PassFactory(Compiler.PEEPHOLE_PASS_NAME, false /* oneTimePass */) { @Override protected CompilerPass create(AbstractCompiler compiler) { return createPeepholeOptimizationsPass(compiler); @@ -1557,7 +1557,7 @@ protected CompilerPass create(AbstractCompiler compiler) { /** Various peephole optimizations. */ private final PassFactory peepholeOptimizationsOnce = - new PassFactory("peepholeOptimizations", true /* oneTimePass */) { + new PassFactory(Compiler.PEEPHOLE_PASS_NAME, true /* oneTimePass */) { @Override protected CompilerPass create(AbstractCompiler compiler) { return createPeepholeOptimizationsPass(compiler); @@ -2220,7 +2220,7 @@ protected CompilerPass create(AbstractCompiler compiler) { * Use data flow analysis to remove dead branches. */ private final PassFactory removeUnreachableCode = - new PassFactory("removeUnreachableCode", false) { + new PassFactory(Compiler.UNREACHABLE_CODE_ELIM_NAME, false) { @Override protected CompilerPass create(AbstractCompiler compiler) { return new UnreachableCodeElimination(compiler, true); diff --git a/src/com/google/javascript/jscomp/PhaseOptimizer.java b/src/com/google/javascript/jscomp/PhaseOptimizer.java index 1b6dcc7d3cd..7aed9bd480b 100644 --- a/src/com/google/javascript/jscomp/PhaseOptimizer.java +++ b/src/com/google/javascript/jscomp/PhaseOptimizer.java @@ -69,6 +69,7 @@ class PhaseOptimizer implements CompilerPass { // Compiler/reportChangeToScope must call reportCodeChange to update all // change handlers. This flag prevents double update in ScopedChangeHandler. private boolean crossScopeReporting; + private final boolean useSizeHeuristicToStopOptimizationLoop; // Used for sanity checks between loopable passes private Node lastAst; @@ -104,9 +105,8 @@ enum State { "minimizeExitPoints", "removeUnreachableCode"); - static final ImmutableList CODE_MOTION_PASSES = - ImmutableList.of( - Compiler.CROSS_MODULE_CODE_MOTION_NAME, Compiler.CROSS_MODULE_METHOD_MOTION_NAME); + static final ImmutableList CODE_REMOVING_PASSES = ImmutableList.of( + Compiler.PEEPHOLE_PASS_NAME, Compiler.UNREACHABLE_CODE_ELIM_NAME); static final int MAX_LOOPS = 100; static final String OPTIMIZE_LOOP_ERROR = @@ -118,8 +118,7 @@ enum State { * @param range the progress range for the process function or null * if progress should not be reported. */ - PhaseOptimizer( - AbstractCompiler comp, PerformanceTracker tracker, ProgressRange range) { + PhaseOptimizer(AbstractCompiler comp, PerformanceTracker tracker, ProgressRange range) { this.compiler = comp; this.jsRoot = comp.getJsRoot(); this.tracker = tracker; @@ -128,6 +127,8 @@ enum State { this.inLoop = false; this.crossScopeReporting = false; this.timestamp = this.lastChange = START_TIME; + this.useSizeHeuristicToStopOptimizationLoop = + comp.getOptions().useSizeHeuristicToStopOptimizationLoop; } /** @@ -252,9 +253,9 @@ private void maybeSanityCheck(Node externs, Node root) { // The cross-module passes are loopable and ran together, but do not // participate in the other optimization loops, and are not relevant to // tracking changed scopes. - if (inLoop && - !currentPass.name.equals(Compiler.CROSS_MODULE_CODE_MOTION_NAME) && - !currentPass.name.equals(Compiler.CROSS_MODULE_METHOD_MOTION_NAME)) { + if (inLoop + && !currentPass.name.equals(Compiler.CROSS_MODULE_CODE_MOTION_NAME) + && !currentPass.name.equals(Compiler.CROSS_MODULE_METHOD_MOTION_NAME)) { NodeUtil.verifyScopeChanges(mtoc, jsRoot, true); } } @@ -422,6 +423,7 @@ class Loop implements CompilerPass { private final List myPasses = new ArrayList<>(); private final Set myNames = new HashSet<>(); private ScopedChangeHandler scopeHandler; + private boolean isCodeRemovalLoop = false; void addLoopedPass(PassFactory factory) { String name = factory.getName(); @@ -436,7 +438,7 @@ public void process(Node externs, Node root) { Preconditions.checkState(!inLoop, "Nested loops are forbidden"); inLoop = true; optimizePasses(); - boolean isCodeMotionLoop = isCodeMotionLoop(); + this.isCodeRemovalLoop = isCodeRemovalLoop(); // Set up function-change tracking scopeHandler = new ScopedChangeHandler(); @@ -497,28 +499,17 @@ public void process(Node externs, Node root) { } } + previousAstSize = astSize; + astSize = NodeUtil.countAstSize(root); if (state == State.RUN_PASSES_NOT_RUN_IN_PREV_ITER) { - if (lastIterMadeChanges) { + if (lastIterMadeChanges && astChangesAreOverThreshold(previousAstSize, astSize)) { state = State.RUN_PASSES_THAT_CHANGED_STH_IN_PREV_ITER; } else { return; } - } else { // state == State.RUN_PASSES_THAT_CHANGED_STH_IN_PREV_ITER - if (!lastIterMadeChanges) { - previousAstSize = astSize; - astSize = NodeUtil.countAstSize(root); - float percentChange = Math.abs(astSize - previousAstSize) / (float) previousAstSize; - // If this loop batch made the code less than 0.1% smaller than the previous loop - // batch, stop before the fixpoint. - // Use this criterion only for loops that remove code; the code-motion loop may - // move code around but not remove code, so this criterion is not correct for - // stopping early. - // This threshold is based on the following heuristic: 1% size difference matters - // to our users. 0.1% size difference is borderline relevant. 0.1% difference - // between loop batches is smaller than 0.1% total difference, so it's unimportant. - if (!isCodeMotionLoop && percentChange < 0.001) { - return; - } + } else { + Preconditions.checkState(state == State.RUN_PASSES_THAT_CHANGED_STH_IN_PREV_ITER); + if (!lastIterMadeChanges || !astChangesAreOverThreshold(previousAstSize, astSize)) { state = State.RUN_PASSES_NOT_RUN_IN_PREV_ITER; } } @@ -529,6 +520,33 @@ public void process(Node externs, Node root) { } } + /** + * If this loop batch made the code less than 0.05% smaller than the previous loop + * batch, stop before the fixpoint. + * The 0.05% threshold is based on the following heuristic: 1% size difference matters + * to our users. 0.1% size difference is borderline relevant. 0.05% difference + * between loop batches is unlikely to grow the final output more than 0.1%. + * + * Use this criterion only for the two code-removing loops. + * The code-motion loop may move code around but not remove code, so this criterion + * is not correct for stopping early. + * (AggressiveInlineAliases is in a loop by itself, and we should fix that; don't use the + * size heuristic for that loop either.) + * + * NOTE: the size heuristic is not robust when passes in the code-removing loop increase + * the AST size; all passes in the loop must make the code smaller. Otherwise, what may seem + * like a small size difference may indeed be big changes, and we miss it because we don't + * compute the AST size after each pass. This can currently happen with inlineFunctions, + * which is why we put this heuristic under a flag, rather than enable it unconditionally. + */ + private boolean astChangesAreOverThreshold(int oldAstSize, int newAstSize) { + if (useSizeHeuristicToStopOptimizationLoop && this.isCodeRemovalLoop) { + float percentChange = 100 * (Math.abs(newAstSize - oldAstSize) / (float) oldAstSize); + return percentChange > 0.05; + } + return true; + } + /** Re-arrange the passes in an optimal order. */ private void optimizePasses() { // It's important that this ordering is deterministic, so that @@ -551,9 +569,9 @@ private void optimizePasses() { myPasses.addAll(optimalPasses); } - private boolean isCodeMotionLoop() { + private boolean isCodeRemovalLoop() { for (NamedPass pass : this.myPasses) { - if (CODE_MOTION_PASSES.contains(pass.name)) { + if (CODE_REMOVING_PASSES.contains(pass.name)) { return true; } } diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index 109fc223f69..e0cfc377633 100644 --- a/test/com/google/javascript/jscomp/IntegrationTest.java +++ b/test/com/google/javascript/jscomp/IntegrationTest.java @@ -2771,14 +2771,11 @@ public void testAddFunctionProperties4() throws Exception { "};" + "goog.addSingletonGetter(Foo);" + "alert(Foo.f());"; - String expected = - "function Foo(){} Foo.f=function(){Foo.i=new Foo}; alert(Foo.f());"; + String expected = "function Foo(){} Foo.f=function(){Foo.i=new Foo}; alert(Foo.f());"; CompilerOptions options = createCompilerOptions(); - CompilationLevel.ADVANCED_OPTIMIZATIONS - .setOptionsForCompilationLevel(options); - options.setRenamingPolicy( - VariableRenamingPolicy.OFF, PropertyRenamingPolicy.OFF); + CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options); + options.setRenamingPolicy(VariableRenamingPolicy.OFF, PropertyRenamingPolicy.OFF); test(options, source, expected); } @@ -3241,8 +3238,7 @@ public void testNamelessParameter() { "function $init() {" + " impl_0 = {};" + "}"; - String result = - "window.f = {};"; + String result = "window.f = {};"; test(options, code, result); } diff --git a/test/com/google/javascript/jscomp/PhaseOptimizerTest.java b/test/com/google/javascript/jscomp/PhaseOptimizerTest.java index 2d82a5f2c90..2ac99f92cab 100644 --- a/test/com/google/javascript/jscomp/PhaseOptimizerTest.java +++ b/test/com/google/javascript/jscomp/PhaseOptimizerTest.java @@ -72,8 +72,8 @@ public void testSchedulingOfLoopablePasses() { Loop loop = optimizer.addFixedPointLoop(); addLoopedPass(loop, "x", 3); addLoopedPass(loop, "y", 1); - // The pass iterations can be grouped as: [x y] [x y] [x] [x] - assertPasses("x", "y", "x", "y", "x", "x"); + // The pass iterations can be grouped as: [x y] [x y] [x] [x] [y] + assertPasses("x", "y", "x", "y", "x", "x", "y"); } public void testNotInfiniteLoop() { @@ -101,7 +101,7 @@ public void testSchedulingOfAnyKindOfPasses1() { addLoopedPass(loop, "x", 3); addLoopedPass(loop, "y", 1); addOneTimePass("z"); - assertPasses("a", "x", "y", "x", "y", "x", "x", "z"); + assertPasses("a", "x", "y", "x", "y", "x", "x", "y", "z"); } public void testSchedulingOfAnyKindOfPasses2() { @@ -115,7 +115,7 @@ public void testSchedulingOfAnyKindOfPasses2() { createPassFactory("f", 0, true))); // The pass iterations can be grouped as: // [a] [b c d] [b c d] [c] [b d] [e] [f] - assertPasses("a", "b", "c", "d", "b", "c", "d", "c", "e", "f"); + assertPasses("a", "b", "c", "d", "b", "c", "d", "c", "b", "d", "e", "f"); } public void testSchedulingOfAnyKindOfPasses3() {