From 56f30e10ab61a26cfd8b5a535956516d78ab482f Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 27 Oct 2025 09:27:24 -0500 Subject: [PATCH 01/14] Deduplicate clinits in comma expressions Fixes #9731 --- .../gwt/dev/js/DuplicateClinitRemover.java | 116 ++++++++++------- .../dev/js/JsDuplicateClinitRemoverTest.java | 117 ++++++++++++++++++ 2 files changed, 189 insertions(+), 44 deletions(-) create mode 100644 dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java 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..ebdd1e61ab 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -24,8 +24,6 @@ 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.JsExpression; import com.google.gwt.dev.js.ast.JsFor; import com.google.gwt.dev.js.ast.JsForIn; @@ -78,52 +76,74 @@ public DuplicateClinitRemover(JsProgram program, Set alreadyCalled) /** * 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()); + // This effectively visits any JsInvocation direct child on both sides, so take care to not + // encounter any clinit twice when descending further. + ClinitStatus left = isDuplicateCall(x.getArg1()); + ClinitStatus right = isDuplicateCall(x.getArg2()); - if (left && right) { + if (left == ClinitStatus.DUPLICATE_CLINIT && right == ClinitStatus.DUPLICATE_CLINIT) { /* * (clinit(), clinit()) --> delete or null. - * - * This construct is very unlikely since the InliningVisitor builds - * the comma expressions in a right-nested manner. + * Repeated inlining can cause this, if there is an earlier clinit statement/expr in the + * branch. */ if (ctx.canRemove()) { ctx.removeMe(); - return false; } else { - // The return value from an XO function is never used + // The return value from a clinit is never used ctx.replaceMe(JsNullLiteral.INSTANCE); - return false; } - - } else if (left) { + return false; + } else if (left == ClinitStatus.DUPLICATE_CLINIT) { // (clinit(), xyz) --> xyz - // This is the common case - ctx.replaceMe(accept(x.getArg2())); + // This is the common case for simply-inlined methods/fields. + if (right == ClinitStatus.NEW_CLINIT) { + // Don't re-visit, it was just a clinit and we already observed it + ctx.replaceMe(x.getArg2()); + } else { + assert right == ClinitStatus.NOT_A_CLINIT; + // Save to re-visit, nested clinits could be removed + ctx.replaceMe(accept(x.getArg2())); + } return false; - - } else if (right) { + } else if (right == ClinitStatus.DUPLICATE_CLINIT) { // (xyz, clinit()) --> xyz - // Possible if a clinit() were the last element - ctx.replaceMe(accept(x.getArg1())); + // This can happen with multiple inlined methods, each adding a new clinit for + // the same class, where xyz might be the first clinit, or for a different class. + if (left == ClinitStatus.NEW_CLINIT) { + // Don't re-visit, it was just a clinit and we already observed it + ctx.replaceMe(x.getArg1()); + } else { + assert left == ClinitStatus.NOT_A_CLINIT; + // Even though this is the left, it is safe to visit despite already looking at the right, + // since we know the right isn't a direct duplicate or supertype (we would have hit a + // different branch). + ctx.replaceMe(accept(x.getArg1())); + } return false; } - + // Descend to both sides only if neither is a clinit at all + return right == ClinitStatus.NOT_A_CLINIT && left == ClinitStatus.NOT_A_CLINIT; } 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; } /** @@ -158,21 +178,6 @@ public boolean visit(JsDefault x, JsContext ctx) { return false; } - @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; - } - } - @Override public boolean visit(JsFor x, JsContext ctx) { // The JsFor may have an expression xor a variable declaration. @@ -188,6 +193,7 @@ public boolean visit(JsFor x, JsContext ctx) { } // The increment expression is optional + // TODO this always executes after the body, so could be a sub-branch of that if (x.getIncrExpr() != null) { x.setIncrExpr(branch(x.getIncrExpr())); } @@ -227,10 +233,13 @@ 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; } @@ -280,12 +289,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)); + 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/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..8d4e310e3e --- /dev/null +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java @@ -0,0 +1,117 @@ +/* + * Copyright 2025 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.jjs.SourceOrigin; +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; +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.List; + +/** + * Need tests for + * super clinits + * if/for/while/do/conditional/short-circuit/switch + */ +public class JsDuplicateClinitRemoverTest extends OptimizerTestBase { + public void testRemoveDupClinitsInBlock() throws Exception { + // Explicitly mark a clinit as such, so that the optimizer can identify it. + JsProgram program = new JsProgram(); + String js = "function emptyFunc(){}" + + "function clinit_A(){clinit_A = emptyFunc}" + + "clinit_A();" + + "clinit_A();"; + List input = JsParser.parse(SourceOrigin.UNKNOWN, + program.getScope(), new StringReader(js)); + program.getGlobalBlock().getStatements().addAll(input); + + setupProgram(program); + + String optimized = optimize(program, JsSymbolResolver.class, DuplicateClinitRemover.class, JsStaticEval.class); + assertEquals("function emptyFunc(){}\n" + + "function clinit_A(){clinit_A=emptyFunc}\n" + + "clinit_A();", optimized); + } + + public void testRemoveDupClinitsInExpr() throws Exception { + JsProgram program = new JsProgram(); + String js = "function emptyFunc(){}" + + "function clinit_A(){clinit_A = emptyFunc}" + + "value = (clinit_A(),clinit_A(), 1);"; + List input = JsParser.parse(SourceOrigin.UNKNOWN, + program.getScope(), new StringReader(js)); + program.getGlobalBlock().getStatements().addAll(input); + + setupProgram(program); + + String optimized = optimize(program, JsSymbolResolver.class, DuplicateClinitRemover.class); + assertEquals("function emptyFunc(){}\n" + + "function clinit_A(){clinit_A=emptyFunc}\n" + + "value=(clinit_A(),1);", optimized); + } + + private void setupProgram(JsProgram program) { + new JsModVisitor() { + @Override + public void endVisit(JsFunction x, JsContext ctx) { + // Ensure the optimizer knows which methods are clinits + if (x.getName().toString().startsWith("clinit_")) { + x.markAsClinit(); + } + + // 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); + } + } + }.accept(program); + } + + /** + * 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(); + } +} From f022e3ff0a6dcc08ccdfbf7789b2a6025d5d90a5 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 27 Oct 2025 10:29:40 -0500 Subject: [PATCH 02/14] Run the clinit dedup mid-loop --- .../com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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, From 9974cddb36070c1189bb3c0ac936c6b77ccf91e2 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 16 Feb 2026 13:11:59 -0600 Subject: [PATCH 03/14] Proposed cleanup of js optimizer tests --- .../gwt/dev/js/JsDuplicateCaseFolderTest.java | 32 ++-- .../dev/js/JsDuplicateClinitRemoverTest.java | 83 +++------- .../js/JsDuplicateFunctionRemoverTest.java | 86 +++-------- .../com/google/gwt/dev/js/JsInlinerTest.java | 83 +++++----- .../google/gwt/dev/js/JsStaticEvalTest.java | 146 ++++++++++-------- .../google/gwt/dev/js/OptimizerTestBase.java | 85 +++++++--- 6 files changed, 243 insertions(+), 272 deletions(-) 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..dbf0b53241 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,32 @@ 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 void doOptimize(JsProgram program) throws Exception { + JsDuplicateCaseFolder.exec(program); } } diff --git a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java index 8d4e310e3e..274c0c6ad3 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2025 GWT Project Authors + * 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 @@ -15,20 +15,11 @@ */ package com.google.gwt.dev.js; -import com.google.gwt.dev.jjs.SourceOrigin; 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; -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.List; /** * Need tests for @@ -36,43 +27,33 @@ * if/for/while/do/conditional/short-circuit/switch */ public class JsDuplicateClinitRemoverTest extends OptimizerTestBase { + private static final String CLINIT_DECL = "function emptyFunc(){}" + + "function clinit_A(){clinit_A = emptyFunc}"; public void testRemoveDupClinitsInBlock() throws Exception { - // Explicitly mark a clinit as such, so that the optimizer can identify it. - JsProgram program = new JsProgram(); - String js = "function emptyFunc(){}" + - "function clinit_A(){clinit_A = emptyFunc}" + - "clinit_A();" + - "clinit_A();"; - List input = JsParser.parse(SourceOrigin.UNKNOWN, - program.getScope(), new StringReader(js)); - program.getGlobalBlock().getStatements().addAll(input); - - setupProgram(program); - - String optimized = optimize(program, JsSymbolResolver.class, DuplicateClinitRemover.class, JsStaticEval.class); - assertEquals("function emptyFunc(){}\n" + - "function clinit_A(){clinit_A=emptyFunc}\n" + - "clinit_A();", optimized); + optimize(CLINIT_DECL, + "clinit_A();", + "clinit_A();") + .into(CLINIT_DECL, + "clinit_A()"); } public void testRemoveDupClinitsInExpr() throws Exception { - JsProgram program = new JsProgram(); - String js = "function emptyFunc(){}" + - "function clinit_A(){clinit_A = emptyFunc}" + - "value = (clinit_A(),clinit_A(), 1);"; - List input = JsParser.parse(SourceOrigin.UNKNOWN, - program.getScope(), new StringReader(js)); - program.getGlobalBlock().getStatements().addAll(input); - - setupProgram(program); + optimize(CLINIT_DECL, + "value = (clinit_A(),clinit_A(), 1);") + .into(CLINIT_DECL, + "value=(clinit_A(),1);"); + } - String optimized = optimize(program, JsSymbolResolver.class, DuplicateClinitRemover.class); - assertEquals("function emptyFunc(){}\n" + - "function clinit_A(){clinit_A=emptyFunc}\n" + - "value=(clinit_A(),1);", optimized); + @Override + protected void doOptimize(JsProgram program) { + JsSymbolResolver.exec(program); + DuplicateClinitRemover.exec(program); + // Duplicate clinits are replaced by nulls, so we need to run static eval to remove them + JsStaticEval.exec(program); } - private void setupProgram(JsProgram program) { + @Override + protected void setupJsProgram(JsProgram program) { new JsModVisitor() { @Override public void endVisit(JsFunction x, JsContext ctx) { @@ -92,26 +73,4 @@ public void endVisit(JsFunction x, JsContext ctx) { } }.accept(program); } - - /** - * 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(); - } } 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..bb9c5ed847 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,9 @@ 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 void doOptimize(JsProgram program) throws Exception { + JsSymbolResolver.exec(program); + JsDuplicateFunctionRemover.exec(program, new MockNameGenerator()); } } 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..5ca85c89d6 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(); @@ -396,26 +389,21 @@ public void testPreserveNameScopeWithDoubleInliningAndObfuscation() throws Excep } private void verifyNoChange(String input) throws Exception { - verifyOptimized(input, input); + 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,41 @@ private void assertCheckerError(String input, String error) throws Exception { testLogger.assertCorrectLogEntries(); } + @Override + protected void doOptimize(JsProgram program) throws JsNamer.IllegalNameException { + JsSymbolResolver.exec(program); + new FixStaticRefsVisitor().accept(program); + doInline(program); + JsUnusedFunctionRemover.exec(program); + if (obfuscateSource) { + JsObfuscateNamer.exec(program); + } + } + /** - * 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 void 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); + JsInliner.exec(program, inlineableFunctions); } } 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..dab707b242 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,88 @@ 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 void doOptimize(JsProgram program) { + JsStaticEval.exec(program); } } 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..a3b79c74f5 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,90 @@ */ 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); + + doOptimize(program); + + TextOutput out = new DefaultTextOutput(true); + return new Result(out.toString(), program); + } + + protected static class Result { + private final String originalCode; + private final JsProgram program; + + private Result(String originalCode, JsProgram program) { + this.originalCode = originalCode; + this.program = program; + } + + /** + * 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()); + } + } + /** * 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 void 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) { + } + + } \ No newline at end of file From afae00bcd588003f8191e4eb078fd19960dd8117 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Tue, 17 Feb 2026 15:46:14 -0600 Subject: [PATCH 04/14] Add more tests, more cleanup --- .../gwt/dev/js/DuplicateClinitRemover.java | 116 +++---- .../dev/js/JsDuplicateClinitRemoverTest.java | 305 +++++++++++++++++- .../com/google/gwt/dev/js/JsInlinerTest.java | 2 +- .../google/gwt/dev/js/JsStaticEvalTest.java | 6 +- 4 files changed, 350 insertions(+), 79 deletions(-) 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 ebdd1e61ab..59d23333d9 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -24,6 +24,8 @@ 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.JsExpression; import com.google.gwt.dev.js.ast.JsFor; import com.google.gwt.dev.js.ast.JsForIn; @@ -76,74 +78,52 @@ public DuplicateClinitRemover(JsProgram program, Set alreadyCalled) /** * 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) { - // This effectively visits any JsInvocation direct child on both sides, so take care to not - // encounter any clinit twice when descending further. - ClinitStatus left = isDuplicateCall(x.getArg1()); - ClinitStatus right = isDuplicateCall(x.getArg2()); + boolean left = isDuplicateCall(x.getArg1()); + boolean right = isDuplicateCall(x.getArg2()); - if (left == ClinitStatus.DUPLICATE_CLINIT && right == ClinitStatus.DUPLICATE_CLINIT) { + if (left && right) { /* * (clinit(), clinit()) --> delete or null. - * Repeated inlining can cause this, if there is an earlier clinit statement/expr in the - * branch. + * + * This construct is very unlikely since the InliningVisitor builds + * the comma expressions in a right-nested manner. */ if (ctx.canRemove()) { ctx.removeMe(); + return false; } else { - // The return value from a clinit is never used + // The return value from an XO function is never used ctx.replaceMe(JsNullLiteral.INSTANCE); + return false; } - return false; - } else if (left == ClinitStatus.DUPLICATE_CLINIT) { + + } else if (left) { // (clinit(), xyz) --> xyz - // This is the common case for simply-inlined methods/fields. - if (right == ClinitStatus.NEW_CLINIT) { - // Don't re-visit, it was just a clinit and we already observed it - ctx.replaceMe(x.getArg2()); - } else { - assert right == ClinitStatus.NOT_A_CLINIT; - // Save to re-visit, nested clinits could be removed - ctx.replaceMe(accept(x.getArg2())); - } + // This is the common case + ctx.replaceMe(accept(x.getArg2())); return false; - } else if (right == ClinitStatus.DUPLICATE_CLINIT) { + + } else if (right) { // (xyz, clinit()) --> xyz - // This can happen with multiple inlined methods, each adding a new clinit for - // the same class, where xyz might be the first clinit, or for a different class. - if (left == ClinitStatus.NEW_CLINIT) { - // Don't re-visit, it was just a clinit and we already observed it - ctx.replaceMe(x.getArg1()); - } else { - assert left == ClinitStatus.NOT_A_CLINIT; - // Even though this is the left, it is safe to visit despite already looking at the right, - // since we know the right isn't a direct duplicate or supertype (we would have hit a - // different branch). - ctx.replaceMe(accept(x.getArg1())); - } + // Possible if a clinit() were the last element + ctx.replaceMe(accept(x.getArg1())); return false; } - // Descend to both sides only if neither is a clinit at all - return right == ClinitStatus.NOT_A_CLINIT && left == ClinitStatus.NOT_A_CLINIT; + } 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; } /** @@ -178,6 +158,21 @@ public boolean visit(JsDefault x, JsContext ctx) { return false; } + @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; + } + } + @Override public boolean visit(JsFor x, JsContext ctx) { // The JsFor may have an expression xor a variable declaration. @@ -193,7 +188,6 @@ public boolean visit(JsFor x, JsContext ctx) { } // The increment expression is optional - // TODO this always executes after the body, so could be a sub-branch of that if (x.getIncrExpr() != null) { x.setIncrExpr(branch(x.getIncrExpr())); } @@ -233,13 +227,10 @@ public boolean visit(JsIf x, JsContext ctx) { */ @Override public boolean visit(JsInvocation x, JsContext ctx) { - if (isDuplicateCall(x) == ClinitStatus.DUPLICATE_CLINIT) { - if (ctx.canRemove()) { - ctx.removeMe(); - } else { - ctx.replaceMe(JsNullLiteral.INSTANCE); - } - return false; + JsFunction func = JsUtils.isExecuteOnce(x); + while (func != null) { + called.add(func); + func = func.getSuperClinit(); } return true; } @@ -289,31 +280,12 @@ private T branch(T x) { return toReturn; } - 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) { + private boolean isDuplicateCall(JsExpression x) { if (!(x instanceof JsInvocation)) { - return ClinitStatus.NOT_A_CLINIT; + return false; } JsFunction func = JsUtils.isExecuteOnce((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; + return (func != null && called.contains(func)); } } diff --git a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java index 274c0c6ad3..f3d776be9f 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java @@ -23,12 +23,19 @@ /** * Need tests for - * super clinits - * if/for/while/do/conditional/short-circuit/switch + * 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_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();", @@ -37,13 +44,288 @@ public void testRemoveDupClinitsInBlock() throws Exception { "clinit_A()"); } - public void testRemoveDupClinitsInExpr() throws Exception { + public void ignore_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(); }" + // ); + } + + 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);"); + + optimize(CLINIT_DECL, + "do { b++; } while(x() + y > (clinit_B(), z));", + "clinit_B();" + ) + .into(CLINIT_DECL, + "do { b++; } while(x() + y > (clinit_B(), z));"); + + 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 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();"); + } + + protected void verifyNoChange(String... input) throws Exception { + optimize(input).into(input); + } + @Override protected void doOptimize(JsProgram program) { JsSymbolResolver.exec(program); @@ -55,11 +337,18 @@ protected void doOptimize(JsProgram program) { @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 + // 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 @@ -71,6 +360,12 @@ public void endVisit(JsFunction x, JsContext ctx) { 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/JsInlinerTest.java b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java index 5ca85c89d6..5a29f66a96 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java @@ -388,7 +388,7 @@ public void testPreserveNameScopeWithDoubleInliningAndObfuscation() throws Excep verifyOptimizedObfuscated(expected, code); } - private void verifyNoChange(String input) throws Exception { + protected void verifyNoChange(String input) throws Exception { optimize(input).into(input); } 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 dab707b242..e0db3c57f3 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java @@ -181,6 +181,10 @@ public void testNullNeNull() throws Exception { @Override protected void doOptimize(JsProgram program) { - JsStaticEval.exec(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)); + } } } From 6eadbd92ecf08469b6e8ace8336d2b98f53b98d2 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Tue, 17 Feb 2026 16:03:51 -0600 Subject: [PATCH 05/14] Restore the new impl, restore the new test --- .../gwt/dev/js/DuplicateClinitRemover.java | 116 +++++++++++------- .../dev/js/JsDuplicateClinitRemoverTest.java | 2 +- 2 files changed, 73 insertions(+), 45 deletions(-) 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..ebdd1e61ab 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -24,8 +24,6 @@ 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.JsExpression; import com.google.gwt.dev.js.ast.JsFor; import com.google.gwt.dev.js.ast.JsForIn; @@ -78,52 +76,74 @@ public DuplicateClinitRemover(JsProgram program, Set alreadyCalled) /** * 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()); + // This effectively visits any JsInvocation direct child on both sides, so take care to not + // encounter any clinit twice when descending further. + ClinitStatus left = isDuplicateCall(x.getArg1()); + ClinitStatus right = isDuplicateCall(x.getArg2()); - if (left && right) { + if (left == ClinitStatus.DUPLICATE_CLINIT && right == ClinitStatus.DUPLICATE_CLINIT) { /* * (clinit(), clinit()) --> delete or null. - * - * This construct is very unlikely since the InliningVisitor builds - * the comma expressions in a right-nested manner. + * Repeated inlining can cause this, if there is an earlier clinit statement/expr in the + * branch. */ if (ctx.canRemove()) { ctx.removeMe(); - return false; } else { - // The return value from an XO function is never used + // The return value from a clinit is never used ctx.replaceMe(JsNullLiteral.INSTANCE); - return false; } - - } else if (left) { + return false; + } else if (left == ClinitStatus.DUPLICATE_CLINIT) { // (clinit(), xyz) --> xyz - // This is the common case - ctx.replaceMe(accept(x.getArg2())); + // This is the common case for simply-inlined methods/fields. + if (right == ClinitStatus.NEW_CLINIT) { + // Don't re-visit, it was just a clinit and we already observed it + ctx.replaceMe(x.getArg2()); + } else { + assert right == ClinitStatus.NOT_A_CLINIT; + // Save to re-visit, nested clinits could be removed + ctx.replaceMe(accept(x.getArg2())); + } return false; - - } else if (right) { + } else if (right == ClinitStatus.DUPLICATE_CLINIT) { // (xyz, clinit()) --> xyz - // Possible if a clinit() were the last element - ctx.replaceMe(accept(x.getArg1())); + // This can happen with multiple inlined methods, each adding a new clinit for + // the same class, where xyz might be the first clinit, or for a different class. + if (left == ClinitStatus.NEW_CLINIT) { + // Don't re-visit, it was just a clinit and we already observed it + ctx.replaceMe(x.getArg1()); + } else { + assert left == ClinitStatus.NOT_A_CLINIT; + // Even though this is the left, it is safe to visit despite already looking at the right, + // since we know the right isn't a direct duplicate or supertype (we would have hit a + // different branch). + ctx.replaceMe(accept(x.getArg1())); + } return false; } - + // Descend to both sides only if neither is a clinit at all + return right == ClinitStatus.NOT_A_CLINIT && left == ClinitStatus.NOT_A_CLINIT; } 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; } /** @@ -158,21 +178,6 @@ public boolean visit(JsDefault x, JsContext ctx) { return false; } - @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; - } - } - @Override public boolean visit(JsFor x, JsContext ctx) { // The JsFor may have an expression xor a variable declaration. @@ -188,6 +193,7 @@ public boolean visit(JsFor x, JsContext ctx) { } // The increment expression is optional + // TODO this always executes after the body, so could be a sub-branch of that if (x.getIncrExpr() != null) { x.setIncrExpr(branch(x.getIncrExpr())); } @@ -227,10 +233,13 @@ 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; } @@ -280,12 +289,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)); + 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/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java index f3d776be9f..ad9d87dc94 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java @@ -44,7 +44,7 @@ public void testRemoveDupClinitsInBlock() throws Exception { "clinit_A()"); } - public void ignore_testRemoveDupClinitsInExpr() throws Exception { + public void testRemoveDupClinitsInExpr() throws Exception { optimize(CLINIT_DECL, "value = (clinit_A(),clinit_A(), 1);") .into(CLINIT_DECL, From f5ec57ad927c0097c08d8a932c379e651789f7bf Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Sat, 21 Feb 2026 18:00:47 -0600 Subject: [PATCH 06/14] use a constant where we have it declared --- dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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); From df6e1c5088ae44905d1920f35f55c333d6ff2ed3 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Sat, 21 Feb 2026 18:01:50 -0600 Subject: [PATCH 07/14] move func to only place that uses it --- .../google/gwt/dev/js/DuplicateClinitRemover.java | 14 +++++++++++++- dev/core/src/com/google/gwt/dev/js/JsUtils.java | 12 ------------ 2 files changed, 13 insertions(+), 13 deletions(-) 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 ebdd1e61ab..b3eae7396c 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -73,6 +73,18 @@ public DuplicateClinitRemover(JsProgram program, Set alreadyCalled) 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. @@ -303,7 +315,7 @@ private ClinitStatus isDuplicateCall(JsExpression x) { return ClinitStatus.NOT_A_CLINIT; } - JsFunction func = JsUtils.isExecuteOnce((JsInvocation) x); + JsFunction func = isClinit((JsInvocation) x); if (func != null) { if (called.contains(func)) { return ClinitStatus.DUPLICATE_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. From 47ca1698b65739fd0e84c34a88711e1a15d3410b Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Sat, 18 Jul 2026 16:54:29 -0500 Subject: [PATCH 08/14] Restore only cleaning up clinits at the end --- .../com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 d2f840784a..71c7e34cee 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java +++ b/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java @@ -1022,8 +1022,6 @@ 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)); @@ -1039,6 +1037,10 @@ private void optimizeJsLoop(Collection toInline) throws InterruptedExcep break; } } + + if (optimizationLevel > OptionOptimize.OPTIMIZE_LEVEL_DRAFT) { + DuplicateClinitRemover.exec(jsProgram); + } } private Map renameJsSymbols(PermutationProperties properties, From f2364a92391eb47870292609da2113ceac8ccbf7 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Sat, 18 Jul 2026 18:34:05 -0500 Subject: [PATCH 09/14] Revert "Restore only cleaning up clinits at the end" This reverts commit 47ca1698b65739fd0e84c34a88711e1a15d3410b. --- .../com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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, From f0de544728cb1d263430bceb5c3d89b8ed5457a9 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Tue, 21 Jul 2026 14:53:51 -0500 Subject: [PATCH 10/14] Add more tests, clean up docs, tighten impl a bit --- .../gwt/dev/js/DuplicateClinitRemover.java | 70 +++++++++++++------ .../gwt/dev/js/JsDuplicateCaseFolderTest.java | 3 +- .../dev/js/JsDuplicateClinitRemoverTest.java | 33 +++++++-- .../js/JsDuplicateFunctionRemoverTest.java | 3 +- .../com/google/gwt/dev/js/JsInlinerTest.java | 10 +-- .../google/gwt/dev/js/JsStaticEvalTest.java | 3 +- .../google/gwt/dev/js/OptimizerTestBase.java | 16 +++-- 7 files changed, 101 insertions(+), 37 deletions(-) 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 b3eae7396c..e478931475 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -19,11 +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.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; @@ -34,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; @@ -44,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. @@ -65,12 +67,12 @@ 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); } /** @@ -158,17 +160,6 @@ public boolean visit(JsBinaryOperation x, JsContext ctx) { } } - /** - * 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 public boolean visit(JsCase x, JsContext ctx) { x.setCaseExpr(accept(x.getCaseExpr())); @@ -190,6 +181,16 @@ public boolean visit(JsDefault x, JsContext ctx) { return false; } + @Override + 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 public boolean visit(JsFor x, JsContext ctx) { // The JsFor may have an expression xor a variable declaration. @@ -204,13 +205,13 @@ public boolean visit(JsFor x, JsContext ctx) { x.setCondition(accept(x.getCondition())); } - // The increment expression is optional - // TODO this always executes after the body, so could be a sub-branch of that + // 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; } @@ -223,7 +224,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; } @@ -256,11 +262,35 @@ public boolean visit(JsInvocation x, JsContext ctx) { 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; } 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 dbf0b53241..bd5f6de089 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java @@ -85,7 +85,8 @@ public void test6b() throws Exception { } @Override - protected void doOptimize(JsProgram program) throws Exception { + 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 index ad9d87dc94..488de8e3e6 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java @@ -172,6 +172,7 @@ public void testRemoveDupClinitsInFor() throws Exception { .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, @@ -179,6 +180,10 @@ public void testRemoveDupClinitsInFor() throws Exception { // .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 { @@ -203,13 +208,20 @@ public void testRemoveDupClinitsInDo() throws Exception { // ) // .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();" ) - .into(CLINIT_DECL, - "do { b++; } while(x() + y > (clinit_B(), z));"); + .noChange(); optimize(CLINIT_DECL, "clinit_A();", @@ -220,6 +232,18 @@ public void testRemoveDupClinitsInDo() throws Exception { "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();", @@ -327,11 +351,12 @@ protected void verifyNoChange(String... input) throws Exception { } @Override - protected void doOptimize(JsProgram program) { + protected boolean doOptimize(JsProgram program) { JsSymbolResolver.exec(program); - DuplicateClinitRemover.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 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 bb9c5ed847..e5998b20d7 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateFunctionRemoverTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateFunctionRemoverTest.java @@ -192,8 +192,9 @@ public void endVisit(JsFunction func, JsContext ctx) { } @Override - protected void doOptimize(JsProgram program) throws Exception { + 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 5a29f66a96..ae0c86fcdc 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java @@ -417,14 +417,15 @@ private void assertCheckerError(String input, String error) throws Exception { } @Override - protected void doOptimize(JsProgram program) throws JsNamer.IllegalNameException { + protected boolean doOptimize(JsProgram program) throws JsNamer.IllegalNameException { JsSymbolResolver.exec(program); new FixStaticRefsVisitor().accept(program); - doInline(program); + boolean madeChanges = doInline(program); JsUnusedFunctionRemover.exec(program); if (obfuscateSource) { JsObfuscateNamer.exec(program); } + return madeChanges; } /** @@ -433,7 +434,7 @@ protected void doOptimize(JsProgram program) throws JsNamer.IllegalNameException * based on their name. * @param program the program to optimize */ - private static void doInline(JsProgram program) { + private static boolean doInline(JsProgram program) { final List inlineableFunctions = Lists.newArrayList(); new JsVisitor() { @Override @@ -450,7 +451,8 @@ public void endVisit(JsFunction x, JsContext ctx) { } } }.accept(program); - JsInliner.exec(program, inlineableFunctions); + 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 e0db3c57f3..d55a181163 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java @@ -180,11 +180,12 @@ public void testNullNeNull() throws Exception { } @Override - protected void doOptimize(JsProgram program) { + 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 a3b79c74f5..43f6846abf 100644 --- a/dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java +++ b/dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java @@ -49,19 +49,21 @@ protected Result optimize(String... snippets) throws Exception { protected Result optimize(JsProgram program) throws Exception { setupJsProgram(program); - doOptimize(program); + boolean madeChanges = doOptimize(program); TextOutput out = new DefaultTextOutput(true); - return new Result(out.toString(), program); + 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) { + private Result(String originalCode, JsProgram program, boolean madeChanges) { this.originalCode = originalCode; this.program = program; + this.madeChanges = madeChanges; } /** @@ -77,6 +79,10 @@ public void into(String... expected) throws Exception { assertEquals(originalCode, expectedProgram.toSource(), program.toSource()); } + + public void noChange() { + assertFalse(madeChanges); + } } /** @@ -108,9 +114,7 @@ protected JsProgram parseToProgram(String... snippets) throws Exception { return program; } - protected void doOptimize(JsProgram program) throws Exception { - - } + protected abstract boolean doOptimize(JsProgram program) throws Exception; /** * Override this method to provide additional pre-optimization setup of the js program. From 9133fefd64f0e5ebb0b8966bb7c454a1544e0b70 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 22 Jul 2026 09:55:11 -0500 Subject: [PATCH 11/14] Fix another bug from visual inspection of output --- .../gwt/dev/js/DuplicateClinitRemover.java | 17 ++++++++++++++--- .../dev/js/JsDuplicateClinitRemoverTest.java | 6 ++++++ 2 files changed, 20 insertions(+), 3 deletions(-) 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 e478931475..a0066ea551 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -102,7 +102,9 @@ public boolean visit(JsBinaryOperation x, JsContext ctx) { if (x.getOperator() == JsBinaryOperator.COMMA) { // This effectively visits any JsInvocation direct child on both sides, so take care to not - // encounter any clinit twice when descending further. + // 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. ClinitStatus left = isDuplicateCall(x.getArg1()); ClinitStatus right = isDuplicateCall(x.getArg2()); @@ -147,8 +149,17 @@ public boolean visit(JsBinaryOperation x, JsContext ctx) { } return false; } - // Descend to both sides only if neither is a clinit at all - return right == ClinitStatus.NOT_A_CLINIT && left == ClinitStatus.NOT_A_CLINIT; + // Descend to both sides only if neither is a clinit at all - if just the left is a clinit, + // we must still descend to the right manually + if (right == ClinitStatus.NOT_A_CLINIT) { + if (left == ClinitStatus.NOT_A_CLINIT) { + // descend into both, neither is a clinit + return true; + } + x.setArg2(accept(x.getArg2())); + return false; + } + return false; } else if (x.getOperator().equals(JsBinaryOperator.AND) || x.getOperator().equals(JsBinaryOperator.OR)) { x.setArg1(accept(x.getArg1())); diff --git a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java index 488de8e3e6..04b8d6b43b 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsDuplicateClinitRemoverTest.java @@ -346,6 +346,12 @@ public void testRemoveDupClinitsInBooleanOps() throws Exception { "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); } From 8a66474d7c3903f168fc6af75712e82b3614841f Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 22 Jul 2026 10:14:34 -0500 Subject: [PATCH 12/14] refactor to ensure we're exhaustive this time --- .../gwt/dev/js/DuplicateClinitRemover.java | 105 ++++++++++-------- 1 file changed, 57 insertions(+), 48 deletions(-) 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 a0066ea551..b69c76837f 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -100,66 +100,75 @@ public static JsFunction isClinit(JsInvocation invocation) { @Override public boolean visit(JsBinaryOperation x, JsContext ctx) { if (x.getOperator() == JsBinaryOperator.COMMA) { - - // This effectively visits any JsInvocation direct child on both sides, 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. + // 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()); - ClinitStatus right = isDuplicateCall(x.getArg2()); - - if (left == ClinitStatus.DUPLICATE_CLINIT && 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 (left == ClinitStatus.DUPLICATE_CLINIT) { - // (clinit(), xyz) --> xyz - // This is the common case for simply-inlined methods/fields. - if (right == ClinitStatus.NEW_CLINIT) { + 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 { assert right == ClinitStatus.NOT_A_CLINIT; - // Save to re-visit, nested clinits could be removed + // Safe to re-visit, nested clinits could be removed ctx.replaceMe(accept(x.getArg2())); + return false; } - return false; - } else if (right == ClinitStatus.DUPLICATE_CLINIT) { - // (xyz, clinit()) --> xyz - // This can happen with multiple inlined methods, each adding a new clinit for - // the same class, where xyz might be the first clinit, or for a different class. - if (left == ClinitStatus.NEW_CLINIT) { - // Don't re-visit, it was just a clinit and we already observed it + } else if (left == ClinitStatus.NEW_CLINIT) { + // Don't visit the left, just keep it, decide how to handle the right + 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 left == ClinitStatus.NOT_A_CLINIT; - // Even though this is the left, it is safe to visit despite already looking at the right, - // since we know the right isn't a direct duplicate or supertype (we would have hit a - // different branch). - ctx.replaceMe(accept(x.getArg1())); + assert right == ClinitStatus.NOT_A_CLINIT; + // Safe to re-visit, nested clinits could be removed + x.setArg2(accept(x.getArg2())); + return false; } - return false; - } - // Descend to both sides only if neither is a clinit at all - if just the left is a clinit, - // we must still descend to the right manually - if (right == ClinitStatus.NOT_A_CLINIT) { - if (left == ClinitStatus.NOT_A_CLINIT) { - // descend into both, neither is a clinit - return true; + } else { + assert left == ClinitStatus.NOT_A_CLINIT; + // Visit left before proceeding + x.setArg1(accept(x.getArg1())); + // Now check right to see if we notice a duplicate + ClinitStatus right = isDuplicateCall(x.getArg2()); + if (right == ClinitStatus.DUPLICATE_CLINIT) { + // Duplicate to remove, leaving only the left + ctx.replaceMe(x.getArg1()); + return false; + } else if (right == ClinitStatus.NEW_CLINIT) { + // Keep as-is, both sides are necessary + return false; + } else { + assert right == ClinitStatus.NOT_A_CLINIT; + // Safe to revisit, nested clinits could be removed + x.setArg2(accept(x.getArg2())); + return false; } - x.setArg2(accept(x.getArg2())); - return false; } - return false; } else if (x.getOperator().equals(JsBinaryOperator.AND) || x.getOperator().equals(JsBinaryOperator.OR)) { x.setArg1(accept(x.getArg1())); From 3a8eff699541ab33a7e922f0371f8927ab4cd0e2 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 22 Jul 2026 10:38:46 -0500 Subject: [PATCH 13/14] Deduplicate branches in the new impl --- .../gwt/dev/js/DuplicateClinitRemover.java | 31 ++++++------------- 1 file changed, 9 insertions(+), 22 deletions(-) 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 b69c76837f..a89eabc622 100644 --- a/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java +++ b/dev/core/src/com/google/gwt/dev/js/DuplicateClinitRemover.java @@ -132,10 +132,16 @@ public boolean visit(JsBinaryOperation x, JsContext ctx) { ctx.replaceMe(accept(x.getArg2())); return false; } - } else if (left == ClinitStatus.NEW_CLINIT) { - // Don't visit the left, just keep it, decide how to handle the right - ClinitStatus right = isDuplicateCall(x.getArg2()); + } 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; + } + // 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()); @@ -149,25 +155,6 @@ public boolean visit(JsBinaryOperation x, JsContext ctx) { x.setArg2(accept(x.getArg2())); return false; } - } else { - assert left == ClinitStatus.NOT_A_CLINIT; - // Visit left before proceeding - x.setArg1(accept(x.getArg1())); - // Now check right to see if we notice a duplicate - ClinitStatus right = isDuplicateCall(x.getArg2()); - if (right == ClinitStatus.DUPLICATE_CLINIT) { - // Duplicate to remove, leaving only the left - ctx.replaceMe(x.getArg1()); - return false; - } else if (right == ClinitStatus.NEW_CLINIT) { - // Keep as-is, both sides are necessary - return false; - } else { - assert right == ClinitStatus.NOT_A_CLINIT; - // Safe to revisit, nested clinits could be removed - x.setArg2(accept(x.getArg2())); - return false; - } } } else if (x.getOperator().equals(JsBinaryOperator.AND) || x.getOperator().equals(JsBinaryOperator.OR)) { From c357a10c4295da70ac9c97eaf4efa8cb4922a8fc Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 22 Jul 2026 11:30:36 -0500 Subject: [PATCH 14/14] quick cleanup --- dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java | 1 - dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) 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 ae0c86fcdc..c32080a035 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java @@ -454,5 +454,4 @@ public void endVisit(JsFunction x, JsContext ctx) { int changes = JsInliner.exec(program, inlineableFunctions); 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 43f6846abf..966b5bb10c 100644 --- a/dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java +++ b/dev/core/test/com/google/gwt/dev/js/OptimizerTestBase.java @@ -122,5 +122,4 @@ protected JsProgram parseToProgram(String... snippets) throws Exception { */ protected void setupJsProgram(JsProgram program) { } - - } \ No newline at end of file +}