John Stalcup has uploaded a new change for review.
https://gwt-review.googlesource.com/3300
Change subject: The job of defineSeed is to mark the existence of a class
and associate its castMap with the class's various constructors.
......................................................................
The job of defineSeed is to mark the existence of a class and associate its
castMap with the class's various constructors.
FragmentExtractor chooses whether and what part of a defineSeed call to
include in a fragment.
Previously this logic would only include a defineSeed function in a
fragment if the fragment was known to make the particular type in
question "live".
In this usage "live" means that if you have ALL other fragments loaded then
you can not instantiate this type but the moment you additionally load
this fragment you become able to instantiate this type.
But it is possible that when you have ALL other fragments loaded you can
already instantiate this type and so when you load this fragment you do not
expand the "liveness" of this type but you DO suddenly make certain of its
constructors accessible that were not previously accessible. In this
situation previously logic would still not have included any defineSeed
function at all and as a consequence these newly live constructors were
never
getting their associated castMaps which led to runtime errors when casting
an instance that had been created by one of these broken constructors.
This CL relaxes the defineSeed inclusion restriction to include fragments
that are expanding a type's constructors liveness and not just fragments
that expand a type's liveness.
Change-Id: Idc87430b94338d289eae4760a1bef8ad127bd699
---
M dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
M dev/core/src/com/google/gwt/dev/js/ast/JsName.java
M dev/core/src/com/google/gwt/dev/js/ast/JsRootScope.java
A dev/core/test/com/google/gwt/dev/jjs/impl/FragmentExtractorTest.java
4 files changed, 332 insertions(+), 80 deletions(-)
diff --git
a/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
b/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
index 8d75fc9..d77b7d2 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
@@ -15,6 +15,7 @@
*/
package com.google.gwt.dev.jjs.impl;
+import com.google.gwt.core.client.impl.AsyncFragmentLoader;
import com.google.gwt.dev.jjs.SourceInfo;
import com.google.gwt.dev.jjs.ast.JClassType;
import com.google.gwt.dev.jjs.ast.JConstructor;
@@ -48,10 +49,25 @@
import java.util.Set;
/**
- * A class that extracts a fragment of code based on a supplied liveness
- * condition.
+ * Extracts multiple JS statements (called a fragment) out of the complete
JS program based on
+ * supplied type/method/field/string liveness conditions.
+ *
+ * <p>
+ * <b>Liveness as defined here is not an intuitive concept.</b> A type or
method (note that
+ * constructors are methods) is considered live for the current fragment
when that type can only be
+ * instantiated or method executed when the current fragment has already
been loaded. That does not
+ * always mean that it was caused by direct execution of the current
fragment. It may instead mean
+ * that direction execution of some other fragment has been affected by
the loading of the current
+ * fragment in a way that results in the instantiation of the type or
execution of the method. It is
+ * this second case that can lead to seemingly contradictory but valid
situations like having a type
+ * which is not currently live but which has a currently live constructor.
For example it might be
+ * possible to instantiate type Foo even with fragment Bar being loaded
(i.e. Foo is not live for
+ * Bar) but the loading of fragment Bar might be required to reach a
particular one of Bar's
+ * multiple constructor (i.e. that constructor is live for Bar).
+ * </p>
*/
public class FragmentExtractor {
+
/**
* A {@link LivenessPredicate} that bases liveness on a single
* {@link ControlFlowAnalyzer}.
@@ -63,23 +79,28 @@
this.cfa = cfa;
}
+ @Override
public boolean isLive(JDeclaredType type) {
return cfa.getInstantiatedTypes().contains(type);
}
+ @Override
public boolean isLive(JField field) {
return cfa.getLiveFieldsAndMethods().contains(field)
|| cfa.getFieldsWritten().contains(field);
}
+ @Override
public boolean isLive(JMethod method) {
return cfa.getLiveFieldsAndMethods().contains(method);
}
+ @Override
public boolean isLive(String string) {
return cfa.getLiveStrings().contains(string);
}
+ @Override
public boolean miscellaneousStatementsAreLive() {
return true;
}
@@ -132,38 +153,99 @@
* A {@link LivenessPredicate} where nothing is alive.
*/
public static class NothingAlivePredicate implements LivenessPredicate {
+ @Override
public boolean isLive(JDeclaredType type) {
return false;
}
+ @Override
public boolean isLive(JField field) {
return false;
}
+ @Override
public boolean isLive(JMethod method) {
return false;
}
+ @Override
public boolean isLive(String string) {
return false;
}
+ @Override
public boolean miscellaneousStatementsAreLive() {
return false;
}
}
/**
- * A logger for statements that the fragment extractor encounters.
Install one
- * using
- * {@link
FragmentExtractor#setStatementLogger(com.google.gwt.fragserv.FragmentExtractor.StatementLogger)}
- * .
+ * A logger for statements that the fragment extractor encounters.
Install one using
+ * {@link FragmentExtractor#setStatementLogger(StatementLogger)} .
*/
public static interface StatementLogger {
void logStatement(JsStatement stat, boolean isIncluded);
}
+ /**
+ * Mutates the provided defineSeed statement to remove references to
constructors which have not
+ * been made live by the current fragment. It also counts the
constructors the references that
+ * were not removed.
+ */
+ private class DefineSeedMinimizerVisitor extends JsModVisitor {
+
+ private final LivenessPredicate alreadyLoadedPredicate;
+ private final LivenessPredicate livenessPredicate;
+ private int liveConstructorCount;
+
+ private DefineSeedMinimizerVisitor(
+ LivenessPredicate alreadyLoadedPredicate, LivenessPredicate
livenessPredicate) {
+ this.alreadyLoadedPredicate = alreadyLoadedPredicate;
+ this.livenessPredicate = livenessPredicate;
+ }
+
+ @Override
+ public void endVisit(JsNameRef x, JsContext ctx) {
+ JMethod method = map.nameToMethod(x.getName());
+ if (!(method instanceof JConstructor)) {
+ return;
+ }
+ // Only examines references to constructor methods.
+
+ JConstructor constructor = (JConstructor) method;
+ boolean fragmentExpandsConstructorLiveness =
+ !alreadyLoadedPredicate.isLive(constructor) &&
livenessPredicate.isLive(constructor);
+ if (fragmentExpandsConstructorLiveness) {
+ // Counts kept references to live constructors.
+ liveConstructorCount++;
+ } else {
+ // Removes references to dead constructors.
+ ctx.removeMe();
+ }
+ }
+
+ /**
+ * Enables varargs mutation.
+ */
+ @Override
+ protected <T extends JsVisitable> void doAcceptList(List<T>
collection) {
+ doAcceptWithInsertRemove(collection);
+ }
+ }
+
+ private static class MinimalDefineSeedResult {
+
+ private int liveConstructorCount;
+ private JsExprStmt statement;
+
+ public MinimalDefineSeedResult(JsExprStmt statement, int
liveConstructorCount) {
+ this.statement = statement;
+ this.liveConstructorCount = liveConstructorCount;
+ }
+ }
+
private static class NullStatementLogger implements StatementLogger {
+ @Override
public void logStatement(JsStatement method, boolean isIncluded) {
}
}
@@ -189,6 +271,13 @@
}
return map.vtableInitToMethod(stat);
+ }
+
+ private static JsExprStmt createDefineSeedClone(JsExprStmt
defineSeedStatement) {
+ Cloner cloner = new Cloner();
+ cloner.accept(defineSeedStatement.getExpression());
+ JsExprStmt minimalDefineSeedStatement =
cloner.getExpression().makeStmt();
+ return minimalDefineSeedStatement;
}
private final JProgram jprogram;
@@ -230,8 +319,8 @@
* ensure that <code>livenessPredicate</code> includes strictly more
live code
* than <code>alreadyLoadedPredicate</code>.
*/
- public List<JsStatement> extractStatements(LivenessPredicate
livenessPredicate,
- LivenessPredicate alreadyLoadedPredicate) {
+ public List<JsStatement> extractStatements(
+ LivenessPredicate livenessPredicate, LivenessPredicate
alreadyLoadedPredicate) {
List<JsStatement> extractedStats = new ArrayList<JsStatement>();
/**
@@ -241,52 +330,48 @@
JClassType pendingVtableType = null;
JsExprStmt pendingDefineSeed = null;
+ List<JsStatement> statements =
jsprogram.getGlobalBlock().getStatements();
+ for (JsStatement statement : statements) {
- // Since we haven't run yet.
- assert jsprogram.getFragmentCount() == 1;
+ boolean keep;
+ JClassType vtableTypeAssigned = vtableTypeAssigned(statement);
+ if (vtableTypeAssigned != null) {
+ // Keeps defineSeed statements of live types or types with a live
constructor.
+ MinimalDefineSeedResult minimalDefineSeedResult =
createMinimalDefineSeed(
+ livenessPredicate, alreadyLoadedPredicate, (JsExprStmt)
statement);
+ boolean liveType
= !alreadyLoadedPredicate.isLive(vtableTypeAssigned)
+ && livenessPredicate.isLive(vtableTypeAssigned);
+ boolean liveConstructors =
minimalDefineSeedResult.liveConstructorCount > 0;
- List<JsStatement> stats = jsprogram.getGlobalBlock().getStatements();
- for (JsStatement stat : stats) {
-
- boolean keepIt;
- JClassType vtableTypeAssigned = vtableTypeAssigned(stat);
- if (vtableTypeAssigned != null
- && livenessPredicate.isLive(vtableTypeAssigned)) {
- boolean[] anyCtorsSetup = new boolean[1];
- JsExprStmt result =
maybeRemoveCtorsFromDefineSeedStmt(livenessPredicate,
- alreadyLoadedPredicate, stat, anyCtorsSetup);
- boolean anyWorkDone = anyCtorsSetup[0]
- || !alreadyLoadedPredicate.isLive(vtableTypeAssigned);
- if (anyWorkDone) {
- stat = result;
- keepIt = true;
+ if (liveConstructors || liveType) {
+ statement = minimalDefineSeedResult.statement;
+ keep = true;
} else {
- pendingDefineSeed = result;
+ pendingDefineSeed = minimalDefineSeedResult.statement;
pendingVtableType = vtableTypeAssigned;
- keepIt = false;
+ keep = false;
}
- } else if (containsRemovableVars(stat)) {
- stat = removeSomeVars((JsVars) stat, livenessPredicate,
alreadyLoadedPredicate);
- keepIt = !(stat instanceof JsEmpty);
+ } else if (containsRemovableVars(statement)) {
+ statement = removeSomeVars((JsVars) statement, livenessPredicate,
alreadyLoadedPredicate);
+ keep = !(statement instanceof JsEmpty);
} else {
- keepIt = isLive(stat, livenessPredicate) && !isLive(stat,
alreadyLoadedPredicate);
+ keep = isLive(statement, livenessPredicate) && !isLive(statement,
alreadyLoadedPredicate);
}
- statementLogger.logStatement(stat, keepIt);
+ statementLogger.logStatement(statement, keep);
- if (keepIt) {
+ if (keep) {
if (vtableTypeAssigned != null) {
currentVtableType = vtableTypeAssigned;
}
- JClassType vtableType = vtableTypeNeeded(stat);
+ JClassType vtableType = vtableTypeNeeded(statement);
if (vtableType != null && vtableType != currentVtableType) {
- assert pendingVtableType == vtableType;
extractedStats.add(pendingDefineSeed);
currentVtableType = pendingVtableType;
pendingDefineSeed = null;
pendingVtableType = null;
}
- extractedStats.add(stat);
+ extractedStats.add(statement);
}
}
@@ -316,12 +401,10 @@
}
/**
- * Check whether this statement is a <code>JsVars</code> that contains
- * individual vars that could be removed. If it does, then
- * {@link #removeSomeVars(JsVars, LivenessPredicate, LivenessPredicate)}
is
- * sensible for this statement and should be used instead of
- * {@link #isLive(JsStatement,
com.google.gwt.fragserv.FragmentExtractor.LivenessPredicate)}
- * .
+ * Check whether this statement is a {@link JsVars} that contains
individual vars that could be
+ * removed. If it does, then {@link #removeSomeVars(JsVars,
LivenessPredicate, LivenessPredicate)}
+ * is sensible for this statement and should be used instead of
+ * {@link #isLive(JsStatement, LivenessPredicate)} .
*/
private boolean containsRemovableVars(JsStatement stat) {
if (stat instanceof JsVars) {
@@ -334,6 +417,26 @@
}
}
return false;
+ }
+
+ /**
+ * DefineSeeds mark the existence of a class and associate a castMaps
with the class's various
+ * constructors. These multiple constructors are provided as JsNameRef
varargs to the defineSeed
+ * call but only the constructors that are live in the current fragment
should be included.
+ *
+ * <p>
+ * This function strips out the dead constructors and returns the
modified defineSeed call. The
+ * stripped constructors will be kept by other defineSeed calls in other
fragments at other times.
+ * </p>
+ */
+ private MinimalDefineSeedResult
createMinimalDefineSeed(LivenessPredicate livenessPredicate,
+ LivenessPredicate alreadyLoadedPredicate, JsExprStmt
defineSeedStatement) {
+ DefineSeedMinimizerVisitor defineSeedMinimizerVisitor =
+ new DefineSeedMinimizerVisitor(alreadyLoadedPredicate,
livenessPredicate);
+ JsExprStmt minimalDefineSeedStatement =
createDefineSeedClone(defineSeedStatement);
+ defineSeedMinimizerVisitor.accept(minimalDefineSeedStatement);
+ return new MinimalDefineSeedResult(
+ minimalDefineSeedStatement,
defineSeedMinimizerVisitor.liveConstructorCount);
}
private boolean isLive(JsStatement stat, LivenessPredicate
livenessPredicate) {
@@ -379,42 +482,6 @@
// It's not an intern variable at all
return livenessPredicate.miscellaneousStatementsAreLive();
- }
-
- /**
- * Weird case: the seed function's liveness is associated with the type
- * itself. However, individual constructors can have a liveness that is a
- * subset of the type's liveness.
- */
- private JsExprStmt maybeRemoveCtorsFromDefineSeedStmt(
- final LivenessPredicate livenessPredicate,
- final LivenessPredicate alreadyLoadedPredicate, JsStatement stat,
- final boolean[] anyCtorsSetup) {
- Cloner c = new Cloner();
- c.accept(((JsExprStmt) stat).getExpression());
- JsExprStmt result = c.getExpression().makeStmt();
- new JsModVisitor() {
- public void endVisit(JsNameRef x, JsContext ctx) {
- JMethod maybeCtor = map.nameToMethod(x.getName());
- if (maybeCtor instanceof JConstructor) {
- JConstructor ctor = (JConstructor) maybeCtor;
- if (!livenessPredicate.isLive(ctor)
- || alreadyLoadedPredicate.isLive(ctor)) {
- ctx.removeMe();
- } else {
- anyCtorsSetup[0] = true;
- }
- }
- };
-
- /**
- * Overridden to allow insert/remove on the varargs portion.
- */
- protected <T extends JsVisitable> void doAcceptList(List<T>
collection) {
- doAcceptWithInsertRemove(collection);
- };
- }.accept(result);
- return result;
}
/**
diff --git a/dev/core/src/com/google/gwt/dev/js/ast/JsName.java
b/dev/core/src/com/google/gwt/dev/js/ast/JsName.java
index 460b53e..edc11ab 100644
--- a/dev/core/src/com/google/gwt/dev/js/ast/JsName.java
+++ b/dev/core/src/com/google/gwt/dev/js/ast/JsName.java
@@ -35,7 +35,7 @@
/**
* @param ident the unmangled ident to use for this name
*/
- JsName(JsScope enclosing, String ident, String shortIdent) {
+ public JsName(JsScope enclosing, String ident, String shortIdent) {
this.enclosing = enclosing;
this.ident = StringInterner.get().intern(ident);
this.shortIdent = StringInterner.get().intern(shortIdent);
diff --git a/dev/core/src/com/google/gwt/dev/js/ast/JsRootScope.java
b/dev/core/src/com/google/gwt/dev/js/ast/JsRootScope.java
index b214d78..e7f372c 100644
--- a/dev/core/src/com/google/gwt/dev/js/ast/JsRootScope.java
+++ b/dev/core/src/com/google/gwt/dev/js/ast/JsRootScope.java
@@ -348,7 +348,7 @@
private final JsName undefined;
- private JsRootScope() {
+ public JsRootScope() {
super("Root");
for (String ident : COMMON_BUILTINS) {
names.put(ident, new JsRootName(this, ident));
diff --git
a/dev/core/test/com/google/gwt/dev/jjs/impl/FragmentExtractorTest.java
b/dev/core/test/com/google/gwt/dev/jjs/impl/FragmentExtractorTest.java
new file mode 100644
index 0000000..feaec09
--- /dev/null
+++ b/dev/core/test/com/google/gwt/dev/jjs/impl/FragmentExtractorTest.java
@@ -0,0 +1,185 @@
+/*
+ * Copyright 2013 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
ANY KIND, either express
+ * or implied. See the License for the specific language governing
permissions and limitations under
+ * the License.
+ */
+package com.google.gwt.dev.jjs.impl;
+
+import com.google.gwt.dev.jjs.ast.JClassType;
+import com.google.gwt.dev.jjs.ast.JConstructor;
+import com.google.gwt.dev.jjs.ast.JDeclaredType;
+import com.google.gwt.dev.jjs.ast.JField;
+import com.google.gwt.dev.jjs.ast.JMethod;
+import com.google.gwt.dev.jjs.impl.FragmentExtractor.LivenessPredicate;
+import com.google.gwt.dev.jjs.impl.FragmentExtractor.NothingAlivePredicate;
+import com.google.gwt.dev.js.ast.JsExprStmt;
+import com.google.gwt.dev.js.ast.JsFunction;
+import com.google.gwt.dev.js.ast.JsInvocation;
+import com.google.gwt.dev.js.ast.JsName;
+import com.google.gwt.dev.js.ast.JsNameRef;
+import com.google.gwt.dev.js.ast.JsProgram;
+import com.google.gwt.dev.js.ast.JsRootScope;
+import com.google.gwt.dev.js.ast.JsStatement;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Unit tests for {@link FragmentExtractor}.
+ */
+public class FragmentExtractorTest extends JJSTestBase {
+
+ private static final JsName DEFINE_SEED_NAME = new
JsName(null, "defineSeed", "defineSeed");
+
+ private static class MockJavaToJavaScriptMap implements
JavaToJavaScriptMap {
+
+ @Override
+ public JsName nameForMethod(JMethod method) {
+ return null;
+ }
+
+ @Override
+ public JsName nameForType(JClassType type) {
+ return null;
+ }
+
+ @Override
+ public JField nameToField(JsName name) {
+ return null;
+ }
+
+ @Override
+ public JMethod nameToMethod(JsName name) {
+ return null;
+ }
+
+ @Override
+ public JClassType nameToType(JsName name) {
+ return null;
+ }
+
+ @Override
+ public JClassType typeForStatement(JsStatement stat) {
+ return null;
+ }
+
+ @Override
+ public JMethod vtableInitToMethod(JsStatement stat) {
+ return null;
+ }
+ }
+
+ private static class MockLivenessPredicate implements LivenessPredicate {
+
+ @Override
+ public boolean isLive(JDeclaredType type) {
+ return false;
+ }
+
+ @Override
+ public boolean isLive(JField field) {
+ return false;
+ }
+
+ @Override
+ public boolean isLive(JMethod method) {
+ return false;
+ }
+
+ @Override
+ public boolean isLive(String literal) {
+ return false;
+ }
+
+ @Override
+ public boolean miscellaneousStatementsAreLive() {
+ return false;
+ }
+ }
+
+ /**
+ * Invokes FragmentExtractor with a fragment description claiming that
Bar was not made live by
+ * the current fragment, but that it has a constructor which *was* made
live. Verifies that the
+ * defineSeed invocation from the global JS block *is* included in the
extracted statement output.
+ */
+ public void testDefineSeed_DeadTypeLiveConstructor() {
+ FragmentExtractor fragmentExtractor;
+ LivenessPredicate constructorLivePredicate;
+
+ // Environment setup.
+ {
+ final JClassType barType = new JClassType(null, "Bar", false, false);
+ final JsName barConstructorName = new JsName(null, "Bar", "Bar");
+ final JConstructor barConstructor = new JConstructor(null, barType);
+ Map<String, JsFunction> functionsByName = new HashMap<String,
JsFunction>();
+ functionsByName.put(
+ "SeedUtil.defineSeed", new JsFunction(null, new JsRootScope(),
DEFINE_SEED_NAME));
+
+ final JsExprStmt defineSeedStatement =
createDefineSeedStatement(barConstructorName);
+
+ JsProgram jsProgram = new JsProgram();
+ jsProgram.setIndexedFunctions(functionsByName);
+ // Defines the entirety of the JS program being split, to be the one
defineSeed statement.
+ jsProgram.getGlobalBlock().getStatements().add(defineSeedStatement);
+
+ JavaToJavaScriptMap map = new MockJavaToJavaScriptMap() {
+ @Override
+ public JMethod nameToMethod(JsName name) {
+ if (name == barConstructorName) {
+ // Finds the Bar constructor by name.
+ return barConstructor;
+ }
+ return null;
+ }
+
+ @Override
+ public JClassType typeForStatement(JsStatement statement) {
+ if (statement == defineSeedStatement) {
+ // Indicates that Bar is the type associated with the
defineSeed statement.
+ return barType;
+ }
+ return null;
+ }
+ };
+ fragmentExtractor = new FragmentExtractor(null, jsProgram, map);
+ constructorLivePredicate = new MockLivenessPredicate() {
+ @Override
+ public boolean isLive(JDeclaredType type) {
+ // Claims that Bar is not made live by the current fragment.
+ return false;
+ }
+
+ @Override
+ public boolean isLive(JMethod method) {
+ // Claims that the bar Constructor *is* made live by the current
fragment.
+ return method == barConstructor;
+ }
+ };
+ }
+
+ List<JsStatement> extractedStatements =
+ fragmentExtractor.extractStatements(constructorLivePredicate, new
NothingAlivePredicate());
+
+ // Asserts that the single defineSeed statement was included in the
extraction output.
+ assertEquals(1, extractedStatements.size());
+ JsStatement defineSeedStatement = extractedStatements.get(0);
+ assertTrue(defineSeedStatement.toString().contains("defineSeed"));
+ }
+
+ private JsExprStmt createDefineSeedStatement(final JsName
barConstructorName) {
+ JsInvocation defineSeedInvocation = new JsInvocation(null);
+ defineSeedInvocation.getArguments().add(new JsNameRef(null,
barConstructorName));
+ defineSeedInvocation.setQualifier(new JsNameRef(null,
DEFINE_SEED_NAME));
+ final JsExprStmt defineSeedStatement = new JsExprStmt(null,
defineSeedInvocation);
+ return defineSeedStatement;
+ }
+}
--
To view, visit https://gwt-review.googlesource.com/3300
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idc87430b94338d289eae4760a1bef8ad127bd699
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Stalcup <[email protected]>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
