Allows enum ordinalization to proceed for enums with static methods/fields -- This offers modest code-size gains, and will open way for further optimizations -- Also, fixes a bug in checking for instanceof arguments for EnumOrdinalizer -- Fixed some formatting in EnumOrdinalizer -- Added new tests to EnumOrdinalizerTest Review at http://gwt-code-reviews.appspot.com/1428808 git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@10135 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JEnumType.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JEnumType.java index d44f0cc..77e01cb 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/ast/JEnumType.java +++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JEnumType.java
@@ -29,6 +29,7 @@ */ private List<JEnumField> enumList = Lists.create(); + private boolean isOrdinalized = false; public JEnumType(SourceInfo info, String name, boolean isAbstract) { super(info, name, isAbstract, false); @@ -63,4 +64,12 @@ public JEnumType isEnumOrSubclass() { return this; } + + public boolean isOrdinalized() { + return isOrdinalized; + } + + public void setOrdinalized() { + isOrdinalized = true; + } }
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java index 0ad3931..c0e5a90 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java +++ b/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
@@ -71,9 +71,13 @@ * This optimizer modifies enum classes to change their field constants to ints, * and to remove initialization of those constants in the clinit method. An * ordinalized enum class will not be removed from the AST by this optimizer, - * but as long as all references to it are replaced (which is one of the - * requirements for ordinalization), then the enum class itself will be pruned - * by subsequent optimizer passes. + * but as long as all references to it are replaced, then the enum class itself + * will be pruned by subsequent optimizer passes. Some enum classes may not be + * completely removed however. Ordinalization can proceed in cases where there + * are added static fields or methods in the enum class. In such cases a reduced + * version of the original enum class can remain in the AST, containing only + * static fields and methods which aren't part of the enum infrastructure (in + * which case it will no longer behave as an enum class at all). * * Regardless of whether an ordinalized enum class ends up being completely * pruned away, the AST is expected to be in a coherent and usable state after @@ -295,13 +299,29 @@ * does this by keeping track of a "black-list" for ordinals which violate the * conditions for ordinalization, below. * - * An enum cannot be ordinalized, if it: is implicitly upcast. is implicitly - * cast to from a nullType. is implicitly cast to or from a javaScriptObject - * type. is explicitly cast to another type (or vice-versa). it's class - * literal is used explicitly. it has an artificial rescue recorded for it. - * has any field referenced, except for: one of it's enum constants - * Enum.ordinal has any method called, except for: ordinal() Enum.ordinal() - * Enum() super constructor Enum.createValueOfMap() + * An enum cannot be ordinalized, if it: + * <ul> + * <li>is implicitly upcast.</li> + * <li>is implicitly cast to from a nullType.</li> + * <li>is implicitly cast to or from a javaScriptObject type.</li> + * <li>is explicitly cast to another type (or vice-versa).</li> + * <li>is tested in an instanceof expression.</li> + * <li>it's class literal is used explicitly.</li> + * <li>it has an artificial rescue recorded for it.</li> + * <li>has any field referenced, except for:</li> + * <ul> + * <li>static fields, other than the synthetic $VALUES field.</li> + * <li>Enum.ordinal.</li> + * </ul> + * <li>has any method called, except for:</li> + * <ul> + * <li>ordinal().</li> + * <li>Enum.ordinal().</li> + * <li>Enum() super constructor.</li> + * <li>Enum.createValueOfMap().</li> + * <li>static methods, other than values() or valueOf().</li> + * </ul> + * </ul> * * This visitor extends the ImplicitUpcastAnalyzer, which encapsulates all the * conditions where implicit upcasting can occur in an AST. The rest of the @@ -394,6 +414,11 @@ JEnumType maybeEnum = x.isEnumOrSubclass(); if (maybeEnum != null) { enumsVisited.put(program.getClassLiteralName(maybeEnum), maybeEnum); + + // don't need to re-ordinalize a previously ordinalized enum + if (maybeEnum.isOrdinalized()) { + addToBlackList(maybeEnum, x.getSourceInfo()); + } } } @@ -408,44 +433,28 @@ // check any instance field reference other than ordinal blackListIfEnumExpression(x.getInstance()); } else if (x.getField().isStatic()) { - // check static field references - /* - * Need to exempt static fieldRefs to the special $VALUES array that - * gets generated for all enum classes, if the reference occurs within - * the enum class itself (such as happens in the clinit() or values() - * method for all enums). + * Black list if the $VALUES static field is referenced outside of an + * enum class. This can happen if there's a call to an enum's values() + * method, which then gets inlined. + * + * TODO (jbrosenberg): Investigate further whether referencing the + * $VALUES array (as well as the values() method) should not block + * ordinalization. Instead, convert $VALUES to an array of int. */ if (x.getField().getName().equals("$VALUES") - && this.currentMethod.getEnclosingType() == x.getField().getEnclosingType()) { - if (getEnumType(x.getField().getEnclosingType()) != null) { - return; - } + && this.currentMethod.getEnclosingType() != x.getField().getEnclosingType()) { + blackListIfEnum(x.getField().getEnclosingType(), x.getSourceInfo()); } - - /* - * Need to exempt static fieldRefs for enum constants themselves. Detect - * these as final fields, that have the same enum type as their - * enclosing type. - */ - if (x.getField().isFinal() - && (x.getField().getEnclosingType() == getEnumType(x.getField().getType()))) { - return; - } - - /* - * Check any other refs to static fields of an enum class. This includes - * references to $VALUES that might occur outside of the enum class - * itself. This can occur when a call to the values() method gets - * inlined, etc. Also check here for any user defined static fields. - */ - blackListIfEnum(x.getField().getEnclosingType(), x.getSourceInfo()); } } @Override public void endVisit(JInstanceOf x, Context ctx) { // If any instanceof tests haven't been optimized out, black list. + blackListIfEnum(x.getExpr().getType(), x.getSourceInfo()); + // TODO (jbrosenberg): Investigate further whether ordinalization can be + // allowed in this case. blackListIfEnum(x.getTestType(), x.getSourceInfo()); } @@ -461,11 +470,10 @@ if (x.getInstance() != null) { blackListIfEnumExpression(x.getInstance()); } else if (x.getTarget().isStatic()) { - /* - * need to exempt static methodCalls for an enum class if it occurs - * within the enum class itself (such as in $clinit() or values()) - */ - if (this.currentMethod.getEnclosingType() != x.getTarget().getEnclosingType()) { + // black-list static method calls on an enum class only for valueOf() + // and values() + String methodName = x.getTarget().getName(); + if (methodName.equals("valueOf") || methodName.equals("values")) { blackListIfEnum(x.getTarget().getEnclosingType(), x.getSourceInfo()); } } @@ -888,17 +896,14 @@ ordinalAnalyzer.accept(program); ordinalAnalyzer.afterVisitor(); - if (tracker != null) { - for (JEnumType type : enumsVisited.values()) { - tracker.addVisited(type.getName()); - if (!ordinalizationBlackList.contains(type)) { - tracker.addOrdinalized(type.getName()); - } - } - } - // Bail if we don't need to do any ordinalization if (enumsVisited.size() == ordinalizationBlackList.size()) { + // Update tracker stats + if (tracker != null) { + for (JEnumType type : enumsVisited.values()) { + tracker.addVisited(type.getName()); + } + } return stats; } @@ -920,6 +925,19 @@ if (tracker != null) { tracker.maybeDumpAST(program, 2); } + + // Update enums ordinalized, and tracker stats + for (JEnumType type : enumsVisited.values()) { + if (tracker != null) { + tracker.addVisited(type.getName()); + } + if (!ordinalizationBlackList.contains(type)) { + if (tracker != null) { + tracker.addOrdinalized(type.getName()); + } + type.setOrdinalized(); + } + } return stats; }
diff --git a/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java b/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java index cf265c8..ad305cc 100644 --- a/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java +++ b/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java
@@ -72,10 +72,11 @@ code.append("package com.google.gwt.lang;\n"); code.append("public final class Cast {\n"); code.append(" public static Object dynamicCast(Object src, int dstId) { return src;}\n"); - code.append(" public static boolean isNull(Object a) { return false;}\n"); - code.append(" public static boolean isNotNull(Object a) { return false;}\n"); - code.append(" public static boolean jsEquals(Object a, Object b) { return false;}\n"); - code.append(" public static boolean jsNotEquals(Object a, Object b) { return false;}\n"); + code.append(" public static boolean instanceOf(Object src, int dstId) { return false;}\n"); + code.append(" public static native boolean isNull(Object a) /*-{ }-*/;\n"); + code.append(" public static native boolean isNotNull(Object a) /*-{ }-*/;\n"); + code.append(" public static native boolean jsEquals(Object a, Object b) /*-{ }-*/;\n"); + code.append(" public static native boolean jsNotEquals(Object a, Object b) /*-{ }-*/;\n"); code.append("}\n"); return code; }
diff --git a/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java b/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java index 6cb1ab9..34f0a84 100644 --- a/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java +++ b/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
@@ -43,12 +43,11 @@ super.setUp(); EnumOrdinalizer.enableTracker(); - // don't need to runDeadCodeElimination, it's after we're done anyway - runDeadCodeElimination = false; - // defaults, can be overridden by individual test cases runTypeTightener = false; runMethodCallTightener = false; + runMethodInliner = true; + runMakeCallsStatic = true; } public void testOrdinalizeBasicAssignment() @@ -240,6 +239,58 @@ assertTrue(tracker.isOrdinalized("test.EntryPoint$Fruit")); } + public void testOrdinalizableStaticFieldRef() + throws UnableToCompleteException { + EnumOrdinalizer.resetTracker(); + + // this will cause a static field ref in the enum clinit + setupFruitEnumWithStaticField(); + optimize("void", "String y = Fruit.staticField;"); + + EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker(); + assertTrue(tracker.isOrdinalized("test.EntryPoint$Fruit")); + } + + public void testOrdinalizableStaticMethod() + throws UnableToCompleteException { + EnumOrdinalizer.resetTracker(); + + // this will cause a static method enum class + setupFruitEnumWithStaticMethod(); + optimize("void", "int y = Fruit.staticMethod();"); + + EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker(); + assertTrue(tracker.isOrdinalized("test.EntryPoint$Fruit")); + } + + public void testNotOrdinalizableInstanceStaticFieldRef() + throws UnableToCompleteException { + EnumOrdinalizer.resetTracker(); + + // this will cause a static field ref in the enum clinit + setupFruitEnumWithStaticField(); + optimize("void", "Fruit fruit = Fruit.APPLE;", + "String y = fruit.staticField;"); + + EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker(); + assertTrue(tracker.isVisited("test.EntryPoint$Fruit")); + assertFalse(tracker.isOrdinalized("test.EntryPoint$Fruit")); + } + + public void testNotOrdinalizableInstanceStaticMethod() + throws UnableToCompleteException { + EnumOrdinalizer.resetTracker(); + + // this will cause a static method enum class + setupFruitEnumWithStaticMethod(); + optimize("void", "Fruit fruit = Fruit.APPLE;", + "int y = fruit.staticMethod();"); + + EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker(); + assertTrue(tracker.isVisited("test.EntryPoint$Fruit")); + assertFalse(tracker.isOrdinalized("test.EntryPoint$Fruit")); + } + public void testNotOrdinalizableClassLiteralReference() throws UnableToCompleteException { EnumOrdinalizer.resetTracker(); @@ -383,20 +434,40 @@ assertFalse(tracker.isOrdinalized("test.EntryPoint$Fruit")); } - public void testNotOrdinalizableInstanceOf() { - /* - * TODO(jbrosenberg): determine if this really needs to be true, and write a - * test to prove it. - */ + public void testNotOrdinalizableInstanceOfEnumExpression() + throws UnableToCompleteException { + setupFruitEnum(); + optimize("void", "Fruit fruit = Fruit.APPLE;", + "if (fruit instanceof Enum) {", + " fruit = Fruit.ORANGE;", + "}"); + + EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker(); + assertTrue(tracker.isVisited("test.EntryPoint$Fruit")); + assertFalse(tracker.isOrdinalized("test.EntryPoint$Fruit")); } - public void testNotOrdinalizableStaticFieldRef() + public void testNotOrdinalizableInstanceOfEnumTestType() + throws UnableToCompleteException { + setupFruitEnum(); + optimize("void", "Object fruitObj = new Object();", + "if (fruitObj instanceof Fruit) {", + " fruitObj = null;", + "}"); + + EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker(); + assertTrue(tracker.isVisited("test.EntryPoint$Fruit")); + assertFalse(tracker.isOrdinalized("test.EntryPoint$Fruit")); + } + + public void testNotOrdinalizableStaticFieldRefToVALUES() throws UnableToCompleteException { EnumOrdinalizer.resetTracker(); - // this will cause a static field ref in the enum clinit - setupFruitEnumWithStaticField(); - optimize("void"); + // this ends up inlining the values() method call, and thus $VALUES is referenced external + // to the Fruit enum class. + setupFruitEnum(); + optimize("void", "Fruit[] fruits = Fruit.values();"); EnumOrdinalizer.Tracker tracker = EnumOrdinalizer.getTracker(); assertTrue(tracker.isVisited("test.EntryPoint$Fruit")); @@ -407,7 +478,9 @@ throws UnableToCompleteException { EnumOrdinalizer.resetTracker(); - // this ends up calling Fruit.clinit() first (which is a static method call) + // make sure values() method call doesn't doesn't get inlined + runMethodInliner = false; + setupFruitEnum(); optimize("void", "Fruit[] fruits = Fruit.values();"); @@ -824,7 +897,6 @@ private void setupFruitEnum() { addSnippetClassDecl("public enum Fruit {APPLE, ORANGE}"); - setupExtraDummyEnum(); } private void setupFruitEnumWithInstanceField() { @@ -834,31 +906,27 @@ " instanceField = str;", " }", "}"); - setupExtraDummyEnum(); } private void setupFruitEnumWithStaticField() { addSnippetClassDecl("public enum Fruit {APPLE, ORANGE;", " public static final String staticField = \"STATIC\";", "}"); - setupExtraDummyEnum(); + } + + private void setupFruitEnumWithStaticMethod() { + addSnippetClassDecl("public enum Fruit {APPLE, ORANGE;", + " public static final int staticMethod() {", + " int x = 0;", + " return x;", + " }", + "}"); } private void setupVegetableEnum() { addSnippetClassDecl("public enum Vegetable {CARROT, SPINACH}"); } - private void setupExtraDummyEnum() { - /* - * Assure there are at least more 2 enums in the program, so inlining or - * tightening doesn't push a single enum sub-class into the methods of the - * Enum super-class itself (which prevents ordinalization in most cases). - * TODO(jbrosenberg): Make ordinalization work if there's only 1 enum in - * a program. - */ - addSnippetClassDecl("public enum DummyEnum {DUMMY}"); - } - private void setupFruitSwitchMethod() { addSnippetClassDecl("public static String fruitSwitch(Fruit fruit) {", " switch(fruit) {", @@ -878,19 +946,11 @@ private final boolean runCastNormalizer = true; private final boolean runEqualityNormalizer = true; - /* - * EnumOrdinalizer depends MakeCallsStatic and MethodInliner running before - * it runs, since they cleanup the internal structure of an enum class to - * inline instance methods like $init. - * TODO(jbrosenberg): Update EnumOrdinalizer to be able to succeed - * irrespective of the ordering and interaction with other optimizers. - */ - private final boolean runMakeCallsStatic = true; - private final boolean runMethodInliner = true; - - // these can be enabled where needed - private boolean runMethodCallTightener = false; - private boolean runTypeTightener = false; + // These are enabled as needed for a given test + private boolean runMakeCallsStatic; + private boolean runMethodInliner; + private boolean runMethodCallTightener; + private boolean runTypeTightener; @Override protected boolean optimizeMethod(JProgram program, JMethod method) {