Revision: 7915
Author: sp...@google.com
Date: Tue Apr 13 10:38:07 2010
Log: Rolling back in r7776, which improves JsInliner, along with r7913 and
r7914, which fix
bugs in it. The merge commands were:
svn merge --ignore-ancestry -r 7775:7776
https://google-web-toolkit.googlecode.com/svn/trunk
svn merge --ignore-ancestry -r 7912:7914
https://google-web-toolkit.googlecode.com/svn/trunk
http://code.google.com/p/google-web-toolkit/source/detail?r=7915
Modified:
/branches/snapshot-2010.03.29-r7809/dev/core/src/com/google/gwt/dev/js/JsHoister.java
/branches/snapshot-2010.03.29-r7809/dev/core/src/com/google/gwt/dev/js/JsInliner.java
/branches/snapshot-2010.03.29-r7809/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
=======================================
---
/branches/snapshot-2010.03.29-r7809/dev/core/src/com/google/gwt/dev/js/JsHoister.java
Mon Apr 5 14:44:40 2010
+++
/branches/snapshot-2010.03.29-r7809/dev/core/src/com/google/gwt/dev/js/JsHoister.java
Tue Apr 13 10:38:07 2010
@@ -221,16 +221,9 @@
stack.push(x);
}
- /**
- * A "this" reference can only effectively be hoisted if the call site
is
- * qualified by a JsNameRef, so we'll ignore this for now.
- */
@Override
public void endVisit(JsThisRef x, JsContext<JsExpression> ctx) {
- // Set a flag to indicate that we cannot continue, and push a null so
- // we don't run out of elements on the stack.
- successful = false;
- stack.push(null);
+ stack.push(new JsThisRef(x.getSourceInfo()));
}
public JsExpression getExpression() {
=======================================
---
/branches/snapshot-2010.03.29-r7809/dev/core/src/com/google/gwt/dev/js/JsInliner.java
Mon Apr 5 14:44:40 2010
+++
/branches/snapshot-2010.03.29-r7809/dev/core/src/com/google/gwt/dev/js/JsInliner.java
Tue Apr 13 10:38:07 2010
@@ -136,7 +136,7 @@
*/
affectedBySideEffects = true;
}
-
+
@Override
public void endVisit(JsObjectLiteral x, JsContext<JsExpression> ctx) {
affectedBySideEffects = true;
@@ -600,6 +600,9 @@
* invocations occur.
*/
private static class EvaluationOrderVisitor extends JsVisitor {
+ public static final JsName THIS_NAME = (new JsScope("fake scope") {
+ }).declareName("this");
+
private boolean maintainsOrder = true;
private final List<JsName> toEvaluate;
private final List<JsName> unevaluated;
@@ -670,11 +673,39 @@
maintainsOrder = false;
}
}
-
+
@Override
public void endVisit(JsNameRef x, JsContext<JsExpression> ctx) {
- JsName name = x.getName();
-
+ checkName(x.getName());
+ }
+
+ @Override
+ public void endVisit(JsNew x, JsContext<JsExpression> ctx) {
+ /*
+ * Unless all arguments have already been evaluated, assume
+ * that invoking the new expression might interfere with
+ * the evaluation of the argument.
+ *
+ * It would be possible to allow this if the invoked function
+ * either does nothing or does nothing that affects the
+ * remaining arguments. However, currently there is no
+ * analysis of the invoked function.
+ */
+ if (unevaluated.size() > 0) {
+ maintainsOrder = false;
+ }
+ }
+
+ @Override
+ public void endVisit(JsThisRef x, JsContext<JsExpression> ctx) {
+ checkName(THIS_NAME);
+ }
+
+ public boolean maintainsOrder() {
+ return maintainsOrder && unevaluated.size() == 0;
+ }
+
+ private void checkName(JsName name) {
if (!toEvaluate.contains(name)) {
return;
}
@@ -683,10 +714,6 @@
maintainsOrder = false;
}
}
-
- public boolean maintainsOrder() {
- return maintainsOrder && unevaluated.size() == 0;
- }
/**
* Determine if an expression contains a reference to a strict
parameter.
@@ -913,6 +940,7 @@
}
inlining.push(invokedFunction);
+ x = tryToUnravelExplicitCall(x);
JsExpression op = process(x, callerFunction, invokedFunction);
if (x != op) {
@@ -1030,6 +1058,7 @@
List<JsExpression> hoisted = new ArrayList<JsExpression>(
statements.size());
+ JsExpression thisExpr = ((JsNameRef)
x.getQualifier()).getQualifier();
List<JsName> localVariableNames = new ArrayList<JsName>();
boolean sawReturnStatement = false;
@@ -1102,13 +1131,14 @@
}
// Confirm that the expression conforms to the desired heuristics
- if (!isInlinable(program, callerFunction, invokedFunction,
+ if (!isInlinable(program, callerFunction, invokedFunction, thisExpr,
x.getArguments(), op)) {
return x;
}
// Perform the name replacement
- NameRefReplacerVisitor v = new NameRefReplacerVisitor(x,
invokedFunction);
+ NameRefReplacerVisitor v = new NameRefReplacerVisitor(thisExpr,
+ x.getArguments(), invokedFunction.getParameters());
for (ListIterator<JsName> nameIterator =
localVariableNames.listIterator(); nameIterator.hasNext();) {
JsName name = nameIterator.next();
@@ -1180,21 +1210,12 @@
@Override
public void endVisit(JsInvocation x, JsContext<JsExpression> ctx) {
- JsFunction function = isFunction(x.getQualifier());
- if (function != null) {
- Integer count = invocationCount.get(function);
- if (count == null) {
- assert (!removingCounts);
- count = 1;
- } else {
- if (removingCounts) {
- count -= 1;
- } else {
- count += 1;
- }
- }
- invocationCount.put(function, count);
- }
+ checkFunctionCall(x.getQualifier());
+ }
+
+ @Override
+ public void endVisit(JsNew x, JsContext<JsExpression> ctx) {
+ checkFunctionCall(x.getConstructorExpression());
}
public Integer invocationCount(JsFunction f) {
@@ -1210,6 +1231,24 @@
accept(expr);
removingCounts = false;
}
+
+ private void checkFunctionCall(JsExpression qualifier) {
+ JsFunction function = isFunction(qualifier);
+ if (function != null) {
+ Integer count = invocationCount.get(function);
+ if (count == null) {
+ assert (!removingCounts);
+ count = 1;
+ } else {
+ if (removingCounts) {
+ count -= 1;
+ } else {
+ count += 1;
+ }
+ }
+ invocationCount.put(function, count);
+ }
+ }
}
/**
@@ -1228,15 +1267,13 @@
final Map<JsName, JsExpression> paramsToArgsMap = new
IdentityHashMap<JsName, JsExpression>();
/**
- * Constructor.
- *
- * @param invocation The call site
- * @param function The function that encloses the inlined statement
+ * A replacement expression for this references.
*/
- public NameRefReplacerVisitor(JsInvocation invocation, JsFunction
function) {
- List<JsParameter> parameters = function.getParameters();
- List<JsExpression> arguments = invocation.getArguments();
-
+ private JsExpression thisExpr;
+
+ public NameRefReplacerVisitor(JsExpression thisExpr,
+ List<JsExpression> arguments, List<JsParameter> parameters) {
+ this.thisExpr = thisExpr;
if (parameters.size() != arguments.size()) {
// This shouldn't happen if the cloned JsInvocation has been
properly
// configured
@@ -1269,6 +1306,12 @@
ctx.replaceMe(replacement);
}
}
+
+ @Override
+ public void endVisit(JsThisRef x, JsContext<JsExpression> ctx) {
+ assert thisExpr != null;
+ ctx.replaceMe(thisExpr);
+ }
/**
* Set a replacement JsName for all references to a JsName.
@@ -1325,25 +1368,36 @@
/**
* Detects uses of parameters that would produce incorrect results if
inlined.
- * Generally speaking, we disallow the use of parameters as lvalues.
+ * Generally speaking, we disallow the use of parameters as lvalues. Also
+ * detects trying to inline a method which references 'this' where the
call
+ * site has no qualifier.
*/
private static class ParameterUsageVisitor extends JsVisitor {
- private boolean lvalue = false;
+ private final boolean hasThisExpr;
private final Set<JsName> parameterNames;
-
- public ParameterUsageVisitor(Set<JsName> parameterNames) {
+ private boolean violation = false;
+
+ public ParameterUsageVisitor(boolean hasThisExpr, Set<JsName>
parameterNames) {
+ this.hasThisExpr = hasThisExpr;
this.parameterNames = parameterNames;
}
@Override
public void endVisit(JsNameRef x, JsContext<JsExpression> ctx) {
if (ctx.isLvalue() && isParameter(x)) {
- lvalue = true;
+ violation = true;
}
}
- public boolean parameterAsLValue() {
- return lvalue;
+ @Override
+ public void endVisit(JsThisRef x, JsContext<JsExpression> ctx) {
+ if (!hasThisExpr) {
+ violation = true;
+ }
+ }
+
+ public boolean hasViolation() {
+ return violation;
}
/**
@@ -1753,6 +1807,13 @@
private static JsFunction isFunction(JsExpression e) {
if (e instanceof JsNameRef) {
JsNameRef ref = (JsNameRef) e;
+
+ // Unravel foo.call(...).
+ if (!ref.getName().isObfuscatable()
&& "call".equals(ref.getIdent())) {
+ if (ref.getQualifier() instanceof JsNameRef) {
+ ref = (JsNameRef) ref.getQualifier();
+ }
+ }
JsNode staticRef = ref.getName().getStaticRef();
if (staticRef instanceof JsFunction) {
@@ -1767,7 +1828,8 @@
* Determine if a statement can be inlined into a call site.
*/
private static boolean isInlinable(JsProgram program, JsFunction caller,
- JsFunction callee, List<JsExpression> arguments, JsNode<?> toInline)
{
+ JsFunction callee, JsExpression thisExpr, List<JsExpression>
arguments,
+ JsNode<?> toInline) {
/*
* This will happen with varargs-style JavaScript functions that rely
on the
@@ -1809,19 +1871,31 @@
if (hasCommonIdents(arguments, toInline, parameterIdents)) {
return false;
}
+
+ List<JsExpression> evalArgs;
+ if (thisExpr == null) {
+ evalArgs = arguments;
+ } else {
+ evalArgs = new ArrayList<JsExpression>(1 + arguments.size());
+ evalArgs.add(thisExpr);
+ evalArgs.addAll(arguments);
+ }
/*
* Determine if the evaluation of the invocation's arguments may
create side
* effects. This will determine how aggressively the parameters may be
* reordered.
*/
- if (isVolatile(program, arguments, caller)) {
+ if (isVolatile(program, evalArgs, caller)) {
/*
* Determine the order in which the parameters must be evaluated.
This
* will vary between call sites, based on whether or not the
invocation's
* arguments can be repeated without ill effect.
*/
List<JsName> requiredOrder = new ArrayList<JsName>();
+ if (thisExpr != null && isVolatile(program, thisExpr, callee)) {
+ requiredOrder.add(EvaluationOrderVisitor.THIS_NAME);
+ }
for (int i = 0; i < arguments.size(); i++) {
JsExpression e = arguments.get(i);
JsParameter p = callee.getParameters().get(i);
@@ -1848,9 +1922,10 @@
}
// Check that parameters aren't used in such a way as to prohibit
inlining
- ParameterUsageVisitor v = new ParameterUsageVisitor(parameterNames);
+ ParameterUsageVisitor v = new ParameterUsageVisitor(thisExpr != null,
+ parameterNames);
v.accept(toInline);
- if (v.parameterAsLValue()) {
+ if (v.hasViolation()) {
return false;
}
@@ -1887,6 +1962,34 @@
return hasSideEffects(list)
|| affectedBySideEffects(program, list, context);
}
+
+ /**
+ * Transforms any <code>foo.call(this)</code> into
<code>this.foo()</code> to
+ * be compatible with our inlining algorithm.
+ */
+ private static JsInvocation tryToUnravelExplicitCall(JsInvocation x) {
+ if (!(x.getQualifier() instanceof JsNameRef)) {
+ return x;
+ }
+ JsNameRef ref = (JsNameRef) x.getQualifier();
+ if (ref.getName().isObfuscatable() || !"call".equals(ref.getIdent())) {
+ return x;
+ }
+ List<JsExpression> oldArgs = x.getArguments();
+ if (oldArgs.size() < 1) {
+ return x;
+ }
+
+ JsNameRef oldTarget = (JsNameRef) ref.getQualifier();
+ JsNameRef newTarget = new JsNameRef(oldTarget.getSourceInfo(),
+ oldTarget.getName());
+ newTarget.setQualifier(oldArgs.get(0));
+ JsInvocation newCall = new JsInvocation(x.getSourceInfo());
+ newCall.setQualifier(newTarget);
+ // Don't have to clone because the returned invocation is transient.
+ newCall.getArguments().addAll(oldArgs.subList(1, oldArgs.size()));
+ return newCall;
+ }
/**
* Utility class.
=======================================
---
/branches/snapshot-2010.03.29-r7809/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
Wed Oct 28 09:10:53 2009
+++
/branches/snapshot-2010.03.29-r7809/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
Tue Apr 13 10:38:07 2010
@@ -79,6 +79,29 @@
+ "function c1() { return ex2 ? a1() :c1(); } c1()";
compare(expected, input);
}
+
+ /**
+ * Test that a new expression breaks argument ordering.
+ */
+ public void testOrderingNew() throws Exception {
+ StringBuffer code = new StringBuffer();
+ // A static variable x
+ code.append("var x;");
+
+ // foo() uses x
+ code.append("function foo() { alert('x = ' + x); }");
+
+ // callee does "new foo" before evaluating its argument
+ code.append("function callee(arg) { new foo(); return arg; }");
+
+ // caller invokes callee with a multi that initializes x
+ code.append("function caller() { callee((x=1,2)); }");
+
+ // bootstrap the program
+ code.append("caller();");
+
+ compare(code.toString(), code.toString());
+ }
public void testSelfRecursion() throws Exception {
String input = "function a1() { return blah && b1() }"
@@ -94,7 +117,6 @@
input = optimize(input, JsSymbolResolver.class,
FixStaticRefsVisitor.class,
JsInliner.class, JsUnusedFunctionRemover.class);
expected = optimize(expected);
- System.err.println("Input vs ");
assertEquals(expected, input);
}
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
To unsubscribe, reply using "remove me" as the subject.
