[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[jbrosenberg]

Since this has been submitted, can you close it?

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[jbrosenberg]

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java#newcode516
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java:516: }
else {
maybe a little cleaner to break the control flow into:

else if (currentClass == program.getIndexedType(Array)) {
  ...
} else {
  ...
}

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java#newcode521
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java:521: }
else {
comment explaining why?

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2161
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2161: //
don't implement, fall through to Object.getClass()
why do arrays need to be different (add to comment?)

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2763
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2763:
private static final long AST_VERSION = 2;
do we need to bump this version, since array classes will now be
serialized differently?

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java#newcode516
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java:516: }
else {
On 2011/07/08 11:24:25, jbrosenberg wrote:

maybe a little cleaner to break the control flow into:



else if (currentClass == program.getIndexedType(Array)) {
   ...
} else {
   ...
}


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java#newcode521
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java:521: }
else {
On 2011/07/08 11:24:25, jbrosenberg wrote:

comment explaining why?


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2161
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2161: //
don't implement, fall through to Object.getClass()
On 2011/07/08 11:24:25, jbrosenberg wrote:

why do arrays need to be different (add to comment?)


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/6053/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2763
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2763:
private static final long AST_VERSION = 2;
On 2011/07/08 11:24:25, jbrosenberg wrote:

do we need to bump this version, since array classes will now be

serialized

differently?


Done.

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[jbrosenberg]

LGTM


http://gwt-code-reviews.appspot.com/1447821/diff/20001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/20001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java#newcode516
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java:516: }
else {
Heh, I think I was thinking the else above could be changed to an else
if, to save a level of parens (no biggy, since this class is likely to
go away altogether once we are sure the new GwtAstBuilder will stick,
which is looking very likely at present)

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[jbrosenberg]

LGTM (w/minor cleanup + a suggestion)
I like the management of class literal rescues much better now!


http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode93
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:93:
whitespace

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode600
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:600:
instantiatedTypes.add(type);
Suggest combining all this into 1 method call to something like
(maybeRescueClassLiteral(type)), and then mRCL contains logic to see
if classLiteralsToBeRescuedIfGetClassLive is null or not, to decide
whether to rescue or mark for rescue.

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode763
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:763:
suggest (see above) changing this to maybeRescueClassLiteral(type), and
have it check whether classLiteralsToBeRescuedIfGetClassLive is null or
not.  This way, the logic to decide whether to rescue immediately or
deferred is separated from the logic for whether getClass is live, etc.

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
is re-entrant the right term (I usually think of it wrt to concurrency)?

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode870
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:870:
whitespace

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode917
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:917:
/**
C

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

I made the fixes and submitted it. Since most people are headed out of
the office for the holiday, I need to get it in early to see if I need
to revert it before everyone is gone. :)



http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode93
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:93:
On 2011/07/01 10:24:38, jbrosenberg wrote:

whitespace


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode600
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:600:
instantiatedTypes.add(type);
On 2011/07/01 10:24:38, jbrosenberg wrote:

Suggest combining all this into 1 method call to something like
(maybeRescueClassLiteral(type)), and then mRCL contains logic to see

if

classLiteralsToBeRescuedIfGetClassLive is null or not, to decide

whether to

rescue or mark for rescue.


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
Not sure, but rescueClassLiteral can end up invoking this function
again, and you'll get a concurrent modification exception if you don't
clone, and if you do, you end up with extra work.

On 2011/07/01 10:24:38, jbrosenberg wrote:

is re-entrant the right term (I usually think of it wrt to

concurrency)?

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode870
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:870:
On 2011/07/01 10:24:38, jbrosenberg wrote:

whitespace


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode917
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:917:
/**
On 2011/07/01 10:24:38, jbrosenberg wrote:

C


IDE has bizarre bug where it was randomly inserting these.

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[Ray Cromwell]

Oops, there's a problem, I need one more cycle before I can commit.
Will send patch tonite. :(


On Fri, Jul 1, 2011 at 1:43 PM,  cromwell...@google.com wrote:

 I made the fixes and submitted it. Since most people are headed out of
 the office for the holiday, I need to get it in early to see if I need
 to revert it before everyone is gone. :)



 http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
 File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
 (right):

 http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode93
 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:93:
 On 2011/07/01 10:24:38, jbrosenberg wrote:

 whitespace

 Done.

 http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode600
 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:600:
 instantiatedTypes.add(type);
 On 2011/07/01 10:24:38, jbrosenberg wrote:

 Suggest combining all this into 1 method call to something like
 (maybeRescueClassLiteral(type)), and then mRCL contains logic to see

 if

 classLiteralsToBeRescuedIfGetClassLive is null or not, to decide

 whether to

 rescue or mark for rescue.

 Done.

 http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
 if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
 Not sure, but rescueClassLiteral can end up invoking this function
 again, and you'll get a concurrent modification exception if you don't
 clone, and if you do, you end up with extra work.

 On 2011/07/01 10:24:38, jbrosenberg wrote:

 is re-entrant the right term (I usually think of it wrt to

 concurrency)?

 http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode870
 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:870:
 On 2011/07/01 10:24:38, jbrosenberg wrote:

 whitespace

 Done.

 http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode917
 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:917:
 /**
 On 2011/07/01 10:24:38, jbrosenberg wrote:

 C

 IDE has bizarre bug where it was randomly inserting these.

 http://gwt-code-reviews.appspot.com/1447821/


-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[jbrosenberg]

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
Ah, so you're saying it could re-enter recursively...

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[jbrosenberg]

Almost ready to signoff, remaining concern about performance hit in CFA
(see comment there).


http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode996
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:996:
(Just copied over the same comments I had earlier from patch set 2)

This seems a bit expensive (iterating through all instantiated types).
It looks
like it could be called many times per optimization pass?  For instance,
traverseFrom() is called multiple times in a loop, in
traverseEntryMethods().
Is there any impact on execution time, for large projects?   It seems
like it
should only be called once per CFA pass, no?   Also, traverseFrom is
called in
many places external to this class, in tight loops iterating over
multiple
methods, etc.

http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1817
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1817:
for (JMethod method : x.getMethods()) {
Can you add a comment of explanation here, as discussed in patch set 2?

http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1822
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1822:
if (JProgram.isClinit(method)) {
Add an assert (as discussed in comment in patch set 2)?

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode996
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:996:

It can be run at the end of a CFA pass, but it must be run before any
attempt to use the data from a CFA pass, this is somewhat difficult
since CFA is used in many places and there's no sort of post-CFA call
that's expected to be made to commit CFA results before you use them,
which is where I'd like to hook in.

I'll try moving it into CFA's visitor to do it incrementally.

On 2011/06/30 16:07:13, jbrosenberg wrote:

(Just copied over the same comments I had earlier from patch set 2)



This seems a bit expensive (iterating through all instantiated types).

 It looks

like it could be called many times per optimization pass?  For

instance,

traverseFrom() is called multiple times in a loop, in

traverseEntryMethods().

Is there any impact on execution time, for large projects?   It seems

like it

should only be called once per CFA pass, no?   Also, traverseFrom is

called in

many places external to this class, in tight loops iterating over

multiple

methods, etc.


http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1817
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1817:
for (JMethod method : x.getMethods()) {
On 2011/06/30 16:07:13, jbrosenberg wrote:

Can you add a comment of explanation here, as discussed in patch set

2?

I added a comment. Basically, the compiler will insert the following,
even on empty classes:

public void init$() {}
public A() { super(); init$(); }

http://gwt-code-reviews.appspot.com/1447821/diff/9006/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1822
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1822:
if (JProgram.isClinit(method)) {
On 2011/06/30 16:07:13, jbrosenberg wrote:

Add an assert (as discussed in comment in patch set 2)?


Also can't assert emptyness. I added a comment. Any static fields
initialized by JSO Array or Object will yield a clinit() with code in
it. Also, if B extend A, then B's clinit will be non-empty and include
code that invokes A's clinit (which could be empty).

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode312
dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:312:
// prune all Object.getClass() overrides and replace with inline field
ref
On 2011/06/10 14:54:40, jbrosenberg wrote:

It's a shame this can't be called earlier on, so that optimizers can

take

advantage of the loss of polymorphism with getClass()...


The Replace pass essentially does what the optimizers would do anyway,
which is inline the reference, but yeah, it would be nice.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode937
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:937: }
On 2011/06/10 14:54:40, jbrosenberg wrote:

whitespace


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode246
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:246:
assert jsprogram.getFragmentCount() == 1;
On 2011/06/13 19:35:25, jbrosenberg wrote:

extra indentation, here and several places below in this method


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode261
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:261:
stat = result;
On 2011/06/13 19:35:25, jbrosenberg wrote:

this assignment is redundant, we have the same assignment below under

the if

(keepIt) block below.


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode345
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:345: */
On 2011/06/13 19:35:25, jbrosenberg wrote:

It looks like all this does is maybe remove some ctors (and if any

ctors remain,

set a flag).  Would a better name be

maybeRemoveCtorsFromDefineSeedStmt()?

Done.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode357
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:357:
JConstructor ctor = (JConstructor) maybeCtor;
On 2011/06/13 19:35:25, jbrosenberg wrote:

how can ctor be null here?


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode504
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:504:
JsNameRef lhs = (JsNameRef) binExpr.getArg1();
Not sure actually, I think this change came originally from scott a long
time ago who had merged something into my patch.

On 2011/06/13 19:35:25, jbrosenberg wrote:

Is there a reason for the change in the way of testing that we are

using the _

tempVar.  E.g. why no longer do:



if (!lhs.getName().getShortIdent().equals(_))



Is it just for brevity?  Or is there a requirement that the underBar

be existing

at this point (and is this a new requirement?).


http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(left):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#oldcode1255
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1255:

On 2011/06/13 19:35:25, jbrosenberg wrote:

why get rid of this comment?


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode610
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:610:

On 2011/06/13 19:35:25, jbrosenberg wrote:

white space


Done.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode724
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:724:

Yes, because immortal types are hoisted to the top, and have no clinits
called. If not for the early exit, the rest of the method would add a
clinit call for to setup fields what have JSO 

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[jbrosenberg]

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode246
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:246:
assert jsprogram.getFragmentCount() == 1;
extra indentation, here and several places below in this method

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode261
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:261:
stat = result;
this assignment is redundant, we have the same assignment below under
the if (keepIt) block below.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode345
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:345: */
It looks like all this does is maybe remove some ctors (and if any ctors
remain, set a flag).  Would a better name be
maybeRemoveCtorsFromDefineSeedStmt()?

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode357
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:357:
JConstructor ctor = (JConstructor) maybeCtor;
how can ctor be null here?

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode504
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:504:
JsNameRef lhs = (JsNameRef) binExpr.getArg1();
Is there a reason for the change in the way of testing that we are using
the _ tempVar.  E.g. why no longer do:

if (!lhs.getName().getShortIdent().equals(_))

Is it just for brevity?  Or is there a requirement that the underBar be
existing at this point (and is this a new requirement?).

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(left):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#oldcode1255
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1255:

why get rid of this comment?

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode572
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:572:
}
Makes sense, but take note that this is only currently enabled for
PRETTY or DETAILED output modes.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode610
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:610:

white space

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode724
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:724:

Are these checks needed here, since the corresponding
visit(JClassType...) method returns false if x is the class literal
holder, or is an immortal code gen type.

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1438
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1438:
if (program.immortalCodeGenTypes.contains(x)) {
s/Handle/Handled

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1665
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1665:
}
Suggest change to return a new empty JsLiteralObject here, if castMap is
null (since this is what all callers must do anyway).

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1816
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1816:
for (JMethod method : x.getMethods()) {
Should this maybe be an assertion, that no immortal types should be
allowed to have virtual methods or constructors?  Otherwise it might be
confusing if they are routinely thrown out?  Should there also be a
comment, similar to the one in JProgram where immortal gen types are
defined, stating that immortal types should not have any non-static
methods or fields?

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1821
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1821:
if (JProgram.isClinit(method)) {

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[jbrosenberg]

I'm about a 3rd of the way through this, but here's some initial
comments.  I'll be able to spend more time on this evening


http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode312
dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:312:
// prune all Object.getClass() overrides and replace with inline field
ref
It's a shame this can't be called earlier on, so that optimizers can
take advantage of the loss of polymorphism with getClass()...

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode937
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:937: }
whitespace

http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode995
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:995:
This seems a bit expensive (iterating through all instantiated types).
It looks like it could be called many times per optimization pass?  For
instance, traverseFrom() is called multiple times in a loop, in
traverseEntryMethods().  Is there any impact on execution time, for
large projects?   It seems like it should only be called once per CFA
pass, no?   Also, traverseFrom is called in many places external to this
class, in tight loops iterating over multiple methods, etc.

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[Ray Cromwell]

BTW Scott, I know you might have liked this as three separate CLs,
unfortunately, I did the refactoring work by iterating continuously on
the three separate items (seed funcs, class literals, and injection of
code), and there were interdependencies (when I changed seed func
code, I had to update the class literal optimization and vice versa)

Also, in the past we have discussed using prototype copying instead of
chaining. It's something to look at in the future once we have a
benchmarking test for it, but I think it's elegant to use chaining or
now because it makes the deferred class literal installation easier.

-Ray


On Wed, Jun 8, 2011 at 4:05 PM,  cromwell...@google.com wrote:
 Reviewers: scottb, jbrosenberg,

 Description:
 This patch substantially reduces the overhead of Java types in the
 output by minimizing vtable setup and virtual class literal fetches.
 Improvements are anywhere from 5% to 10% uncompressed obfuscated JS.

 The patch includes the following changes:

 1) A new Immortal CodeGenType. Immortal CodeGenTypes are CodeGenTypes
 that cannot even be pruned by the final pruner pass. They exist in order
 to conveniently define code in Java (as opposed to encoding JS in Java
 strings) which must be injected into the Javascript AST during
 construction. They must consist of only static methods and static
 fields, and static field initializers are only permitted to be literals
 and JSO.createObject()/createArray(). In addition, they are guaranteed
 to be hoisted prior to the first non-Immortal type statement.

 2) A new SeedUtil Immortal type which provides utility functions for
 setting up vtables. It eliminates empty constructor seed functions by
 using 2 helper functions to setup anonymous closure versions. Something
 like this before the patch:

 function fooSeed() {}
 _ = fooSeed.prototype = FooConstructor.prototype = new superType();
 _.castableTypeMap = { ... }
 _.getClass = function() {
 return classLiteral;
 }

 is replaced with

 defineSeed(seedId, superSeedid, { castableTypeMap }, FooConstructor1,
 FooConstructor2, ...)

 This has two effects. First, it reduces both compressed and uncompressed
 codesize by a non-trivial amount by eliminating empty globally named
 seed functions which are only used as prototype placeholders. Secondly,
 it frees up extra obfuscated identifiers (by using numeric ids to
 identifer function seeds) in the global scope. Note: the seedId is not
 necessarily the queryId. Third, prototypes can be
 looked up by seedId.

 3) Eliminate of All getClass() overrides. Two designs were tried, one
 which removes the overrides during AST generation and the other which
 leaves them in, but removes them later in the compilation. The latter
 turned out to be simpler and produce some side benefits. This works as
 follows:

 First, java.lang.Object is given a new Class? clazz field and
 Object.getClass() returns it.

 Second, the ClassLiteralHolder fields, which are evaluated last, install
 the appropriate Class instance into this field on the appropriate
 prototype. That is, the Class.createForClass() method for example,
 creates a new ClassLiteral, looks up the appropriate prototype, and
 installs it into the Object.clazz field for that type.

 Third, a final pass, just prior to generating the Javascript AST, prunes
 all getClass() overrides. The overrides are left in during the
 GenerateJavaAST phase, because it is advantageous to allow some of them
 to be inlined when
 method call tightening allows it, and because it minimizes the amount of
 changes to ControlFlowAnalyzer. CFA did had
 to be changed to rescue ClassLiterals for any instantiated type, if the
 getClass() method is live.

 deRPC had to be modified to deal with the new scheme of looking up the
 prototype/seed function by number instead of name, as well as the new
 naming scheme when -XdisableClassMetadata is active.


 Please review this at http://gwt-code-reviews.appspot.com/1447821/

 Affected files:
  M dev/core/src/com/google/gwt/core/ext/linker/SymbolData.java
  M dev/core/src/com/google/gwt/core/ext/linker/impl/StandardSymbolData.java
  M dev/core/src/com/google/gwt/dev/javac/testing/impl/JavaResourceBase.java
  M dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
  M dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
  A dev/core/src/com/google/gwt/dev/jjs/ast/JSeedIdOf.java
  M dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
  M dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java
  M dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java
  M dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
  M dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
  M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
  M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
  M dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
  M
 dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java
  M 

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[cromwellian]

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: This patch substantially reduces the overhead of Java types in the output by minimizing vtable s... (issue1447821)

[Scott Blum]

Don't worry about it, since I'm punting the review over to Jason and Eric.
:P

-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors