Skip to content

Commit

Permalink
Allow hoisting of anonymous functions in J2CL clinits.
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124483541
  • Loading branch information
@blickly
michaelthomas authored and blickly committed Jun 9, 2016
1 parent cbb7337 commit d455a72
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 11 deletions.
45 changes: 36 additions & 9 deletions src/com/google/javascript/jscomp/J2clConstantHoisterPass.java
View file
Expand Up @@ -24,6 +24,8 @@
import com.google.javascript.rhino.Node;

import java.util.Collection;
import java.util.HashSet;
import java.util.Set;

/**
* An optimization pass for J2CL-generated code to hoist some constant assignments out clinit method
Expand All @@ -40,21 +42,40 @@ public class J2clConstantHoisterPass implements CompilerPass {
@Override
public void process(Node externs, Node root) {
final Multimap<String, Node> fieldAssignments = ArrayListMultimap.create();
final Set<Node> hoistableFunctions = new HashSet<>();
NodeTraversal.traverseEs6(compiler, root, new AbstractPostOrderCallback() {
@Override
public void visit(NodeTraversal t, Node node, Node parent) {
if (parent != null && NodeUtil.isLValue(node)) {
fieldAssignments.put(node.getQualifiedName(), parent);
Node rValue = NodeUtil.getRValueOfLValue(node);
if (isHoistableFunction(t, parent, rValue)) {
hoistableFunctions.add(rValue);
}
}
}
});

for (Collection<Node> assignments : fieldAssignments.asMap().values()) {
maybeHoistClassField(assignments);
maybeHoistClassField(assignments, hoistableFunctions);
}
}

private void maybeHoistClassField(Collection<Node> assignments) {
/**
* Returns whether the specified rValue is a function which does not receive any variables from
* its containing scope, and is thus 'hoistable'.
*/
private static boolean isHoistableFunction(NodeTraversal t, Node parent, Node rValue) {
// TODO(michaelthomas): This could be improved slightly by not assuming that any variable in the
// outer scope is used in the function.
return rValue != null
&& rValue.isFunction()
&& NodeUtil.isAssignmentOp(parent)
&& t.getScope().getVarCount() == 0;
}

private void maybeHoistClassField(
Collection<Node> assignments, Collection<Node> hoistableFunctions) {
// The field is only assigned twice:
if (assignments.size() != 2) {
return;
Expand All @@ -63,7 +84,7 @@ private void maybeHoistClassField(Collection<Node> assignments) {
Node firstAssignment = Iterables.get(assignments, 0);
Node secondAssignment = Iterables.get(assignments, 1);

// One of them is for field initilization in declaration phase:
// One of them is for field initialization in declaration phase:
if (!isClassFieldInitialization(secondAssignment)) {
return;
}
Expand All @@ -73,7 +94,9 @@ private void maybeHoistClassField(Collection<Node> assignments) {
return;
}
// And it is assigned to a literal value; hence could be used in static eval and safe to move:
if (!isLiteralValue(firstAssignment.getSecondChild())) {
Node firstAssignmentRhs = firstAssignment.getSecondChild();
if (!NodeUtil.isLiteralValue(firstAssignmentRhs, true /* includeFunctions */)
|| (firstAssignmentRhs.isFunction() && !hoistableFunctions.contains(firstAssignmentRhs))) {
return;
}

Expand Down Expand Up @@ -105,11 +128,14 @@ private void hoistConstantLikeField(Node clinitAssignment, Node declarationAssig
declarationInClass.putBooleanProp(Node.IS_CONSTANT_VAR, true);

// Sanity check
checkState(isLiteralValue(declarationAssignedValue));
checkState(NodeUtil.isLiteralValue(declarationAssignedValue, false /* includeFunctions */));
}

private static boolean isClassFieldInitialization(Node node) {
return node.getParent().isScript() && node.isVar();
return node.getParent().isScript()
&& node.isVar()
&& node.getFirstFirstChild() != null
&& NodeUtil.isLiteralValue(node.getFirstFirstChild(), false /* includeFunctions */);
}

private static boolean isClinitFieldAssignment(Node node) {
Expand All @@ -125,10 +151,11 @@ private static boolean isClinitMethod(Node fnNode) {
}

String fnName = NodeUtil.getName(fnNode);
return fnName != null && (fnName.endsWith("$$0clinit") || fnName.endsWith(".$clinit"));
return fnName != null && isClinitMethodName(fnName);
}

private static boolean isLiteralValue(Node node) {
return NodeUtil.isLiteralValue(node, false /* exclude functions */);
private static boolean isClinitMethodName(String methodName) {
return methodName != null
&& (methodName.endsWith("$$0clinit") || methodName.endsWith(".$clinit"));
}
}
52 changes: 50 additions & 2 deletions test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java
View file
Expand Up @@ -30,16 +30,19 @@ public void testHoistClinitConstantAssignments() {
" someClass.$clinit = function() {};",
" someClass$foo = true;",
" someClass$bar = 'hey';",
" someClass$buzz = function() { return someClass$bar; };",
"};",
"var someClass$foo = false;",
"var someClass$bar = null;"),
"var someClass$bar = null;",
"var someClass$buzz = null;"),
LINE_JOINER.join(
"var someClass = /** @constructor */ function() {};",
"someClass.$clinit = function() {",
" someClass.$clinit = function() {};",
"};",
"var someClass$foo = true;",
"var someClass$bar = 'hey';"));
"var someClass$bar = 'hey';",
"var someClass$buzz = function(){ return someClass$bar; };"));
}

public void testHoistClinitConstantAssignments_avoidUnsafe() {
Expand All @@ -63,4 +66,49 @@ public void testHoistClinitConstantAssignments_avoidUnsafe() {
"var someClass$foo2 = false;",
"var someClass$hoo = false;"));
}

/**
* Tests that hoisting works as expected when encountering a clinit that has been devirtualized.
*/
public void testHoistClinitConstantAssignments_devirtualized() {
test(
LINE_JOINER.join(
"var someClass = /** @constructor */ function() {};",
"var someClass$$0clinit = function() {",
" someClass$$0clinit = function() {};",
" someClass$shouldHoist = true;",
" someClass$shouldNotHoist = new Object();",
"};",
"var someClass$shouldHoist = false;",
"var someClass$shouldNotHoist = null;"),
LINE_JOINER.join(
"var someClass = /** @constructor */ function() {};",
"var someClass$$0clinit = function() {",
" someClass$$0clinit = function() {};",
" someClass$shouldNotHoist = new Object();",
"};",
"var someClass$shouldHoist = true;",
"var someClass$shouldNotHoist = null;"));
}

public void testHoistClinitConstantAssignments_avoidUnsafeFunc() {
testSame(
LINE_JOINER.join(
"var someClass = /** @constructor */ function() {};",
"someClass.$clinit = function() {",
" someClass.$clinit = function() {};",
" var x = 10;",
" someClass$foo = function() { return x; };", // Depends on the scope of the clinit.
"};",
"var someClass$foo = null;"));
testSame(
LINE_JOINER.join(
"var someClass = /** @constructor */ function() {};",
"someClass.$clinit = function() {",
" someClass.$clinit = function() {};",
" var x = 10;",
" someClass$foo = function() { return 1; };", // x is assumed to be used
"};",
"var someClass$foo = null;"));
}
}

0 comments on commit d455a72

Please sign in to comment.