Fix for issue #497. Reviewed by gwt.team.mmendez
http://code.google.com/p/google-web-toolkit/issues/detail?id=497
This is one of the most subtle compiler issues I've ever debugged. The
following sequence of events triggers the problem described in the issue.
Assume Foo is a class that implements IFoo.
1) MakeCallsStatic creates a static implementation "Foo.$foo()" of
instance method "Foo.foo()"; tthis moves the body of foo() into $foo(),
and replaces the body of foo() with a call to $foo()
2) MethodInliner inlines calls to Foo.$foo() at all call sites, including
the call from Foo.foo()
3) Pruner prunes Foo.$foo(), because it's been inlined everywhere, there
are no outstanding calls to it
4) More optimizations occur which allow an IFoo type variable to be
tightened to a Foo type variables.
5) MakeCallsStatic sees the tightened variable and statically resolves an
IFoo.foo() call to a Foo.$foo(); unfortunately, $foo() has already been
pruned and there is no process for unpruning
6) For various reasons, the new call to Foo.$foo() does not get inlined at
the new call site (this is a rare case, that it would get inlined
everywhere else, but not here).
7) Foo.$foo() does not get visited during GenerateJavaScriptAST (because
it's pruned); the outstanding non-inlined call to it causes a compiler
error.
My solution addresses step #3 in the problem chain. I modified Pruner so
that Foo.$foo() will not pruned as long as Foo.foo() is live, even if
Foo.$foo() has been inlined into Foo.foo(). On the final pruning pass, we
will go ahead and prune Foo.$foo() if it's unreferenced. By that time,
MakeCallsStatic will never run again because all optimizations will
be done.
However, having $foo() in limbo for some time raised a couple of issues.
I discovered that all of its parameters were being tightening to null,
which led me to two observations.
1) It doesn't make sense to nullflow an unreferenced parameter. Period.
Unlike fields & locals, there is never an implicit null initialization;
the value has to come from SOMEWHERE.
2) It doesn't make sense to make Foo.$foo()'s params tighter than
foo()'s params as long as foo() is still live. This is because
some call site currently pointing to foo() may change and point at
$foo() later, meaning $foo()'s params are now invalidly too tight.
So I made a special case upref from $foo() to foo(), which makes sense
because the normal case is explicit type flow from foo() to $foo() when
inlining does not occur.
I fixed both of the above issues in TypeTightener.
I also changed the semantics of the return value of
tryInlineSimpleMethodCall to differentiate between methods that cannot be
inlined (even in theory) and methods that cannot be inlined at this
particular call site. This prevents a caching bug where a failure to
inline at one site would prevent you from even trying at other sites.
There are also a couple of field sorts mixed into this patch.
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@220 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
index 4df85d9..7ea27b8 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
@@ -30,6 +30,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
/**
* Root for the AST representing an entire Java program.
@@ -105,6 +106,13 @@
* JArrayType>>
*/();
+ private final Map/* <JMethod, JMethod> */instanceToStaticMap = new IdentityHashMap/*
+ * <JMethod,
+ * JMethod>
+ */();
+
+ private List/* <JsonObject> */jsonTypeTable;
+
private final JAbsentArrayDimension literalAbsentArrayDim = new JAbsentArrayDimension(
this);
@@ -120,12 +128,28 @@
private final JBooleanLiteral literalTrue = new JBooleanLiteral(this, true);
+ private final TreeLogger logger;
+
private JField nullField;
private JMethod nullMethod;
+ private Map/* <JReferenceType, Integer> */queryIds;
+
private JMethod rebindCreateMethod;
+ private final RebindOracle rebindOracle;
+
+ private final Map/* <String, JField> */specialFields = new HashMap/*
+ * <String,
+ * JField>
+ */();
+
+ private final Map/* <String, JMethod> */specialMethods = new HashMap/*
+ * <String,
+ * JMethod>
+ */();
+
private final JPrimitiveType typeBoolean = new JPrimitiveType(this,
"boolean", "Z", literalFalse);
@@ -179,29 +203,6 @@
private final JPrimitiveType typeVoid = new JPrimitiveType(this, "void", "V",
null);
- private final Map/* <JMethod, JMethod> */instanceToStaticMap = new IdentityHashMap/*
- * <JMethod,
- * JMethod>
- */();
-
- private List/* <JsonObject> */jsonTypeTable;
-
- private final TreeLogger logger;
-
- private Map/* <JReferenceType, Integer> */queryIds;
-
- private final RebindOracle rebindOracle;
-
- private final Map/* <String, JField> */specialFields = new HashMap/*
- * <String,
- * JField>
- */();
-
- private final Map/* <String, JMethod> */specialMethods = new HashMap/*
- * <String,
- * JMethod>
- */();
-
public JProgram(TreeLogger logger, RebindOracle rebindOracle) {
super(null);
this.logger = logger;
@@ -654,6 +655,21 @@
this.queryIds = queryIds;
}
+ /**
+ * If <code>method</code> is a static impl method, returns the instance
+ * method that <code>method</code> is the implementation of. Otherwise,
+ * returns <code>null</code>.
+ */
+ public JMethod staticImplFor(JMethod method) {
+ for (Iterator it = instanceToStaticMap.entrySet().iterator(); it.hasNext();) {
+ Map.Entry entry = (Entry) it.next();
+ if (entry.getValue() == method) {
+ return (JMethod) entry.getKey();
+ }
+ }
+ return null;
+ }
+
public JReferenceType strongerType(JReferenceType type1, JReferenceType type2) {
if (type1 == type2) {
return type1;
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
index 78d45ec..3cce0f6 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
@@ -108,18 +108,18 @@
}
List/* <JStatement> */stmts = method.body.statements;
- boolean didInline = false;
+ boolean possibleToInline = false;
if (stmts.isEmpty()) {
- didInline = inlineEmptyMethodCall(x, m, method);
+ possibleToInline = inlineEmptyMethodCall(x, m, method);
} else if (stmts.size() == 1) {
JStatement stmt = (JStatement) stmts.get(0);
if (stmt instanceof JReturnStatement) {
- didInline = tryInlineSimpleMethodCall(x, m, method,
+ possibleToInline = tryInlineSimpleMethodCall(x, m, method,
(JReturnStatement) stmt);
}
}
- if (!didInline) {
+ if (!possibleToInline) {
cannotInline.add(method);
}
}
@@ -155,8 +155,8 @@
// just reference the same JLiteral
/*
* hackish: pretend there is an arg that is returned which comes after
- * all the real args; this allows the evaluation order check below to
- * succeed
+ * all the real args; this allows the evaluation order check in
+ * tryInlineSimpleMethodCall to succeed
*/
magicArg[0] = args.size();
return new Holder(targetReturnExpr);
@@ -237,7 +237,7 @@
JMultiExpression multi = new JMultiExpression(program);
- // Evaluate the instance argument (we can one even with static calls)
+ // Evaluate the instance argument (we can have one even with static calls)
JExpression instance = x.getInstance();
if (instance != null && instance.hasSideEffects()) {
changes.addExpression(x.instance, multi.exprs);
@@ -258,8 +258,11 @@
* Due to the way we construct multis, the magic arg must come last.
* However, we've encountered a case where an argument coming after
* the magic arg must be evaluated. Just bail.
+ *
+ * However, we return true because this call might be inlinable at
+ * other call sites.
*/
- return false;
+ return true;
}
}
}
@@ -333,9 +336,8 @@
return new MethodInliner(program).execImpl();
}
- private final JProgram program;
-
private JMethod currentMethod;
+ private final JProgram program;
private MethodInliner(JProgram program) {
this.program = program;
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java b/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
index 288fe33..a9c22a4 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
@@ -94,7 +94,7 @@
}
for (int i = 0; i < type.methods.size(); ++i) {
JMethod it = (JMethod) type.methods.get(i);
- if (!referencedNonTypes.contains(it)
+ if (!methodIsReferenced(it)
|| pruneViaNoninstantiability(isInstantiated, it)) {
changeList.removeMethod(it);
}
@@ -119,7 +119,7 @@
for (int i = 1; i < type.methods.size(); ++i) {
JMethod it = (JMethod) type.methods.get(i);
// all other interface methods are instance and abstract
- if (!isInstantiated || !referencedNonTypes.contains(it)) {
+ if (!isInstantiated || !methodIsReferenced(it)) {
changeList.removeMethod(it);
}
}
@@ -141,6 +141,31 @@
return false;
}
+ /**
+ * Returns <code>true</code> if a method is referenced.
+ */
+ private boolean methodIsReferenced(JMethod method) {
+ // Is the method directly referenced?
+ if (referencedNonTypes.contains(method)) {
+ return true;
+ }
+
+ /*
+ * Special case: if method is the static impl for a live instance method,
+ * don't prune it unless this is the final prune.
+ *
+ * In some cases, the staticImpl can be inlined into the instance method
+ * but still be needed at other call sites.
+ */
+ JMethod staticImplFor = program.staticImplFor(method);
+ if (staticImplFor != null && referencedNonTypes.contains(staticImplFor)) {
+ if (noSpecialTypes) {
+ return true;
+ }
+ }
+ return false;
+ }
+
private boolean pruneViaNoninstantiability(boolean isInstantiated, JField it) {
return (!isInstantiated && !it.isStatic());
}
@@ -480,8 +505,8 @@
*/
private class UpRefVisitor extends JVisitor {
- private final RescueVisitor rescuer;
private boolean didRescue = false;
+ private final RescueVisitor rescuer;
public UpRefVisitor(RescueVisitor rescuer) {
this.rescuer = rescuer;
@@ -529,14 +554,14 @@
return new Pruner(program, noSpecialTypes).execImpl();
}
- private final JProgram program;
-
private final boolean noSpecialTypes;
- private final Set/* <JReferenceType> */referencedTypes = new HashSet/* <JReferenceType> */();
+ private final JProgram program;
private final Set/* <JNode> */referencedNonTypes = new HashSet/* <JNode> */();
+ private final Set/* <JReferenceType> */referencedTypes = new HashSet/* <JReferenceType> */();
+
private JMethod stringValueOfChar = null;
private Pruner(JProgram program, boolean noSpecialTypes) {
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java b/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
index 633c3c9..8136f94 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
@@ -298,7 +298,25 @@
List/* <JMethod> */overrides = x.overrides;
JMethod[] virtualOverrides = program.typeOracle.getAllVirtualOverrides(x);
- if (overrides.isEmpty() && virtualOverrides.length == 0) {
+ /*
+ * Special case: also add upRefs from a staticImpl's params to the params
+ * of the instance method it is implementing. Most of the time, this would
+ * happen naturally since the instance method delegates to the static.
+ * However, in cases where the static has been inlined into the instance
+ * method, future optimization could tighten an instance call into a
+ * static call at some other call site, and fail to inline. If we allowed
+ * a staticImpl param to be tighter than its instance param, badness would
+ * ensue.
+ */
+ JMethod staticImplFor = program.staticImplFor(x);
+ // Unless the instance method has already been pruned, of course.
+ if (staticImplFor != null
+ && !staticImplFor.getEnclosingType().methods.contains(staticImplFor)) {
+ staticImplFor = null;
+ }
+
+ if (overrides.isEmpty() && virtualOverrides.length == 0
+ && staticImplFor == null) {
return true;
}
@@ -319,6 +337,11 @@
JParameter baseParam = (JParameter) baseMethod.params.get(j);
set.add(baseParam);
}
+ if (staticImplFor != null && j > 1) {
+ // static impls have an extra first "this" arg
+ JParameter baseParam = (JParameter) staticImplFor.params.get(j - 1);
+ set.add(baseParam);
+ }
}
return true;
@@ -481,11 +504,17 @@
List/* <JReferenceType> */typeList = new ArrayList/* <JReferenceType> */();
/*
- * Always assume at least one null assignment; if there really aren't any
- * other assignments, then this variable will get the null type. If there
- * are, it won't hurt anything because null type will always lose.
+ * For non-parameters, always assume at least one null assignment; if
+ * there really aren't any other assignments, then this variable will get
+ * the null type. If there are, it won't hurt anything because null type
+ * will always lose.
+ *
+ * For parameters, don't perform any tightening if we can't find any
+ * actual assignments. The method should eventually get pruned.
*/
- typeList.add(typeNull);
+ if (!(x instanceof JParameter)) {
+ typeList.add(typeNull);
+ }
Set/* <JExpression> */myAssignments = (Set) assignments.get(x);
if (myAssignments != null) {
@@ -509,6 +538,10 @@
}
}
+ if (typeList.isEmpty()) {
+ return;
+ }
+
JReferenceType resultType = program.generalizeTypes(typeList);
resultType = program.strongerType(refType, resultType);
if (refType != resultType) {
@@ -531,18 +564,13 @@
set.add(value);
}
- private final JProgram program;
- private final JNullType typeNull;
-
- private final Map/* <JReferenceType, Set<JClassType>> */implementors = new IdentityHashMap();
-
- private final Map/* <JMethod, Set<JExpression>> */returns = new IdentityHashMap();
-
- private final Map/* <JMethod, Set<JMethod>> */overriders = new IdentityHashMap();
-
private final Map/* <JVariable, Set<JExpression>> */assignments = new IdentityHashMap();
-
+ private final Map/* <JReferenceType, Set<JClassType>> */implementors = new IdentityHashMap();
+ private final Map/* <JMethod, Set<JMethod>> */overriders = new IdentityHashMap();
private final Map/* <JParameter, Set<JParameter>> */paramUpRefs = new IdentityHashMap();
+ private final JProgram program;
+ private final Map/* <JMethod, Set<JExpression>> */returns = new IdentityHashMap();
+ private final JNullType typeNull;
private TypeTightener(JProgram program) {
this.program = program;
