Nice idea. I'm not sure how much it will matter once we change over clinits to be eager, but it certainly helps right now.
The one thing that needs looking at in this patch is the computation of the target clinit to get it to the best result on one iteration of recomputeAfterOptimization(). There are also some cosmetic things. Broadly, I am struck by how many different pieces of code have to be careful about clinits and had to be changed with this patch. It would be nice to put the clinit calls into the tree explicitly, so that not so many parts of the system have to check for an implicit clinit call. http://gwt-code-reviews.appspot.com/184802/diff/1/2 File dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java (right): http://gwt-code-reviews.appspot.com/184802/diff/1/2#newcode58 dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java:58: private JDeclaredType clinitTarget = this; Nice idea. http://gwt-code-reviews.appspot.com/184802/diff/1/2#newcode130 dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java:130: * generalizes for say siblings targeting the same superclass.) It's easier to read if it's written directly: "The clinit for the source of the reference must already have run, so if it's the same as this one, there it must have already run. One example is a reference from a subclass to something in a superclass." http://gwt-code-reviews.appspot.com/184802/diff/1/2#newcode275 dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java:275: */ Intentional? http://gwt-code-reviews.appspot.com/184802/diff/1/2#newcode294 dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java:294: void setClinitTo(JDeclaredType newClinitTarget) { The Java idiom is "setClinit". http://gwt-code-reviews.appspot.com/184802/diff/1/2#newcode299 dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java:299: removeClinit(); If the target is another class, shouldn't the clinit of this one also be removed? Pruner is a really heavyweight optimization, so in general it's not good to wait for it. http://gwt-code-reviews.appspot.com/184802/diff/1/3 File dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java (right): http://gwt-code-reviews.appspot.com/184802/diff/1/3#newcode654 dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java:654: type.setClinitTo(target); Can't the target have another target, which has another one? If so, then calling recomputeAfterOptimizations() twice could mean that the clinit targets get improved more than just calling it once. Perhaps make a map mapping each type to the result of computeHasClinitTargetRecursive, and then update the map until every non-null entry points to an entry that points to itself? Then call all the setClinitTo's at the end. http://gwt-code-reviews.appspot.com/184802/diff/1/3#newcode659 dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java:659: private JDeclaredType computeHasClinitTargetRecursive(JDeclaredType type, I guess it's computing the target, not whether it "has" a target. http://gwt-code-reviews.appspot.com/184802/diff/1/4 File dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java (right): http://gwt-code-reviews.appspot.com/184802/diff/1/4#newcode421 dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java:421: targetType.getClinitTarget())); The getClinitTarget here is not needed, is it? createClinitCall finds the right one. http://gwt-code-reviews.appspot.com/184802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe, reply using "remove me" as the subject.
