[gwt-contrib] Re: RR : Soft permutations (issue160801)
LGTM http://gwt-code-reviews.appspot.com/160801/diff/70001/71009 File dev/core/src/com/google/gwt/dev/CompilePerms.java (right): http://gwt-code-reviews.appspot.com/160801/diff/70001/71009#newcode354 dev/core/src/com/google/gwt/dev/CompilePerms.java:354: PropertyPermutations workList = collapsedPermutations.get(permId); It ended up being not a workList, so the earlier name was better. 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.
[gwt-contrib] Re: RR : Soft permutations (issue160801)
Ready for another round of reviews.
I rebased patch set 6 to r7776.
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 ArtifactSoftPermutation
{
SoftPermutation is no longer a top-level artifact.
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#newcode419
dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java:419:
protected abstract String getModuleSuffix(TreeLogger logger,
LinkerContext context)
I'll re-import the formatter settings before submitting the final thing,
but I don't want to hold this up.
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) {
All hard permutations now implicitly contain a single soft permutation
with no additional properties that require evaluation. This removes a
number of special-case if-statements.
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));
Done.
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.
It does now.
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#newcode115
dev/core/src/com/google/gwt/dev/Permutation.java:115: * together.
Done.
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 defensive, per the existing logic. Moved to a destroy() method.
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#newcode302
dev/core/src/com/google/gwt/dev/Precompile.java:302: private static
class CollapsedPermutationKey extends StringKey {
Moved CollapsedPermutationKey to a top-level type called
CollapsedPropertyKey so the logic can be shared between Precompile and
PropertyPermutations.
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
No longer special-cased.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode600
dev/core/src/com/google/gwt/dev/Precompile.java:600: // Construct a key
from the stringified map of live rebind answers.
Created a new RebindAnswersPermutationKey that handles this.
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
Nothing. Removed special case.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode686
dev/core/src/com/google/gwt/dev/Precompile.java:686:
mergeInto.putRebindAnswer(com.google.gwt.lang.CollapsedPropertyHolder,
Removed code smell.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode696
dev/core/src/com/google/gwt/dev/Precompile.java:696: }
Renumbering added.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode759
dev/core/src/com/google/gwt/dev/Precompile.java:759:
module.getActiveLinkerNames()).size();
Added knowledge of soft-property collapsing to PropertyPermutations that
doesn't require spinning up entire Permutation objects. Verified that a
distributed build with and without sharded generators produces expected
number of hard/soft permutations.
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
Deferring this since it's not critical to the patch.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode272
dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:272:
MapString, SortedSetString map = new LinkedHashMapString,
SortedSetString();
Fixed.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode276
dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:276: //
[gwt-contrib] Re: RR : Soft permutations (issue160801)
LGTM with nits, but Lex should double-check the CompilePerms change.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17015
File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(right):
http://gwt-code-reviews.appspot.com/160801/diff/16001/17015#newcode240
dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:240:
ResolveRebinds.exec(jprogram, permutation);
Actually, it doesn't. :)
http://gwt-code-reviews.appspot.com/160801/diff/70001/71022
File dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java
(right):
http://gwt-code-reviews.appspot.com/160801/diff/70001/71022#newcode197
dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java:197: for
(int i = 0, j = orderedPropertyOracles.length; i j; i++) {
Change this to orderedRebindAnswers.length, and you don't need the
property oracles at all.
http://gwt-code-reviews.appspot.com/160801/diff/70001/71022#newcode236
dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java:236:
JExpression mosteUsedExpression = null;
moste? :)
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.
[gwt-contrib] Re: RR : Soft permutations (issue160801)
Mostly LG, some comments. Also mostly agree with Lex's comments, too.
I called out a couple in particular that are sort of style questions
where I agree.
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 ArtifactSoftPermutation
{
Agreed.
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();
Agreed.
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#newcode419
dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java:419:
protected abstract String getModuleSuffix(TreeLogger logger,
LinkerContext context)
Heh, for me this formats the way it already is.
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#newcode58
dev/core/src/com/google/gwt/dev/Permutation.java:58: private Object
readResolve() throws ObjectStreamException {
Unnecessary throws.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17009#newcode66
dev/core/src/com/google/gwt/dev/Permutation.java:66:
OracleComparator.INSTANCE);
I tend to agree. The merges should happen deterministically, right?
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#newcode600
dev/core/src/com/google/gwt/dev/Precompile.java:600: // Construct a key
from the stringified map of live rebind answers.
Per my comment on 686, this is basically the wrong key. We need a
string that looks like {request1=[result1, result2],...}
http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode686
dev/core/src/com/google/gwt/dev/Precompile.java:686:
mergeInto.putRebindAnswer(com.google.gwt.lang.CollapsedPropertyHolder,
This smells bad to me. I think the problem is that the logic that
merges hard perms together needs to be updated. Basically, that logic
needs something like CollapsedPermutationKey itself, so that the key
generated is not based simply on the canonical property oracle, but
rather is a function of all the soft rebinds. I agree with Lex that the
idea of a canonical property oracle ought to die.
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#newcode272
dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:272:
MapString, SortedSetString map = new LinkedHashMapString,
SortedSetString();
Since you're doing a sort below, this shouldn't need to actually be a
Linked HashMap.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode276
dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:276: // Examine
each orinial value in the set
original
http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode283
dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:283: // If so,
merge the existing set into this one and update pointers
Comment and code in opposite order.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode299
dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:299: return
o1.toString().compareTo(o2.toString());
String s1 = o1.toString();
String s2 = o2.toString();
assert !s1.equals(s2); // asserts no two sets are the same
return s1.compareTo(s2);
http://gwt-code-reviews.appspot.com/160801/diff/16001/17012
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right):
http://gwt-code-reviews.appspot.com/160801/diff/16001/17012#newcode407
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:407: public void
setCollapseAllProperties(boolean collapse) {
I know it's a break with tradition, but this (and really all the other
mutators) ought to be package-protected. Wanna break the old bad trend?
http://gwt-code-reviews.appspot.com/160801/diff/16001/17013
File dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java (right):
http://gwt-code-reviews.appspot.com/160801/diff/16001/17013#newcode1034
dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java:1034: private
static class PropertyValueGlob {
This should sort down with the other new classes.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17015
File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(right):
http://gwt-code-reviews.appspot.com/160801/diff/16001/17015#newcode240
Re: [gwt-contrib] Re: RR : Soft permutations (issue160801)
On Thu, Mar 18, 2010 at 7:38 PM, John Tamplin j...@google.com wrote: On Thu, Mar 18, 2010 at 7:25 PM, sp...@google.com wrote: 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. Wouldn't you have to run generators in precompile if you wanted to collapse equivalent permutations down? There are different kinds of collapsing. I mean the collapsing that this patch adds, which not coincidentally does not depend on generator results. Lex -- 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.
[gwt-contrib] Re: RR : Soft permutations (issue160801)
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 ArtifactSoftPermutation
{
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
Re: [gwt-contrib] Re: RR : Soft permutations (issue160801)
On Thu, Mar 18, 2010 at 7:25 PM, sp...@google.com wrote: 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. Wouldn't you have to run generators in precompile if you wanted to collapse equivalent permutations down? -- John A. Tamplin Software Engineer (GWT), Google -- 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.
