The module changes look great. Most of the implementation looks great, though I saw a few places we might be able to simplify it.
The main issue is that I don't believe that sharded builds will take full advantage of the collapsing. We need for Precompile to emit the number of *collapsed* permutations, but it looks like it emits the number before collapsing. Also, CompilePerms needs to treat its input number as an index into the collapsed permutations. http://gwt-code-reviews.appspot.com/160801/diff/16001/17002 File dev/core/src/com/google/gwt/core/ext/linker/SoftPermutation.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17002#newcode25 dev/core/src/com/google/gwt/core/ext/linker/SoftPermutation.java:25: public abstract class SoftPermutation extends Artifact<SoftPermutation> { Do these need to be top-level artifacts of there own? It is making my head spin. Linkers already grab a list of CompilationResults, and each CompilationResult already has a list of SoftPermutations. What code, then, will go fishing for SoftPermutations directly? http://gwt-code-reviews.appspot.com/160801/diff/16001/17002#newcode34 dev/core/src/com/google/gwt/core/ext/linker/SoftPermutation.java:34: public abstract CompilationResult getCompilationResult(); Likewise, won't linkers already have the CompilationResult handy? Dropping this field would make the object graph more daggy. http://gwt-code-reviews.appspot.com/160801/diff/16001/17002#newcode59 dev/core/src/com/google/gwt/core/ext/linker/SoftPermutation.java:59: if (getCompilationResult() != o.getCompilationResult()) { This should be ==, not !=, shouldn't it? http://gwt-code-reviews.appspot.com/160801/diff/16001/17003 File dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17003#newcode445 dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java:445: if (result.getSoftPermutations().length > 0) { Why the if? If there's one soft perm, can't we just call it soft perm number 0? http://gwt-code-reviews.appspot.com/160801/diff/16001/17003#newcode454 dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java:454: softMap)); Better, I think, to add a field to SelectionInformation than to make strongName have a : in it. The colon can be added by any linker that supports soft permutations. http://gwt-code-reviews.appspot.com/160801/diff/16001/17004 File dev/core/src/com/google/gwt/core/ext/linker/impl/StandardCompilationResult.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17004#newcode121 dev/core/src/com/google/gwt/core/ext/linker/impl/StandardCompilationResult.java:121: * This will ignore empty property maps. Why? A 1-perm, 1-soft-perm, compile will have a single, empty property map.... http://gwt-code-reviews.appspot.com/160801/diff/16001/17009 File dev/core/src/com/google/gwt/dev/Permutation.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17009#newcode64 dev/core/src/com/google/gwt/dev/Permutation.java:64: private final StaticPropertyOracle originalPropertyOracle; This field looks unnecessary, at least in the patch so far. The one use is for ordering permutations, but can't that be done by using the numeric id? http://gwt-code-reviews.appspot.com/160801/diff/16001/17009#newcode66 dev/core/src/com/google/gwt/dev/Permutation.java:66: OracleComparator.INSTANCE); Makes sense. However, it would be simpler to make two lists: one list of property oracles, one list of rebind answers. That should also allow removing OracleComparator, wouldn't it? Comparators are substantial work to write and maintain so long as we have to keep coding them all the way out explicitly. http://gwt-code-reviews.appspot.com/160801/diff/16001/17009#newcode115 dev/core/src/com/google/gwt/dev/Permutation.java:115: * together. "two permutations that either have identical rebind answers or were explicitly collapsed using <collapse-property>" http://gwt-code-reviews.appspot.com/160801/diff/16001/17009#newcode140 dev/core/src/com/google/gwt/dev/Permutation.java:140: other.rebindAnswers.clear(); It's odd to destroy the old permutation from here. I assume this is to be defensive? If so, perhaps have the caller of Permutation.mergeFrom do Permutation.destroy, for a new method destroy(), on the one that is being eliminated? http://gwt-code-reviews.appspot.com/160801/diff/16001/17010 File dev/core/src/com/google/gwt/dev/Precompile.java (left): http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#oldcode521 dev/core/src/com/google/gwt/dev/Precompile.java:521: // Sort the permutations by an ordered key to ensure determinism. Isn't it simpler to sort by id? Perhaps this code predates permutations knowing their id? http://gwt-code-reviews.appspot.com/160801/diff/16001/17010 File dev/core/src/com/google/gwt/dev/Precompile.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode339 dev/core/src/com/google/gwt/dev/Precompile.java:339: return collapsedPropertyMap.isEmpty() ? null Why special case an empty map? http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode662 dev/core/src/com/google/gwt/dev/Precompile.java:662: // No collapsed properties in that permutation If this was dropped, and the equivalence set ended up with one value, what would go wrong? http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode696 dev/core/src/com/google/gwt/dev/Precompile.java:696: } This function should probably also renumber the merged permutations to go from 1..n. http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode759 dev/core/src/com/google/gwt/dev/Precompile.java:759: module.getActiveLinkerNames()).size(); For sharded builds to work, this numPermutations needs to reflect the number of *collapsed* permutations. It might be easier to refactor things so that PropertyPermutations also does collapsing. There is related code in CompilePerms that takes a permutation number. That number needs to refer to a *collapsed* permutation. http://gwt-code-reviews.appspot.com/160801/diff/16001/17011 File dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode271 dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:271: // Maps a value to the set that contains that value This is a union-find operation. It would be faster and about the same amount of code to use the fast union-find algorithm. http://www.cs.princeton.edu/courses/archive/spr07/cos226/lectures/01union-find.pdf http://gwt-code-reviews.appspot.com/160801/diff/16001/17014 File dev/core/src/com/google/gwt/dev/cfg/StaticPropertyOracle.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17014#newcode163 dev/core/src/com/google/gwt/dev/cfg/StaticPropertyOracle.java:163: * Dumps the binding property key/value pairs; For debugging use only. Very handy. http://gwt-code-reviews.appspot.com/160801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe from this group, send email to google-web-toolkit-contributors+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
