[gwt-contrib] [google-web-toolkit] r9362 committed - Partial fix for http://code.google.com/p/google-web-toolkit/issues/det...
Revision: 9362
Author: gwt.mirror...@gmail.com
Date: Mon Dec 6 14:51:53 2010
Log: Partial fix for
http://code.google.com/p/google-web-toolkit/issues/detail?id=5707
Modify JsInliner to avoid propagating expressions incorrectly.
in EvaluationOrderVisitor, two changes were made. First of all, a clinit()
call
is treated as violating order, the same as any other invocation.
Secondly, a JsNameRef that can be affected by side effects (a global,
non-param,
non-local) that is visited when there are still expressions that need
evaluation
first, is treated as violating order.
This fixes the showcase issue, but is still insufficient. Side-effect
inducing
expressions (assignment, post/prefix increment ops) are still not handled.
The
exemption of JsNameRefs that are locals is also insufficient, since a side
effect inducing parameter value may be aliased to the same object as this
local.
After your review, I may remove it and just ban any non-param reference from
occuring while there are still params to be evaluated.
I tested this and it seems to fix the problem.
Review by: sco...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9362
Modified:
/trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java
/trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Mon Sep 20
06:50:17 2010
+++ /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Mon Dec 6
14:51:53 2010
@@ -611,12 +611,37 @@
private boolean maintainsOrder = true;
private final ListJsName toEvaluate;
private final ListJsName unevaluated;
-
-public EvaluationOrderVisitor(ListJsName toEvaluate) {
+private final SetJsName paramsOrLocals = new HashSetJsName();
+
+public EvaluationOrderVisitor(ListJsName toEvaluate, JsFunction
callee) {
this.toEvaluate = toEvaluate;
this.unevaluated = new ArrayListJsName(toEvaluate);
+ // collect params and locals from callee function
+ new JsVisitor() {
+@Override
+public void endVisit(JsParameter x, JsContextJsParameter ctx) {
+ paramsOrLocals.add(x.getName());
+}
+
+@Override
+public boolean visit(JsVar x, JsContextJsVar ctx) {
+ // record this before visiting initializer
+ paramsOrLocals.add(x.getName());
+ return true;
+}
+ }.accept(callee);
}
+/**
+ * Referencing an array breaks order if there are unevaluated
arguments.
+ */
+@Override
+public void endVisit(JsArrayAccess x, JsContextJsExpression ctx) {
+ if (unevaluated.size() 0) {
+maintainsOrder = false;
+ }
+}
+
@Override
public void endVisit(JsBinaryOperation x, JsContextJsExpression ctx)
{
JsBinaryOperator op = x.getOperator();
@@ -669,12 +694,7 @@
*/
@Override
public void endVisit(JsInvocation x, JsContextJsExpression ctx) {
- /*
- * The check for isExecuteOnce() is potentially incorrect here,
however
- * the original Java semantics of the clinit would have made the code
- * incorrect anyway.
- */
- if ((isExecuteOnce(x) == null) unevaluated.size() 0) {
+ if (unevaluated.size() 0) {
maintainsOrder = false;
}
}
@@ -710,11 +730,35 @@
return maintainsOrder unevaluated.size() == 0;
}
+/**
+ * Check to see if the evaluation of this JsName will break program
order assumptions given
+ * the parameters left to be substituted.
+ *
+ * The cases are as follows:
+ * 1) JsName is a function parameter name which has side effects or is
affected by side effects
+ * (hereafter called 'volatile'), so it will be in 'toEvaluate'
+ * 2) JsName is a function parameter which is not volatile (not in
toEvaluate)
+ * 3) JsName is a reference to a global variable
+ * 4) JsName is a reference to a local variable
+ *
+ * A reference to a global while there are still parameters left to
evaluate / substitute
+ * implies an order volation.
+ *
+ * A reference to a volatile parameter is ok if it if is the next
parameter in sequence to
+ * be evaluated (beginning of unevaluated list). Else, it is either
being evaluated out of
+ * order with respect to other parameters, or it is being evaluated
more than once.
+ */
private void checkName(JsName name) {
if (!toEvaluate.contains(name)) {
+// if the name is a non-local/non-parameter (e.g. global) and
there are params left to eval
+if (!paramsOrLocals.contains(name) unevaluated.size() 0) {
+ maintainsOrder = false;
+}
+// else this may be a local, or all volatile params have already
been evaluated, so it's ok.
return;
}
+ // either this param is being evaled twice, or out of order
if (unevaluated.size() == 0 ||
Re: [gwt-contrib] [google-web-toolkit] r9362 committed - Partial fix for http://code.google.com/p/google-web-toolkit/issues/det...
I forgot to edit the log description before I submitted, this isn't a
partial fix, it's a complete fix, although it is more conservative
than it needs to be (an inliner could substitute and propagate
expressions if it did better dataflow analysis). It can be made more
optimal later.
-Ray
On Mon, Dec 6, 2010 at 2:52 PM, codesite-nore...@google.com wrote:
Revision: 9362
Author: gwt.mirror...@gmail.com
Date: Mon Dec 6 14:51:53 2010
Log: Partial fix for
http://code.google.com/p/google-web-toolkit/issues/detail?id=5707
Modify JsInliner to avoid propagating expressions incorrectly.
in EvaluationOrderVisitor, two changes were made. First of all, a clinit()
call
is treated as violating order, the same as any other invocation.
Secondly, a JsNameRef that can be affected by side effects (a global,
non-param,
non-local) that is visited when there are still expressions that need
evaluation
first, is treated as violating order.
This fixes the showcase issue, but is still insufficient. Side-effect
inducing
expressions (assignment, post/prefix increment ops) are still not handled.
The
exemption of JsNameRefs that are locals is also insufficient, since a side
effect inducing parameter value may be aliased to the same object as this
local.
After your review, I may remove it and just ban any non-param reference from
occuring while there are still params to be evaluated.
I tested this and it seems to fix the problem.
Review by: sco...@google.com
http://code.google.com/p/google-web-toolkit/source/detail?r=9362
Modified:
/trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java
/trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
===
--- /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Mon Sep 20
06:50:17 2010
+++ /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Mon Dec 6
14:51:53 2010
@@ -611,12 +611,37 @@
private boolean maintainsOrder = true;
private final ListJsName toEvaluate;
private final ListJsName unevaluated;
-
- public EvaluationOrderVisitor(ListJsName toEvaluate) {
+ private final SetJsName paramsOrLocals = new HashSetJsName();
+
+ public EvaluationOrderVisitor(ListJsName toEvaluate, JsFunction
callee) {
this.toEvaluate = toEvaluate;
this.unevaluated = new ArrayListJsName(toEvaluate);
+ // collect params and locals from callee function
+ new JsVisitor() {
+ �...@override
+ public void endVisit(JsParameter x, JsContextJsParameter ctx) {
+ paramsOrLocals.add(x.getName());
+ }
+
+ �...@override
+ public boolean visit(JsVar x, JsContextJsVar ctx) {
+ // record this before visiting initializer
+ paramsOrLocals.add(x.getName());
+ return true;
+ }
+ }.accept(callee);
}
+ /**
+ * Referencing an array breaks order if there are unevaluated
arguments.
+ */
+ �...@override
+ public void endVisit(JsArrayAccess x, JsContextJsExpression ctx) {
+ if (unevaluated.size() 0) {
+ maintainsOrder = false;
+ }
+ }
+
@Override
public void endVisit(JsBinaryOperation x, JsContextJsExpression ctx) {
JsBinaryOperator op = x.getOperator();
@@ -669,12 +694,7 @@
*/
@Override
public void endVisit(JsInvocation x, JsContextJsExpression ctx) {
- /*
- * The check for isExecuteOnce() is potentially incorrect here,
however
- * the original Java semantics of the clinit would have made the code
- * incorrect anyway.
- */
- if ((isExecuteOnce(x) == null) unevaluated.size() 0) {
+ if (unevaluated.size() 0) {
maintainsOrder = false;
}
}
@@ -710,11 +730,35 @@
return maintainsOrder unevaluated.size() == 0;
}
+ /**
+ * Check to see if the evaluation of this JsName will break program
order assumptions given
+ * the parameters left to be substituted.
+ *
+ * The cases are as follows:
+ * 1) JsName is a function parameter name which has side effects or is
affected by side effects
+ * (hereafter called 'volatile'), so it will be in 'toEvaluate'
+ * 2) JsName is a function parameter which is not volatile (not in
toEvaluate)
+ * 3) JsName is a reference to a global variable
+ * 4) JsName is a reference to a local variable
+ *
+ * A reference to a global while there are still parameters left to
evaluate / substitute
+ * implies an order volation.
+ *
+ * A reference to a volatile parameter is ok if it if is the next
parameter in sequence to
+ * be evaluated (beginning of unevaluated list). Else, it is either
being evaluated out of
+ * order with respect to other parameters, or it is being evaluated
more than once.
+ */
private void checkName(JsName name) {
if (!toEvaluate.contains(name)) {
+ // if the name is a
