diff --git a/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java b/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java index 71c7e34cee..d2f840784a 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java +++ b/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java @@ -1022,6 +1022,8 @@ private void optimizeJsLoop(Collection toInline) throws InterruptedExcep stats.recordModified(JsStaticEval.exec(jsProgram)); // Inline Js function invocations stats.recordModified(JsInliner.exec(jsProgram, toInline)); + // After inlining, reduce clinit calls within each function + stats.recordModified(DuplicateClinitRemover.exec(jsProgram)); // Remove unused functions if possible. stats.recordModified(JsUnusedFunctionRemover.exec(jsProgram)); @@ -1037,10 +1039,6 @@ private void optimizeJsLoop(Collection toInline) throws InterruptedExcep break; } } - - if (optimizationLevel > OptionOptimize.OPTIMIZE_LEVEL_DRAFT) { - DuplicateClinitRemover.exec(jsProgram); - } } private Map renameJsSymbols(PermutationProperties properties, diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java b/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java index eb8b806cc2..717e2a89ce 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java +++ b/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java @@ -3815,7 +3815,7 @@ private void writeEnumValueOfMethod(JEnumType type, JMethod method, JMethod valu JFieldRef mapRef = new JFieldRef(info, null, mapField, mapClass); JDeclarationStatement declStmt = new JDeclarationStatement(info, mapRef, call); JMethod clinit = - createSyntheticMethod(info, "$clinit", mapClass, JPrimitiveType.VOID, false, true, + createSyntheticMethod(info, CLINIT_METHOD_NAME, mapClass, JPrimitiveType.VOID, false, true, true, AccessModifier.PRIVATE); JBlock clinitBlock = ((JMethodBody) clinit.getBody()).getBlock(); clinitBlock.addStmt(declStmt); diff --git a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java index 59d23333d9..a89eabc622 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -19,13 +19,12 @@ import com.google.gwt.dev.jjs.impl.OptimizerStats; import com.google.gwt.dev.js.ast.JsBinaryOperation; import com.google.gwt.dev.js.ast.JsBinaryOperator; -import com.google.gwt.dev.js.ast.JsBlock; import com.google.gwt.dev.js.ast.JsCase; +import com.google.gwt.dev.js.ast.JsCatch; import com.google.gwt.dev.js.ast.JsConditional; import com.google.gwt.dev.js.ast.JsContext; import com.google.gwt.dev.js.ast.JsDefault; -import com.google.gwt.dev.js.ast.JsEmpty; -import com.google.gwt.dev.js.ast.JsExprStmt; +import com.google.gwt.dev.js.ast.JsDoWhile; import com.google.gwt.dev.js.ast.JsExpression; import com.google.gwt.dev.js.ast.JsFor; import com.google.gwt.dev.js.ast.JsForIn; @@ -36,6 +35,7 @@ import com.google.gwt.dev.js.ast.JsNode; import com.google.gwt.dev.js.ast.JsNullLiteral; import com.google.gwt.dev.js.ast.JsProgram; +import com.google.gwt.dev.js.ast.JsTry; import com.google.gwt.dev.js.ast.JsWhile; import java.util.HashSet; @@ -46,7 +46,7 @@ * This is used to clean up duplication invocations of clinit function. Whenever there is a * possible branch in program flow, the remover will create a new instance of * itself to handle the possible branches. - * + *

* We don't look at combining branch choices. This will not produce the most * efficient elimination of duplicated calls, but it handles the general case * and is simple to verify. @@ -67,74 +67,104 @@ public class DuplicateClinitRemover extends JsModVisitor { public DuplicateClinitRemover(JsProgram program) { this.program = program; - called = new HashSet(); + this.called = new HashSet<>(); } public DuplicateClinitRemover(JsProgram program, Set alreadyCalled) { this.program = program; - called = new HashSet(alreadyCalled); + this.called = new HashSet<>(alreadyCalled); + } + + /** + * Given a JsInvocation, determine if it is invoking a JsFunction that is + * specified to be executed only once during the program's lifetime. + */ + public static JsFunction isClinit(JsInvocation invocation) { + JsFunction f = JsUtils.isFunction(invocation.getQualifier()); + if (f != null && f.isClinit()) { + return f; + } + return null; } /** * Look for comma expressions that contain duplicate calls and handle the * conditional-evaluation case of logical and/or operations. + *

+ * The comma case seems like it would be handled better by just visiting and removing/rewriting + * invocations, under the assumption that later passes would tidy up better, but the + * (clinit(), null) output case will leave behind the null as if it was going to be returned and + * thus can't be removed. Since to address that, we must handle both (xyz, clinit()) and + * (clinit(), clinit()) inputs, we might as well handle them all here. */ @Override public boolean visit(JsBinaryOperation x, JsContext ctx) { if (x.getOperator() == JsBinaryOperator.COMMA) { - - boolean left = isDuplicateCall(x.getArg1()); - boolean right = isDuplicateCall(x.getArg2()); - - if (left && right) { - /* - * (clinit(), clinit()) --> delete or null. - * - * This construct is very unlikely since the InliningVisitor builds - * the comma expressions in a right-nested manner. - */ - if (ctx.canRemove()) { - ctx.removeMe(); + // This effectively visits any JsInvocation direct child, so take care to not encounter any + // clinit twice when descending further. Important: if the right was a new clinit, we must not + // go back and re-check the left or we'll have changed the order of execution. To ensure this, + // we shallowly check the left and based on that result decide to either visit the left or + // how to check the right. + ClinitStatus left = isDuplicateCall(x.getArg1()); + if (left == ClinitStatus.DUPLICATE_CLINIT) { + // We've already seen the left clinit and can remove it, decide how to replace the right + ClinitStatus right = isDuplicateCall(x.getArg2()); + if (right == ClinitStatus.DUPLICATE_CLINIT) { + /* + * (clinit(), clinit()) --> delete or null. + * Repeated inlining can cause this, if there is an earlier clinit statement/expr in the + * branch. + */ + if (ctx.canRemove()) { + ctx.removeMe(); + } else { + // The return value from a clinit is never used + ctx.replaceMe(JsNullLiteral.INSTANCE); + } + return false; + } else if (right == ClinitStatus.NEW_CLINIT) { + // Don't re-visit, it was just a clinit and we already observed it + ctx.replaceMe(x.getArg2()); return false; } else { - // The return value from an XO function is never used - ctx.replaceMe(JsNullLiteral.INSTANCE); + assert right == ClinitStatus.NOT_A_CLINIT; + // Safe to re-visit, nested clinits could be removed + ctx.replaceMe(accept(x.getArg2())); return false; } + } else { + if (left == ClinitStatus.NOT_A_CLINIT) { + // Visit left before proceeding, so we've fully checked that expression + x.setArg1(accept(x.getArg1())); + } else { + assert left == ClinitStatus.NEW_CLINIT; + } - } else if (left) { - // (clinit(), xyz) --> xyz - // This is the common case - ctx.replaceMe(accept(x.getArg2())); - return false; - - } else if (right) { - // (xyz, clinit()) --> xyz - // Possible if a clinit() were the last element - ctx.replaceMe(accept(x.getArg1())); - return false; + // Shallow check of the right to decide how to proceed + ClinitStatus right = isDuplicateCall(x.getArg2()); + if (right == ClinitStatus.DUPLICATE_CLINIT) { + // Discard right, keep only left + ctx.replaceMe(x.getArg1()); + return false; + } else if (right == ClinitStatus.NEW_CLINIT) { + // Must keep both as-is + return false; + } else { + assert right == ClinitStatus.NOT_A_CLINIT; + // Safe to re-visit, nested clinits could be removed + x.setArg2(accept(x.getArg2())); + return false; + } } - } else if (x.getOperator().equals(JsBinaryOperator.AND) || x.getOperator().equals(JsBinaryOperator.OR)) { x.setArg1(accept(x.getArg1())); // Possibility of conditional evaluation of second parameter x.setArg2(branch(x.getArg2())); return false; + } else { + return true; } - - return true; - } - - /** - * Most of the branching statements (as well as JsFunctions) will visit with - * a JsBlock, so we don't need to explicitly enumerate all JsStatement - * subtypes. - */ - @Override - public boolean visit(JsBlock x, JsContext ctx) { - branch(x.getStatements()); - return false; } @Override @@ -159,18 +189,13 @@ public boolean visit(JsDefault x, JsContext ctx) { } @Override - public boolean visit(JsExprStmt x, JsContext ctx) { - if (isDuplicateCall(x.getExpression())) { - if (ctx.canRemove()) { - ctx.removeMe(); - } else { - ctx.replaceMe(new JsEmpty(x.getSourceInfo())); - } - return false; - - } else { - return true; - } + public boolean visit(JsDoWhile x, JsContext ctx) { + // We have to visit manually, the visitor looks at the condition before the body. At this time, + // both must be branch()es, since we can't reliably ensure that either will be hit - an + // if statement could "continue" and skip the rest of the method. + x.setBody(branch(x.getBody())); + x.setCondition(branch(x.getCondition())); + return false; } @Override @@ -187,12 +212,13 @@ public boolean visit(JsFor x, JsContext ctx) { x.setCondition(accept(x.getCondition())); } - // The increment expression is optional + // The increment expression is optional. When present, it always runs after the body, so it + // could be a sub-branch of that, when we reliably can determine what clinits are called + // executing a block that could continue if (x.getIncrExpr() != null) { x.setIncrExpr(branch(x.getIncrExpr())); } - // The body is not guaranteed to be a JsBlock x.setBody(branch(x.getBody())); return false; } @@ -205,7 +231,12 @@ public boolean visit(JsForIn x, JsContext ctx) { x.setObjExpr(accept(x.getObjExpr())); - // The body is not guaranteed to be a JsBlock + x.setBody(branch(x.getBody())); + return false; + } + + @Override + public boolean visit(JsFunction x, JsContext ctx) { x.setBody(branch(x.getBody())); return false; } @@ -227,19 +258,46 @@ public boolean visit(JsIf x, JsContext ctx) { */ @Override public boolean visit(JsInvocation x, JsContext ctx) { - JsFunction func = JsUtils.isExecuteOnce(x); - while (func != null) { - called.add(func); - func = func.getSuperClinit(); + if (isDuplicateCall(x) == ClinitStatus.DUPLICATE_CLINIT) { + if (ctx.canRemove()) { + ctx.removeMe(); + } else { + ctx.replaceMe(JsNullLiteral.INSTANCE); + } + return false; } return true; } + @Override + public boolean visit(JsTry x, JsContext ctx) { + if (!x.getCatches().isEmpty()) { + // Catch could return control to parent block without completing the try block, so branch + // the try block if there is any catch. + x.setTryBlock(branch(x.getTryBlock())); + List catches = x.getCatches(); + for (int i = 0; i < catches.size(); i++) { + JsCatch aCatch = catches.get(i); + JsCatch c = accept(aCatch); + catches.set(i, c); + } + } else { + if (x.getFinallyBlock() != null) { + // On the other hand, if there is a finally block, the try block isn't guaranteed to complete + // before finally runs, so finally needs to start from the same initial state as try did. We + // can do that by branch()ing finally first, then accept()ing try + x.setFinallyBlock(branch(x.getFinallyBlock())); + } + x.setTryBlock(accept(x.getTryBlock())); + } + + return false; + } + @Override public boolean visit(JsWhile x, JsContext ctx) { x.setCondition(accept(x.getCondition())); - // The body is not guaranteed to be a JsBlock x.setBody(branch(x.getBody())); return false; } @@ -280,12 +338,31 @@ private T branch(T x) { return toReturn; } - private boolean isDuplicateCall(JsExpression x) { + private enum ClinitStatus { + NOT_A_CLINIT, + NEW_CLINIT, + DUPLICATE_CLINIT + } + + /** + * If the expression is a clinit, mark it as seen, and return true if it should be removed. + */ + private ClinitStatus isDuplicateCall(JsExpression x) { if (!(x instanceof JsInvocation)) { - return false; + return ClinitStatus.NOT_A_CLINIT; } - JsFunction func = JsUtils.isExecuteOnce((JsInvocation) x); - return (func != null && called.contains(func)); + JsFunction func = isClinit((JsInvocation) x); + if (func != null) { + if (called.contains(func)) { + return ClinitStatus.DUPLICATE_CLINIT; + } + while (func != null) { + called.add(func); + func = func.getSuperClinit(); + } + return ClinitStatus.NEW_CLINIT; + } + return ClinitStatus.NOT_A_CLINIT; } } diff --git a/dev/core/src/com/google/gwt/dev/js/JsUtils.java b/dev/core/src/com/google/gwt/dev/js/JsUtils.java index dbad2c108a..b8aadc8d6e 100644 --- a/dev/core/src/com/google/gwt/dev/js/JsUtils.java +++ b/dev/core/src/com/google/gwt/dev/js/JsUtils.java @@ -53,18 +53,6 @@ * Utils for JS AST. */ public class JsUtils { - /** - * Given a JsInvocation, determine if it is invoking a JsFunction that is - * specified to be executed only once during the program's lifetime. - */ - public static JsFunction isExecuteOnce(JsInvocation invocation) { - JsFunction f = isFunction(invocation.getQualifier()); - if (f != null && f.isClinit()) { - return f; - } - return null; - } - /** * Given an expression, determine if it is a JsNameRef that refers to a * statically-defined JsFunction. diff --git a/dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java b/dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java index bf1412f89f..bd5f6de089 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java @@ -15,6 +15,8 @@ */ package com.google.gwt.dev.js; +import com.google.gwt.dev.js.ast.JsProgram; + /** * Tests the JsDuplicateCaseFolder optimizer. */ @@ -23,34 +25,34 @@ public class JsDuplicateCaseFolderTest extends OptimizerTestBase { public void test1() throws Exception { String input = "function a(x){switch(x){case 0:return 17;case 12:case 1:return 18;case 2:return 17;case 3:return 18;}}"; String expected = "function a(x){switch(x){case 2:case 0:return 17;case 12:case 3:case 1:return 18;}}\n"; - check(expected, input); + optimize(input).into(expected); } public void test1b() throws Exception { String input = "function a(x){switch(x){case 0:return 17;case 12:case 1:return 18;default:return 17;case 3:return 18;}}"; String expected = "function a(x){switch(x){default:case 0:return 17;case 12:case 3:case 1:return 18;}}\n"; - check(expected, input); + optimize(input).into(expected); } public void test2() throws Exception { // Don't coalesces cases 0 and 2 since 2 can be reached via fallthrough from 1 String input = "function a(x){var y;switch(x){case 0:return 17;case 1:y=18;case 2:return 17;case 3:y=18;}return y}"; String expected = "function a(x){var y;switch(x){case 0:return 17;case 1:y=18;case 2:return 17;case 3:y=18;}return y}\n"; - check(expected, input); + optimize(input).into(expected); } public void test3() throws Exception { // cases 1 and 3 can fall through, don't coalesce String input = "function a(x){var y;switch(x){case 0:return 17;case 1:y=18;break;case 2:return 17;case 3:y=18;break;}return y}"; String expected = "function a(x){var y;switch(x){case 2:case 0:return 17;case 3:case 1:y=18;break;}return y}\n"; - check(expected, input); + optimize(input).into(expected); } public void test4() throws Exception { // cases 1 and 3 may be coalesced String input = "function a(x,z){var y;switch(x){case 0:return 17;case 1:if (z==0){y=18;break}else{y=19;break}case 2:return 17;case 3:if(z==0){y=18;break}else{y=19;break}}return y}"; String expected = "function a(x,z){var y;switch(x){case 2:case 0:return 17;case 3:case 1:if(z==0){y=18;break}else{y=19;break}}return y}\n"; - check(expected, input); + optimize(input).into(expected); } public void test4b() throws Exception { @@ -58,38 +60,33 @@ public void test4b() throws Exception { // ensure additional fallthroughs are handled correctly String input = "function a(x,z){var y;switch(x){case 0:case 22:return 17;case 100:case 1:if (z==0){y=18;break}else{y=19;return 20}case 2:return 17;case 200:case 3:if(z==0){y=18;break}else{y=19;return 20}}return y}"; String expected = "function a(x,z){var y;switch(x){case 0:case 2:case 22:return 17;case 100:case 1:if(z==0){y=18;break}else{y=19;return 20}case 200:case 3:if(z==0){y=18;break}else{y=19;return 20}}return y}\n"; - check(expected, input); + optimize(input).into(expected); } public void test5() throws Exception { // cases 1 and 3 can fall through due to no else clause, don't coalesce String input = "function a(x,z){var y;switch(x){case 0:return 17;case 1:if (z==0){y=18;break}else{y=19}case 2:return 17;case 3:if(z==0){y=18;break}else{y=19}}return y}"; String expected = "function a(x,z){var y;switch(x){case 0:return 17;case 1:if(z==0){y=18;break}else{y=19}case 2:return 17;case 3:if(z==0){y=18;break}else{y=19}}return y}\n"; - check(expected, input); + optimize(input).into(expected); } public void test6() throws Exception { // cases 1 and 3 can fall through due to no else clause, don't coalesce String input = "function a(x,z){var y;switch(x){case 0:y=17;break;case 1:if(z==0){y=18;break}else{y=19}case 2:return 22;case 3:if(z==0){y=18;break}else{y=19}case 4:y=17;break;case 5:y=17;break;case 6:return 22;}return y}"; String expected = "function a(x,z){var y;switch(x){case 0:y=17;break;case 1:if(z==0){y=18;break}else{y=19}case 6:case 2:return 22;case 3:if(z==0){y=18;break}else{y=19}case 5:case 4:y=17;break;}return y}\n"; - check(expected, input); + optimize(input).into(expected); } public void test6b() throws Exception { // cases 1 and 3 can fall through due to no else clause, don't coalesce String input = "function a(x,z){var y;switch(x){case 0:y=17;break;case 1:if(z==0){y=18;break}else{y=19}default:return 22;case 3:if(z==0){y=18;break}else{y=19}case 4:y=17;break;case 5:y=17;break;case 6:return 22;}return y}"; String expected = "function a(x,z){var y;switch(x){case 0:y=17;break;case 1:if(z==0){y=18;break}else{y=19}case 6:default:return 22;case 3:if(z==0){y=18;break}else{y=19}case 5:case 4:y=17;break;}return y}\n"; - check(expected, input); - } - - private void check(String expected, String input) throws Exception { - // Pass the expected code through the parser to normalize it - expected = super.optimizeToSource(expected, new Class[0]); - String output = optimize(input); - assertEquals(expected, output); + optimize(input).into(expected); } - private String optimize(String js) throws Exception { - return optimizeToSource(js, JsDuplicateCaseFolder.class); + @Override + protected boolean doOptimize(JsProgram program) throws Exception { + JsDuplicateCaseFolder.exec(program); + return true; } } diff --git a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java new file mode 100644 index 0000000000..04b8d6b43b --- /dev/null +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java @@ -0,0 +1,402 @@ +/* + * Copyright 2026 GWT Project Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.google.gwt.dev.js; + +import com.google.gwt.dev.js.ast.JsContext; +import com.google.gwt.dev.js.ast.JsFunction; +import com.google.gwt.dev.js.ast.JsModVisitor; +import com.google.gwt.dev.js.ast.JsName; +import com.google.gwt.dev.js.ast.JsProgram; + +/** + * Need tests for + * switch/forin + * super clinits: + * if/for/while/do/conditional/short-circuit/switch/forin + * + * Several tests use an expression like "x + (clinit_A(), y) > 0" to avoid short-circuiting but + * still allow for side effects so the clinit won't be moved out before the for condition. + */ +public class JsDuplicateClinitRemoverTest extends OptimizerTestBase { + private static final String CLINIT_DECL = "function emptyFunc(){}" + + "function clinit_A(){clinit_A = emptyFunc}" + + "function clinit_B(){clinit_B = emptyFunc}" + + "function clinit_C(){clinit_C = emptyFunc; clinit_B();}"; + + public void testRemoveDupClinitsInBlock() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "clinit_A();") + .into(CLINIT_DECL, + "clinit_A()"); + } + + public void testRemoveDupClinitsInExpr() throws Exception { + optimize(CLINIT_DECL, + "value = (clinit_A(),clinit_A(), 1);") + .into(CLINIT_DECL, + "value=(clinit_A(),1);"); + } + + public void testDupClinitsBlockAndExpr() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "value = (clinit_A(),clinit_A(), 1);") + .into(CLINIT_DECL, + "clinit_A();", + "value=1;"); + optimize(CLINIT_DECL, + "value = (clinit_A(), 1);", + "clinit_A();") + .into(CLINIT_DECL, + "value=(clinit_A(), 1);" + ); + } + + public void testRemoveDupClinitsInIf() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "if (cond1) { while(cond2) { b++; clinit_A(); } } else { clinit_A(); c(); }") + .into(CLINIT_DECL, + "clinit_A();", + "if (cond1) { while(cond2) { b++; } } else { c(); }"); + + verifyNoChange(CLINIT_DECL, + "if (cond1) { while(cond2) { b++; clinit_A(); } } else { clinit_B(); c(); }", + "clinit_A();", + "clinit_B();" + ); + + verifyNoChange(CLINIT_DECL, + "if (cond1) { while(cond2) { b++; clinit_A(); } } else { clinit_A(); c(); }", + "clinit_A();"); + } + + public void testRemoveDupClinitsInWhile() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "while(cond) { b++; clinit_A(); }") + .into(CLINIT_DECL, + "clinit_A();", + "while(cond) { b++; }"); + + verifyNoChange(CLINIT_DECL, + "while(cond) { b++; clinit_A(); }", + "clinit_A();"); + + optimize(CLINIT_DECL, + "clinit_A();", + "while(x() + (clinit_A(), y) > 0) { b++; clinit_A(); }") + .into(CLINIT_DECL, + "clinit_A();", + "while(x() + y > 0) { b++; }"); + + optimize(CLINIT_DECL, + "while(x() + (clinit_A(), y) > 0) { b++; }", + "clinit_A();") + .into(CLINIT_DECL, + "while(x() + (clinit_A(), y) > 0) { b++; }"); + } + + public void testRemoveDupClinitsInFor() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "for(;cond;){ b++; clinit_A(); }") + .into(CLINIT_DECL, + "clinit_A();", + "for(;cond;){ b++; }"); + + verifyNoChange(CLINIT_DECL, + "for(;cond;){ b++; clinit_A(); }", + "clinit_A();"); + + optimize(CLINIT_DECL, + "clinit_A();", + "for (var a = x() + (clinit_A(), y); a < 10; a++) { b++; }") + .into(CLINIT_DECL, + "clinit_A();", + "for (var a = x() + y; a < 10; a++) { b++; }"); + + optimize(CLINIT_DECL, + "for (var a = x() + (clinit_A(), y); a < 10; a++) { b++; }", + "clinit_A();") + .into(CLINIT_DECL, + "for (var a = x() + (clinit_A(), y); a < 10; a++) { b++; }"); + + optimize(CLINIT_DECL, + "clinit_A();", + "for(;x() + (clinit_A(), y) > 0;){ b++; }") + .into(CLINIT_DECL, + "clinit_A();", + "for(;x() + y > 0;){ b++; }"); + + optimize(CLINIT_DECL, + "for(;x() + (clinit_A(), y) > 0;){ b++; }", + "clinit_A();") + .into(CLINIT_DECL, + "for(;x() + (clinit_A(), y) > 0;){ b++; }"); + + // Increment operation might not run, so can't remove later clinits + verifyNoChange(CLINIT_DECL, + "for(var i = 0; i < 10; i += x() + (clinit_A(), y)){ b++; clinit_A(); }", + "clinit_A();"); + + // but it need not run if we are sure the clinit already ran + optimize(CLINIT_DECL, + "clinit_A();", + "for(var i = 0; i < 10; i += x() + (clinit_A(), y)){ b++; }") + .into(CLINIT_DECL, + "clinit_A();", + "for(var i = 0; i < 10; i += x() + y){ b++; }" + ); + optimize(CLINIT_DECL, + "for(var i = x() + (clinit_A(), y); i < 10; i += x() + (clinit_A(), y)){ b++; }") + .into(CLINIT_DECL, + "for(var i = x() + (clinit_A(), y); i < 10; i += x() + y){ b++; }" + ); + optimize(CLINIT_DECL, + "for(var i = 0; i < (clinit_A(), 10); i += x() + (clinit_A(), y)){ b++; }") + .into(CLINIT_DECL, + "for(var i = 0; i < (clinit_A(), 10); i += x() + y){ b++; }" + ); + + // Fails, but increment always runs after the body (at least if there are no "continue" + // statements). + // optimize(CLINIT_DECL, + // "for(var i = 0; i < 10; i += x() + (clinit_A(), y)){ b++; clinit_A(); }") + // .into(CLINIT_DECL, + // "for(var i = 0; i < 10; i += x() + y){ b++; clinit_A(); }" + // ); + // For now, asserting that we can't improve these cases: + optimize(CLINIT_DECL, + "for(var i = 0; i < 10; i += x() + (clinit_A(), y)){ b++; clinit_A(); }") + .noChange(); + } + + public void testRemoveDupClinitsInDo() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "do { b++; clinit_A(); } while(cond);" + ) + .into(CLINIT_DECL, + "clinit_A();", + "do { b++; } while(cond);"); + + // Fails, but the body always runs once (at least if there are no "break"/"continue" + // statements) before the condition or code after it. + // optimize(CLINIT_DECL, + // "do { b++; clinit_B(); } while(cond);", + // "clinit_B();" + // ) + // .into(CLINIT_DECL, + // "do { b++; clinit_B(); } while(cond);"); + // optimize(CLINIT_DECL, + // "do { b++; clinit_A(); } while(x() + (clinit_A(), y) > 0);" + // ) + // .into(CLINIT_DECL, + // "do { b++; clinit_A(); } while(x() + y > 0);"); + // For now, asserting that we can't improve these cases: + optimize(CLINIT_DECL, + "do { b++; clinit_B(); } while(cond);", + "clinit_B();" + ).noChange(); + optimize(CLINIT_DECL, + "do { b++; clinit_A(); } while(x() + (clinit_A(), y) > 0);" + ).noChange(); + + optimize(CLINIT_DECL, + "do { b++; } while(x() + y > (clinit_B(), z));", + "clinit_B();" + ) + .noChange(); + + optimize(CLINIT_DECL, + "clinit_A();", + "do { b++; } while(x() + y > (clinit_B(), z));" + ) + .into(CLINIT_DECL, + "clinit_A();", + "do { b++; } while(x() + y > (clinit_B(), z));"); + } + + public void testKeepClinitsAroundTry() throws Exception { + // Ensure we're careful with exceptions as flow control to avoid a clinit + optimize(CLINIT_DECL, """ + try { + a(); + clinit_A(); + } catch (e) { + clinit_A(); + } + """).noChange(); + } + + public void testRemoveDupClinitsInInvocation() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "alert(a(), (clinit_A(),b()));" + ) + .into(CLINIT_DECL, + "clinit_A();", + "alert(a(), b());"); + optimize(CLINIT_DECL, + "alert(a(), (clinit_A(),b()));", + "clinit_A();" + ) + .into(CLINIT_DECL, + "alert(a(), (clinit_A(),b()));"); + } + public void testRemoveDupClinitsInConditionals() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "alert((clinit_A(), cond) ? a() : b());" + ) + .into(CLINIT_DECL, + "clinit_A();", + "alert(cond ? a() : b());"); + optimize(CLINIT_DECL, + "alert((clinit_A(), cond) ? a() : b());", + "clinit_A();" + ) + .into(CLINIT_DECL, + "alert((clinit_A(), cond) ? a() : b());"); + + optimize(CLINIT_DECL, + "clinit_A();", + "alert(cond ? (clinit_A(), a()) : b());" + ) + .into(CLINIT_DECL, + "clinit_A();", + "alert(cond ? a() : b());"); + verifyNoChange(CLINIT_DECL, + "alert(cond ? (clinit_A(), a()) : b());", + "clinit_A();" + ); + + optimize(CLINIT_DECL, + "clinit_A();", + "alert(cond ? a() : (clinit_A(), b()));" + ) + .into(CLINIT_DECL, + "clinit_A();", + "alert(cond ? a() : b());"); + verifyNoChange(CLINIT_DECL, + "alert(cond ? (clinit_A(), a()) : b());", + "clinit_A();" + ); + } + + public void testRemoveDupClinitsInBooleanOps() throws Exception { + optimize(CLINIT_DECL, + "clinit_A();", + "alert((clinit_A(), cond1) && cond2);" + ) + .into(CLINIT_DECL, + "clinit_A();", + "alert(cond1 && cond2);"); + optimize(CLINIT_DECL, + "clinit_A();", + "alert(cond1 && (clinit_A(), cond2));" + ) + .into(CLINIT_DECL, + "clinit_A();", + "alert(cond1 && cond2);"); + optimize(CLINIT_DECL, + "alert((clinit_A(), cond1) && (clinit_A(), cond2));" + ) + .into(CLINIT_DECL, + "alert((clinit_A(), cond1) && cond2);"); + + optimize(CLINIT_DECL, + "alert((clinit_A(), cond1) && (clinit_B(), cond2));", + "clinit_A();", + "clinit_B();" + ) + .into(CLINIT_DECL, + "alert((clinit_A(), cond1) && (clinit_B(), cond2));", + "clinit_B();"); + + optimize(CLINIT_DECL, + "clinit_A();", + "alert((clinit_A(), cond1) || (clinit_A(), cond2));" + ) + .into(CLINIT_DECL, + "clinit_A();", + "alert(cond1 || cond2);"); + optimize(CLINIT_DECL, + "alert((clinit_A(), cond1) || (clinit_B(), cond2));", + "clinit_A();", + "clinit_B();" + ) + .into(CLINIT_DECL, + "alert((clinit_A(), cond1) || (clinit_B(), cond2));", + "clinit_B();"); + } + + public void testRemoveClinitsInMultiExprs() throws Exception { + optimize(CLINIT_DECL, + "var val = (clinit_A(), cond?(clinit_A(), a()):(clinit_A(), b()));") + .into(CLINIT_DECL, "var val = (clinit_A(), cond?a():b());"); + } + + protected void verifyNoChange(String... input) throws Exception { + optimize(input).into(input); + } + + @Override + protected boolean doOptimize(JsProgram program) { + JsSymbolResolver.exec(program); + int changes = DuplicateClinitRemover.exec(program); + // Duplicate clinits are replaced by nulls, so we need to run static eval to remove them + JsStaticEval.exec(program); + return changes != 0; + } + + @Override + protected void setupJsProgram(JsProgram program) { + new JsModVisitor() { + JsFunction clinitA = null; + JsFunction clinitC = null; + @Override + public void endVisit(JsFunction x, JsContext ctx) { + // Ensure the optimizer knows which methods are clinits, hierarchy + if (x.getName().toString().startsWith("clinit_")) { + x.markAsClinit(); + if (x.getName().toString().endsWith("C")) { + clinitC = x; + } else if (x.getName().toString().endsWith("A")) { + clinitA = x; + } + } + + // Indicate that all methods were compiled from Java source + x.setFromJava(true); + + // Provide a static ref for each function, as if it was from Java source + JsName name = x.getName(); + if (name != null) { + name.setStaticRef(x); + } + } + + @Override + public void endVisit(JsProgram x, JsContext ctx) { + assert clinitA != null && clinitC != null; + clinitC.setSuperClinit(clinitA); + } + }.accept(program); + } +} diff --git a/dev/core/test/com/google/gwt/dev/js/JsDuplicateFunctionRemoverTest.java b/dev/core/test/com/google/gwt/dev/js/JsDuplicateFunctionRemoverTest.java index 6772a46b6a..e5998b20d7 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateFunctionRemoverTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateFunctionRemoverTest.java @@ -26,12 +26,8 @@ import com.google.gwt.dev.js.ast.JsProgram; import com.google.gwt.dev.js.ast.JsScope; import com.google.gwt.dev.js.ast.JsStatement; -import com.google.gwt.dev.js.ast.JsVisitor; -import com.google.gwt.dev.util.DefaultTextOutput; -import com.google.gwt.dev.util.TextOutput; import java.io.StringReader; -import java.lang.reflect.Method; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -51,44 +47,33 @@ public String getFreshName() { } } - // JsDuplicateFunctionRemover does not have a one parameter exec function. Test infrastructure - // call exec(JsProgram) reflectively. - private static class JsDuplicateFunctionRemoverProxy { - static public void exec(JsProgram program) { - JsDuplicateFunctionRemover.exec(program, new MockNameGenerator()); - } - } - public void testDontRemoveCtors() throws Exception { // As fieldref qualifier - assertEquals("function a(){}\n;function b(){}\nb.prototype={};a();b();", - optimize("function a(){};function b(){} b.prototype={}; a(); b();")); + optimize("function a(){};function b(){} b.prototype={}; a(); b();") + .into("function a(){}\n;function b(){}\nb.prototype={};a();b();"); // As parameter - assertEquals( - "function defineClass(a,b){}\n;function a(){}\n;function b(){}\ndefineClass(a,b);a();b();", - optimize("function defineClass(a,b){};function a(){};function b(){}" - + " defineClass(a,b); a(); b();")); + optimize("function defineClass(a,b){};function a(){};function b(){}" + + " defineClass(a,b); a(); b();").into("function defineClass(a,b){}\n;function a(){}\n;function b(){}\ndefineClass(a,b);a();b();"); } public void testRemoveDuplicates() throws Exception { - assertEquals("function a(){}\n;a();a();", - optimize("function a(){};function b(){} a(); b();")); + optimize("function a(){};function b(){} a(); b();") + .into("function a(){}\n;a();a();"); } public void testVirtualRemoveDuplicates() throws Exception { - JsProgram program = new JsProgram(); - String js = "_.method1=function(){};_.method2=function(){};_.method1();_.method2();"; - List input = JsParser.parse(SourceOrigin.UNKNOWN, - program.getScope(), new StringReader(js)); - program.getGlobalBlock().getStatements().addAll(input); + JsProgram program = parseToProgram("_.method1=function(){};", + "_.method2=function(){};", + "_.method1();_.method2();"); // Mark all functions as if they were translated from Java sources. setAllFromJava(program); String firstName = new MockNameGenerator().getFreshName(); - assertEquals("_.method1=" + firstName + ";_.method2=" + firstName + - ";_.method1();_.method2();function " + firstName + "(){}\n", - optimize(program, JsSymbolResolver.class, JsDuplicateFunctionRemoverProxy.class)); + optimize(program).into("_.method1=" + firstName + ";", + "_.method2=" + firstName + ";", + "_.method1();_.method2();", + "function " + firstName + "(){}"); } @@ -117,13 +102,14 @@ public void testDuplicateNamesWithCodeSplitterError() throws Exception { // Mark all functions as if they were translated from Java sources. setAllFromJava(program); - optimize(program, JsSymbolResolver.class, JsDuplicateFunctionRemoverProxy.class); + doOptimize(program); // There should be two distinct dedupped functions here. MockNameGenerator tempFreshNameGenerator = new MockNameGenerator(); String firstName = tempFreshNameGenerator.getFreshName(); String secondName = tempFreshNameGenerator.getFreshName(); + assertNotNull(program.getScope().findExistingName(firstName)); assertNotNull(program.getScope().findExistingName(secondName)); } @@ -153,7 +139,6 @@ public static Map exec(JsProgram jsProgram) { * that had been assigned invalidating the irrevocable decision made by the deduper. */ public void testRerunNamerError() throws Exception { - JsProgram program = new JsProgram(); // Reference to a in _.b is to the top level scope a function where the one in _.c is to the // local a definition. // @@ -164,11 +149,11 @@ public void testRerunNamerError() throws Exception { // but we use them to model functions that refer to names at different scopes. Because this // optimization only runs on JsFunctions that come from Java source this situation does not // happen. - String js = "var c; function a(){return f1;}; function f1() {_.b = function() {return a;} }; " - + "function f2() { var a = null; _.c = function() {return a;} };f1();f2();_.b();_.c();"; - List input = JsParser.parse(SourceOrigin.UNKNOWN, - program.getScope(), new StringReader(js)); - program.getGlobalBlock().getStatements().addAll(input); + JsProgram program = parseToProgram("var c;", + "function a(){return f1;};", + "function f1() {_.b = function() {return a;} };", + "function f2() { var a = null; _.c = function() {return a;} };", + "f1();f2();_.b();_.c();"); // Mark all functions as if they were translated from Java sources. setAllFromJava(program); @@ -184,7 +169,7 @@ public void testRerunNamerError() throws Exception { assertTrue(topScope_a != f2_a); - optimize(program, JsSymbolResolver.class, JsDuplicateFunctionRemoverProxy.class); + doOptimize(program); // collect values assigned to some identifiers. final Map assignments = AssignmentGatherer.exec(program); @@ -206,30 +191,10 @@ public void endVisit(JsFunction func, JsContext ctx) { }.accept(program); } - private String optimize(String js) throws Exception { - return optimizeToSource(js, JsSymbolResolver.class, - JsDuplicateFunctionRemoverProxy.class); - } - - /** - * Optimize a JS program. - * - * @param program the source program - * @param toExec a list of classes that implement - * static void exec(JsProgram) - * @return optimized JS - */ - protected String optimize(JsProgram program, Class... toExec) throws Exception { - - for (Class clazz : toExec) { - Method m = clazz.getMethod("exec", JsProgram.class); - m.invoke(null, program); - } - - TextOutput text = new DefaultTextOutput(true); - JsVisitor generator = new JsSourceGenerationVisitor(text); - - generator.accept(program); - return text.toString(); + @Override + protected boolean doOptimize(JsProgram program) throws Exception { + JsSymbolResolver.exec(program); + JsDuplicateFunctionRemover.exec(program, new MockNameGenerator()); + return true; } } diff --git a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java index e6411df915..c32080a035 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java @@ -37,16 +37,9 @@ */ public class JsInlinerTest extends OptimizerTestBase { - private static class FixStaticRefsVisitor extends JsModVisitor { - - /** - * Called reflectively. - */ - @SuppressWarnings("unused") - public static void exec(JsProgram program) { - (new FixStaticRefsVisitor()).accept(program); - } + private boolean obfuscateSource = false; + private static class FixStaticRefsVisitor extends JsModVisitor { @Override public void endVisit(JsFunction x, JsContext ctx) { JsName name = x.getName(); @@ -395,27 +388,22 @@ public void testPreserveNameScopeWithDoubleInliningAndObfuscation() throws Excep verifyOptimizedObfuscated(expected, code); } - private void verifyNoChange(String input) throws Exception { - verifyOptimized(input, input); + protected void verifyNoChange(String input) throws Exception { + optimize(input).into(input); } private void verifyOptimized(String expected, String input) throws Exception { - String actual = optimizeToSource(input, JsSymbolResolver.class, FixStaticRefsVisitor.class, - JsInlinerProxy.class, JsUnusedFunctionRemover.class); - String expectedAfterParse = optimizeToSource(expected); - assertEquals(expectedAfterParse, actual); + optimize(input).into(expected); } private void verifyOptimizedObfuscated(String expected, String input) throws Exception { - String actual = optimizeToSource(input, JsSymbolResolver.class, FixStaticRefsVisitor.class, - JsInlinerProxy.class, JsUnusedFunctionRemover.class, JsObfuscateNamer.class); - String expectedAfterParse = optimizeToSource(expected); - assertEquals(expectedAfterParse, actual); + obfuscateSource = true; + optimize(input).into(expected); } private void assertCheckerError(String input, String error) throws Exception { - JsProgram optimizedProgram = optimize(input, JsSymbolResolver.class, FixStaticRefsVisitor.class, - JsInlinerProxy.class, JsUnusedFunctionRemover.class); + JsProgram optimizedProgram = (parseToProgram(input)); + doOptimize(optimizedProgram); UnitTestTreeLogger.Builder builder = new UnitTestTreeLogger.Builder(); builder.setLowestLogLevel(TreeLogger.ERROR); builder.expectError(error, null); @@ -428,32 +416,42 @@ private void assertCheckerError(String input, String error) throws Exception { testLogger.assertCorrectLogEntries(); } + @Override + protected boolean doOptimize(JsProgram program) throws JsNamer.IllegalNameException { + JsSymbolResolver.exec(program); + new FixStaticRefsVisitor().accept(program); + boolean madeChanges = doInline(program); + JsUnusedFunctionRemover.exec(program); + if (obfuscateSource) { + JsObfuscateNamer.exec(program); + } + return madeChanges; + } + /** - * A Proxy class to call JsInlner, due to its lack of a single parameter exec method. + * Helper to call JsInliner, and collect the functions we expect to be inlinable. For the purposes + * of this test, all functions are potentially inlinable, albit with different inlining modes + * based on their name. + * @param program the program to optimize */ - private static class JsInlinerProxy { - /** - * Static entry point used by JavaToJavaScriptCompiler. - */ - public static void exec(JsProgram program) { - final List inlineableFunctions = Lists.newArrayList(); - new JsVisitor() { - @Override - public void endVisit(JsFunction x, JsContext ctx) { - inlineableFunctions.add(x); - JsName functionName = x.getName(); - if (functionName == null) { - return; - } - if (functionName.getIdent().endsWith("_forceInline")) { - x.setInliningMode(InliningMode.FORCE_INLINE); - } else if (functionName.getIdent().endsWith("_doNotInline")) { - x.setInliningMode(InliningMode.DO_NOT_INLINE); - } + private static boolean doInline(JsProgram program) { + final List inlineableFunctions = Lists.newArrayList(); + new JsVisitor() { + @Override + public void endVisit(JsFunction x, JsContext ctx) { + inlineableFunctions.add(x); + JsName functionName = x.getName(); + if (functionName == null) { + return; } - }.accept(program); - JsInliner.exec(program, inlineableFunctions); - } + if (functionName.getIdent().endsWith("_forceInline")) { + x.setInliningMode(InliningMode.FORCE_INLINE); + } else if (functionName.getIdent().endsWith("_doNotInline")) { + x.setInliningMode(InliningMode.DO_NOT_INLINE); + } + } + }.accept(program); + int changes = JsInliner.exec(program, inlineableFunctions); + return changes > 0; } - } diff --git a/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java b/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java index 3177b09319..d55a181163 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java @@ -15,74 +15,81 @@ */ package com.google.gwt.dev.js; +import com.google.gwt.dev.js.ast.JsProgram; + /** * Tests the JsStaticEval optimizer. */ public class JsStaticEvalTest extends OptimizerTestBase { public void testAddLiterals() throws Exception { - assertEquals("alert(42);", optimize("alert(21+21);")); - assertEquals("alert('Hello World');", optimize("alert('Hello '+'World');")); - assertEquals("alert('Hello 42');", optimize("alert('Hello ' + 42);")); - assertEquals("alert('42 Hello');", optimize("alert(42 + ' Hello');")); - assertEquals("alert('42 Hello');", optimize("alert(42.0 + ' Hello');")); - assertEquals("alert('42.2 Hello');", optimize("alert(42.2 + ' Hello');")); - assertEquals("alert('Hello 42.2');", optimize("alert('Hello ' + 42.2);")); - assertEquals("alert('2004318071');", optimize("alert(2004318071 + '');")); + optimize("alert(21+21);").into("alert(42);"); + optimize("alert('Hello '+'World');").into("alert('Hello World');"); + optimize("alert('Hello ' + 42);").into("alert('Hello 42');"); + optimize("alert(42 + ' Hello');").into("alert('42 Hello');"); + optimize("alert(42.0 + ' Hello');").into("alert('42 Hello');"); + optimize("alert(42.2 + ' Hello');").into("alert('42.2 Hello');"); + optimize("alert('Hello ' + 42.2);").into("alert('Hello 42.2');"); + optimize("alert(2004318071 + '');").into("alert('2004318071');"); } public void testAssociativity() throws Exception { + // This test method uses optimizeToSource, as the precedence of the "expected" source doesn't + // even match itself after printing without a pass through JsStaticEval. That is, this test + // would fail: + // optimizeJs("alert(a||b||c)").into("alert(a||b||c);"); + // Simple test - assertEquals("alert(a||b||c||d);", optimize("alert((a||b)||(c||d));")); - assertEquals("alert(a||b||c||d||e||f);", optimize("alert((a||b)||(c||(d||(e||f))));")); - assertEquals("alert(a&&b&&c&&d);", optimize("alert((a&&b)&&(c&&d));")); + assertEquals("alert(a||b||c||d);", optimizeToSource("alert((a||b)||(c||d));")); + assertEquals("alert(a||b||c||d||e||f);", optimizeToSource("alert((a||b)||(c||(d||(e||f))));")); + assertEquals("alert(a&&b&&c&&d);", optimizeToSource("alert((a&&b)&&(c&&d));")); // Preserve precedence assertEquals("alert((a||b)&&(c||d));", - optimize("alert((a || b) && (c || d));")); + optimizeToSource("alert((a || b) && (c || d));")); assertEquals("alert(a&&b||c&&d);", - optimize("alert((a && b) || ( c && d));")); - assertEquals("a(),b&&c();", optimize("a(), b && c()")); - assertEquals("a()&&b,c();", optimize("a() && b, c()")); + optimizeToSource("alert((a && b) || ( c && d));")); + assertEquals("a(),b&&c();", optimizeToSource("a(), b && c()")); + assertEquals("a()&&b,c();", optimizeToSource("a() && b, c()")); // Don't damage math expressions assertEquals("alert(seconds/3600);", - optimize("alert(seconds / (60 * 60))")); + optimizeToSource("alert(seconds / (60 * 60))")); assertEquals("alert(seconds/60*60);", - optimize("alert(seconds / 60 * 60)")); - assertEquals("alert(1-(1-foo));", optimize("alert(1 - (1 - foo))")); + optimizeToSource("alert(seconds / 60 * 60)")); + optimize("alert(1 - (1 - foo))").into("alert(1-(1-foo));"); // Don't damage assignments assertEquals("alert((a=0,b=foo));", - optimize("alert((a = 0, b = (bar, foo)))")); + optimizeToSource("alert((a = 0, b = (bar, foo)))")); assertEquals("alert(1+(a='2')+3+4);", - optimize("alert(1 + (a = '2') + 3 + 4);")); + optimizeToSource("alert(1 + (a = '2') + 3 + 4);")); assertEquals("alert(1+(a='2')+7);", - optimize("alert(1 + (a = '2') + (3 + 4));")); + optimizeToSource("alert(1 + (a = '2') + (3 + 4));")); // Break comma expressions up assertEquals("alert((a(),b(),c(),d));", - optimize("alert(((a(),b()),(c(),d)));")); + optimizeToSource("alert(((a(),b()),(c(),d)));")); assertEquals("alert((a(),b(),c(),d));", - optimize("alert(((a(),b()),(c(),d)));")); + optimizeToSource("alert(((a(),b()),(c(),d)));")); // and remove expressions without side effects - assertEquals("alert(d);", optimize("alert(((a,b),(c,d)));")); + assertEquals("alert(d);", optimizeToSource("alert(((a,b),(c,d)));")); // Pattern of coercing a numeric add operation to a string - assertEquals("alert(''+(a+b));", optimize("alert('' + (a + b))")); + optimize("alert('' + (a + b))").into("alert(''+(a+b));"); // Tests involving numeric and string literals and identifiers assertEquals("alert(21+(1+$foo));", - optimize("alert((20 + 1) + (1 + $foo));")); + optimizeToSource("alert((20 + 1) + (1 + $foo));")); // These are also tricky, because $foo could be non-numeric - assertEquals("alert($foo+1+21);", optimize("alert(($foo + 1) + (20 + 1));")); + assertEquals("alert($foo+1+21);", optimizeToSource("alert(($foo + 1) + (20 + 1));")); assertEquals("alert($bar+13+7+(2+$foo));", - optimize("alert((($bar + (10 + 3)) + (2 + 5)) + (2 + $foo));")); + optimizeToSource("alert((($bar + (10 + 3)) + (2 + 5)) + (2 + $foo));")); // Without type info, there's nothing that can be done for this expr assertEquals("alert($foo+($bar+($baz+$quux)));", - optimize("alert($foo + ($bar + ($baz + $quux)));")); + optimizeToSource("alert($foo + ($bar + ($baz + $quux)));")); } /** @@ -92,85 +99,93 @@ public void testAssociativity() throws Exception { public void testDeclareAfterReturn() throws Exception { // TODO(rluble): Note that the source output has the wrong precedence for function definition // and application. - assertEquals("(function(){return 0;var a;var b}());", - optimize("(function(){return 0;{var a;var b}})();")); + optimize("(function(){return 0;{var a;var b}})();") + .into("(function(){return 0;var a;var b}());"); } public void testIfWithEmptyThen() throws Exception { - assertEquals("a();", optimize("if (a()) { }")); + optimize("if (a()) { }").into("a();"); } public void testIfWithEmptyThenAndElseExpression() throws Exception { - assertEquals("a()||b();", optimize("if (a()) { } else { b(); }")); + optimize("if (a()) { } else { b(); }").into("a()||b();"); } public void testIfWithEmptyThenAndElse() throws Exception { - assertEquals("if(!a()){throw 1}", - optimize("if (a()) { } else { throw 1; }")); + optimize("if (a()) { } else { throw 1; }") + .into("if(!a()){throw 1}"); } public void testIfWithEmptyThenAndEmptyElse() throws Exception { - assertEquals("a();", optimize("if (a()) { } else { }")); + optimize("if (a()) { } else { }").into("a();"); } public void testIfWithThenAndEmptyElse() throws Exception { - assertEquals("if(a()){throw 1}", optimize("if (a()) { throw 1; } else { }")); + optimize("if (a()) { throw 1; } else { }").into("if(a()){throw 1}"); } public void testIfWithThenExpressionAndEmptyElse() throws Exception { - assertEquals("a()&&b();", optimize("if (a()) { b() } else { }")); + optimize("if (a()) { b() } else { }").into("a()&&b();"); } public void testIfWithThenExpressionAndElseExpression() throws Exception { - assertEquals("a()?b():c();", optimize("if (a()) { b() } else { c(); }")); + optimize("if (a()) { b() } else { c(); }").into("a()?b():c();"); } public void testIfWithThenExpressionAndElseStatement() throws Exception { - // This can't be optimized further - assertEquals("if(a()){b()}else{throw 1}", - optimize("if (a()) { b() } else { throw 1; }")); + // This can't be optimized further at present + optimize("if (a()) { b() } else { throw 1; }") + .into("if(a()){b()}else{throw 1}"); } public void testLiteralCompares() throws Exception { - assertEquals("alert(false);", optimize("alert(2 != 2)")); - assertEquals("alert(false);", optimize("alert(2 == 3)")); - assertEquals("alert(true);", optimize("alert(2 == 2)")); - assertEquals("alert(true);", optimize("alert(2 != 3)")); - assertEquals("alert(true);", optimize("alert(2 < 3)")); - assertEquals("alert(true);", optimize("alert(3 <= 3)")); - assertEquals("alert(true);", optimize("alert(3 > 2)")); - assertEquals("alert(true);", optimize("alert(3 >= 3)")); - assertEquals("alert(false);", optimize("alert(2 > 3)")); - assertEquals("alert(false);", optimize("alert(2 >= 3)")); - assertEquals("alert(false);", optimize("alert(3 < 2)")); - assertEquals("alert(false);", optimize("alert(3 <= 2)")); - assertEquals("alert(false);", optimize("alert(1.8E+10308 < 1.9E+10308)")); - assertEquals("alert(false);", optimize("alert(1.8E+10308 > 1.9E+10308)")); - assertEquals("alert(true);", optimize("alert(\"a\" == \"a\")")); - assertEquals("alert(true);", optimize("alert(\"a\" === \"a\")")); - assertEquals("alert(true);", optimize("alert(\"a\" != \"b\")")); - assertEquals("alert(true);", optimize("alert(\"a\" !== \"b\")")); - assertEquals("alert(true);", optimize("alert(\"a\" != null)")); - assertEquals("alert(true);", optimize("alert(\"a\" !== null)")); + optimize("alert(2 != 2)").into("alert(false);"); + optimize("alert(2 == 3)").into("alert(false);"); + optimize("alert(2 == 2)").into("alert(true);"); + optimize("alert(2 != 3)").into("alert(true);"); + optimize("alert(2 < 3)").into("alert(true);"); + optimize("alert(3 <= 3)").into("alert(true);"); + optimize("alert(3 > 2)").into("alert(true);"); + optimize("alert(3 >= 3)").into("alert(true);"); + optimize("alert(2 > 3)").into("alert(false);"); + optimize("alert(2 >= 3)").into("alert(false);"); + optimize("alert(3 < 2)").into("alert(false);"); + optimize("alert(3 <= 2)").into("alert(false);"); + optimize("alert(1.8E+10308 < 1.9E+10308)").into("alert(false);"); + optimize("alert(1.8E+10308 > 1.9E+10308)").into("alert(false);"); + + + optimize("alert(\"a\" == \"a\")").into("alert(true);"); + optimize("alert(\"a\" === \"a\")").into("alert(true);"); + optimize("alert(\"a\" != \"b\")").into("alert(true);"); + optimize("alert(\"a\" !== \"b\")").into("alert(true);"); + optimize("alert(\"a\" != null)").into("alert(true);"); + optimize("alert(\"a\" !== null)").into("alert(true);"); } public void testLiteralEqNull() throws Exception { - assertEquals("alert(false);", optimize("alert('test' == null)")); + optimize("alert('test' == null)").into("alert(false);"); } public void testLiteralNeNull() throws Exception { - assertEquals("alert(true);", optimize("alert('test' != null)")); + optimize("alert('test' != null)").into("alert(true);"); } public void testNullEqNull() throws Exception { - assertEquals("alert(true);", optimize("alert(null == null)")); + optimize("alert(null == null)").into("alert(true);"); } public void testNullNeNull() throws Exception { - assertEquals("alert(false);", optimize("alert(null != null)")); + optimize("alert(null != null)").into("alert(false);"); } - private String optimize(String js) throws Exception { - return optimizeToSource(js, JsStaticEval.class); + @Override + protected boolean doOptimize(JsProgram program) { + int changes = JsStaticEval.exec(program); + if (changes != 0) { + // Try one more time, to ensure that it correctly converged in a single run + assertEquals(0, JsStaticEval.exec(program)); + } + return changes != 0; } } diff --git a/dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java b/dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java index 228be8ffec..966b5bb10c 100644 --- a/dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java +++ b/dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java @@ -22,10 +22,10 @@ import com.google.gwt.dev.util.DefaultTextOutput; import com.google.gwt.dev.util.TextOutput; +import com.google.gwt.thirdparty.guava.common.base.Joiner; import junit.framework.TestCase; import java.io.StringReader; -import java.lang.reflect.Method; import java.util.List; /** @@ -33,45 +33,93 @@ */ public abstract class OptimizerTestBase extends TestCase { + /** + * Optimize a JS program, so that the test can also parse the expected value and normalize away + * simple differences. Applies the subclass's setup and optimization steps. + */ + protected Result optimize(String... snippets) throws Exception { + JsProgram program = parseToProgram(snippets); + + return optimize(program); + } + + /** + * Given a built program, applies setup and optimization steps. + */ + protected Result optimize(JsProgram program) throws Exception { + setupJsProgram(program); + + boolean madeChanges = doOptimize(program); + + TextOutput out = new DefaultTextOutput(true); + return new Result(out.toString(), program, madeChanges); + } + + protected static class Result { + private final String originalCode; + private final JsProgram program; + private final boolean madeChanges; + + private Result(String originalCode, JsProgram program, boolean madeChanges) { + this.originalCode = originalCode; + this.program = program; + this.madeChanges = madeChanges; + } + + /** + * Asserts that the optimized program is the same as the expected program by parsing and + * comparing the printed source of both. + * @param expected the expected js program + */ + public void into(String... expected) throws Exception { + JsProgram expectedProgram = new JsProgram(); + List input = JsParser.parse(SourceOrigin.UNKNOWN, + program.getScope(), new StringReader(Joiner.on("").join(expected))); + expectedProgram.getGlobalBlock().getStatements().addAll(input); + + assertEquals(originalCode, expectedProgram.toSource(), program.toSource()); + } + + public void noChange() { + assertFalse(madeChanges); + } + } + /** * Optimize a JS program. * * @param js the source program - * @param toExec a list of classes that implement - * static void exec(JsProgram) * @return optimized JS source */ - protected String optimizeToSource(String js, Class... toExec) throws Exception { - JsProgram program = optimize(js, toExec); + protected String optimizeToSource(String js) throws Exception { + JsProgram program = parseToProgram(js); + doOptimize(program); TextOutput text = new DefaultTextOutput(true); JsVisitor generator = new JsSourceGenerationVisitor(text); - generator.accept(program); return text.toString(); } /** - * Optimize a JS program. - * - * @param js the source program - * @param toExec a list of classes that implement - * static void exec(JsProgram) - * @return optimized JS program + * Helper that only joins strings and parses to a program. */ - protected JsProgram optimize(String js, Class... toExec) throws Exception { + protected JsProgram parseToProgram(String... snippets) throws Exception { JsProgram program = new JsProgram(); List expected = JsParser.parse(SourceOrigin.UNKNOWN, - program.getScope(), new StringReader(js)); + program.getScope(), new StringReader(Joiner.on("").join(snippets))); program.getGlobalBlock().getStatements().addAll(expected); - for (Class clazz : toExec) { - Method m = clazz.getMethod("exec", JsProgram.class); - m.invoke(null, program); - } - return program; } -} \ No newline at end of file + protected abstract boolean doOptimize(JsProgram program) throws Exception; + + /** + * Override this method to provide additional pre-optimization setup of the js program. + * @param program the program to update + */ + protected void setupJsProgram(JsProgram program) { + } +}