Tune the heuristic that stops optimization loops based on a size thre…

…shold, so it can fire more often. Speeds up optimized builds up to 11%, with code size increase up to 0.14%.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150372451

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";

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) {

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);

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<String> CODE_MOTION_PASSES =
-      ImmutableList.of(
-          Compiler.CROSS_MODULE_CODE_MOTION_NAME, Compiler.CROSS_MODULE_METHOD_MOTION_NAME);
+  static final ImmutableList<String> 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<NamedPass> myPasses = new ArrayList<>();
     private final Set<String> 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;
         }
       }

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);
   }
 

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() {