From 57fd8fa5fa5373ec7ae74fa1ce494605c1d2b4ed Mon Sep 17 00:00:00 2001 From: goktug Date: Mon, 18 Sep 2017 19:41:26 -0700 Subject: [PATCH] Optimize more J2CL clinit patterns. When clinit X just exists to call another clinit Y, we can replace all clinit X reference with clinit Y which will result in clinit X to be unused and get pruned. And that is exactly what this CL does. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=169185339 --- .../jscomp/J2clClinitPrunerPass.java | 81 ++++++++++++++----- .../jscomp/J2clClinitPrunerPassTest.java | 31 +++++++ 2 files changed, 93 insertions(+), 19 deletions(-) diff --git a/src/com/google/javascript/jscomp/J2clClinitPrunerPass.java b/src/com/google/javascript/jscomp/J2clClinitPrunerPass.java index 323d8263ea8..9937841eb9d 100644 --- a/src/com/google/javascript/jscomp/J2clClinitPrunerPass.java +++ b/src/com/google/javascript/jscomp/J2clClinitPrunerPass.java @@ -29,14 +29,17 @@ import java.util.Collection; import java.util.Deque; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; +import java.util.Map.Entry; import java.util.Set; import javax.annotation.Nullable; /** An optimization pass to prune J2CL clinits. */ public class J2clClinitPrunerPass implements CompilerPass { - private final Set emptiedClinitMethods = new HashSet<>(); + private final Map emptiedClinitMethods = new LinkedHashMap<>(); private final AbstractCompiler compiler; private final List changedScopeNodes; @@ -93,6 +96,21 @@ private void pruneEmptyClinits(Node root, List changedScopes) { emptiedClinitMethods.clear(); NodeTraversal.traverseEs6ScopeRoots( compiler, root, changedScopes, new EmptyClinitPruner(), false); + + // Make sure replacements are to final destination instead of pointing intermediate ones. + for (Entry clinitReplacementEntry : emptiedClinitMethods.entrySet()) { + clinitReplacementEntry.setValue(resolveReplacement(clinitReplacementEntry.getValue())); + } + } + + private Node resolveReplacement(Node node) { + if (node == null) { + return null; + } + String clinitName = getClinitMethodName(node); + return emptiedClinitMethods.containsKey(clinitName) + ? resolveReplacement(emptiedClinitMethods.get(clinitName)) + : node; } private Multimap collectClinitReferences(Node root) { @@ -103,7 +121,7 @@ private Multimap collectClinitReferences(Node root) { new AbstractPostOrderCallback() { @Override public void visit(NodeTraversal t, Node node, Node parent) { - String clinitName = node.isCall() ? getClinitMethodName(node.getFirstChild()) : null; + String clinitName = getClinitMethodName(node); if (clinitName != null) { clinitReferences.put(clinitName, node); } @@ -114,11 +132,23 @@ public void visit(NodeTraversal t, Node node, Node parent) { private List cleanEmptyClinitReferences(Multimap clinitReferences) { final List newChangedScopes = new ArrayList<>(); - for (String clinitName : emptiedClinitMethods) { + // resolveReplacement step above should not require any particular iteration order of the + // emptiedClinitMethods but we are using LinkedHashMap to be extra safe. + for (Entry clinitReplacementEntry : emptiedClinitMethods.entrySet()) { + String clinitName = clinitReplacementEntry.getKey(); + Node replacement = clinitReplacementEntry.getValue(); + Collection references = clinitReferences.removeAll(clinitName); for (Node reference : references) { Node changedScope = NodeUtil.getEnclosingChangeScopeRoot(reference.getParent()); - NodeUtil.deleteFunctionCall(reference, compiler); + if (replacement == null) { + NodeUtil.deleteFunctionCall(reference, compiler); + } else { + replacement = replacement.cloneTree(); + reference.replaceWith(replacement); + compiler.reportChangeToChangeScope(changedScope); + clinitReferences.put(getClinitMethodName(replacement), replacement); + } newChangedScopes.add(changedScope); } } @@ -181,7 +211,7 @@ public void visit(NodeTraversal t, Node node, Node parent) { } private void tryRemovingClinit(Node node) { - String clinitName = node.isCall() ? getClinitMethodName(node.getFirstChild()) : null; + String clinitName = getClinitMethodName(node); if (clinitName == null) { return; } @@ -219,8 +249,7 @@ public void visit(NodeTraversal t, Node node, Node parent) { } // Find clinit calls. - String clinitName = - node.getFirstChild().isCall() ? getClinitMethodName(node.getFirstFirstChild()) : null; + String clinitName = getClinitMethodName(node.getFirstChild()); if (clinitName == null) { return; } @@ -310,8 +339,7 @@ private boolean callsClinit(Node fnNode, String clinitName) { Node child = fnNode.getLastChild().getFirstChild(); return child != null && child.isExprResult() - && child.getFirstChild().isCall() - && clinitName.equals(getClinitMethodName(child.getFirstFirstChild())); + && clinitName.equals(getClinitMethodName(child.getFirstChild())); } } @@ -341,18 +369,30 @@ private void trySubstituteEmptyFunction(Node fnNode) { return; } - // Ensure that the first expression in the body is setting itself to the empty function and - // there are no other expressions. + // Ensure that the first expression in the body is setting itself to the empty function Node firstExpr = body.getFirstChild(); - if (!isAssignToEmptyFn(firstExpr, fnQualifiedName) || firstExpr.getNext() != null) { + if (!isAssignToEmptyFn(firstExpr, fnQualifiedName)) { return; } - body.removeChild(firstExpr); - NodeUtil.markFunctionsDeleted(firstExpr, compiler); - compiler.reportChangeToEnclosingScope(body); + Node secondExpr = firstExpr.getNext(); + Node replaceReferencesWith; + if (secondExpr == null) { + // There are no expressions in clinit so it is noop; remove all references. + replaceReferencesWith = null; + } else if (secondExpr.getNext() == null + && secondExpr.isExprResult() + && getClinitMethodName(secondExpr.getFirstChild()) != null) { + // Only expression in clinit is a call to another clinit. We can safely replace all + // references with the other clinit. + replaceReferencesWith = secondExpr.getFirstChild(); + } else { + // Clinit is not empty. + return; + } - emptiedClinitMethods.add(fnQualifiedName); + emptiedClinitMethods.put(fnQualifiedName, replaceReferencesWith); + NodeUtil.deleteNode(firstExpr, compiler); } private boolean isAssignToEmptyFn(Node node, String enclosingFnName) { @@ -370,9 +410,12 @@ private static boolean isClinitMethod(Node node) { return node.isFunction() && isClinitMethodName(NodeUtil.getName(node)); } - private static String getClinitMethodName(Node fnNode) { - String fnName = fnNode.getQualifiedName(); - return isClinitMethodName(fnName) ? fnName : null; + private static String getClinitMethodName(Node node) { + if (node.isCall()) { + String fnName = node.getFirstChild().getQualifiedName(); + return isClinitMethodName(fnName) ? fnName : null; + } + return null; } private static boolean isClinitMethodName(String fnName) { diff --git a/test/com/google/javascript/jscomp/J2clClinitPrunerPassTest.java b/test/com/google/javascript/jscomp/J2clClinitPrunerPassTest.java index 52d860d392d..5b636803a2e 100644 --- a/test/com/google/javascript/jscomp/J2clClinitPrunerPassTest.java +++ b/test/com/google/javascript/jscomp/J2clClinitPrunerPassTest.java @@ -422,6 +422,37 @@ public void testFoldClinit_classHierarchy() { "someChildClass.someFunction = function() {};")); } + public void testFoldClinit_classHierarchyNonEmpty() { + test( + LINE_JOINER.join( + "var someClass = {};", + "someClass.$clinit = function() {", + " someClass.$clinit = function() {};", + " somefn();", + "};", + "var someChildClass = {};", + "someChildClass.$clinit = function() {", + " someChildClass.$clinit = function() {};", + " someClass.$clinit();", + "};", + "someChildClass.someFunction = function() {", + " someChildClass.$clinit();", + "};"), + LINE_JOINER.join( + "var someClass = {};", + "someClass.$clinit = function() {", + " someClass.$clinit = function() {};", + " somefn();", + "};", + "var someChildClass = {};", + "someChildClass.$clinit = function() {", + " someClass.$clinit();", + "};", + "someChildClass.someFunction = function() {", + " someClass.$clinit();", + "};")); + } + public void testFoldClinit_invalidCandidates() { testSame( LINE_JOINER.join(