[gwt-contrib] Re: Pruner runs only once. (issue1436802)
http://gwt-code-reviews.appspot.com/1436802/diff/4001/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/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode84
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:84: *
Marks as referenced any types, methods, and fields that are reachable.
On 2011/05/20 20:14:17, jbrosenberg wrote:
s/any the classes/any classes/
Done.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode382
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:382:
Done, factored out the into a well-documented method,
rescueArgumentsIfParametersCanBeRead.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode385
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:385:
if (program.staticImplFor(method) != null) {
On 2011/05/20 20:14:17, jbrosenberg wrote:
Pruner.CleanupRefsVisitor
Done.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode772
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:772:
On 2011/05/20 20:14:17, jbrosenberg wrote:
Perhaps add a comment for this map, similar to the Schrodinger's
comment below
for membersLiveExceptForInstantiability. I'm a bit confused by the
name
argsLiveExceptForParamRead, how about argsToAcceptIfParamRead.
Done.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode787
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:787:
*/
On 2011/05/20 20:14:17, jbrosenberg wrote:
How about membersToRescueIfInstantiable
Done.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
File dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java (right):
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java#newcode421
dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java:421: *
devirtualizations. If the instance method has been pruned, then it's
On 2011/05/20 20:14:17, jbrosenberg wrote:
Here I think it's assumed saveCodeGenTypes == true = it's the final
pass.
Perhaps this should be made more clear, or the comment updated to
clarify.
Done.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java#newcode570
dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java:570: // Retain the
original arguments, they will be evaluated for side effects.
Yeah, when I went and reread the spec, I discovered that arguments are
evaluated before NPE is checked. :/
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java#newcode600
dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java:600:
On 2011/05/20 20:14:17, jbrosenberg wrote:
Is it now certain that the Pruner should not need any subsequent
passes to
complete all possible pruning? Or is it most of the way there, and
this is
now fair to cut it off to save time?
It's not 100%, but it's close enough that not iterating on Pruner
doesn't add more passes to the outer optimization loop for the compiles
I've tried.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java
File dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java (right):
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java#newcode145
dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java:145:
program.addEntryMethod(findMainMethod(program));
The test contains one of the few cases that still tickles through,
namely the transformation from x.foo() - null.nullMethod() where x is
eventually pruned. It seemed more expedient to realize the immediate
benefit and let someone else solve the last case later. It was tricky
enough that I would have needed to invest more time than I had on it.
We don't have a test for the 'false' case.
http://gwt-code-reviews.appspot.com/1436802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
LGTM http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java File dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java (right): http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java#newcode145 dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java:145: program.addEntryMethod(findMainMethod(program)); Can you leave a TODO comment for the possible follow-on work there? http://gwt-code-reviews.appspot.com/1436802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java File dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java (right): http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java#newcode145 dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java:145: program.addEntryMethod(findMainMethod(program)); On 2011/05/23 17:26:46, jbrosenberg wrote: Can you leave a TODO comment for the possible follow-on work there? Done. http://gwt-code-reviews.appspot.com/1436802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
Most of the way there to signing off, with a few questions/suggestions.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/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/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode84
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:84: *
Marks as referenced any types, methods, and fields that are reachable.
s/any the classes/any classes/
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode382
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:382:
This is specific only to Pruner.CleanupRefsVisitor? Why is it
'crazy'? Seems pretty straightforward, this is where you are populating
the argsLiveExceptForParamRead map. Maybe describe why this useful (or
add a comment below where argsLiveExceptForParamRed is declared, as is
done for membersLiveExceptForInstantiability).
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode385
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:385:
if (program.staticImplFor(method) != null) {
Pruner.CleanupRefsVisitor
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode772
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:772:
Perhaps add a comment for this map, similar to the Schrodinger's
comment below for membersLiveExceptForInstantiability. I'm a bit
confused by the name argsLiveExceptForParamRead, how about
argsToAcceptIfParamRead.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode787
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:787:
*/
How about membersToRescueIfInstantiable
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
File dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java (right):
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java#newcode421
dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java:421: *
devirtualizations. If the instance method has been pruned, then it's
Here I think it's assumed saveCodeGenTypes == true = it's the final
pass. Perhaps this should be made more clear, or the comment updated
to clarify.
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java#newcode570
dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java:570: // Retain the
original arguments, they will be evaluated for side effects.
This seems like a good change!
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java#newcode600
dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java:600:
Is it now certain that the Pruner should not need any subsequent passes
to complete all possible pruning? Or is it most of the way there, and
this is now fair to cut it off to save time?
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java
File dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java (right):
http://gwt-code-reviews.appspot.com/1436802/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java#newcode145
dev/core/test/com/google/gwt/dev/jjs/impl/PrunerTest.java:145:
program.addEntryMethod(findMainMethod(program));
Why is this looping necessary? Since the change in Pruner removes
looping, shouldn't we be testing here the behavior with no looping also?
Do we have a test for the second arg to Pruner.exec() is false?
http://gwt-code-reviews.appspot.com/1436802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Pruner runs only once. (issue1436802)
Cool beans. On Mon, May 16, 2011 at 8:14 PM, Ray Cromwell cromwell...@google.comwrote: I've done a quick scan of this and it looks ok, but I'd like to take some time tonight to review it in more detail. I like the 'membersLiveExceptForInstantiability' change, I was going the suggest something like that when I was doing the JSO CFA overhaul last time. Also the checks in Pruner are simpler now. On Fri, May 13, 2011 at 11:37 AM, Scott Blum sco...@google.com wrote: On Fri, May 13, 2011 at 11:15 AM, Eric Ayers zun...@google.com wrote: I've been reading the code and talking to Scott about it. The loop being removed is the while() loop in execImpl(). The jitter is the fact that the ControlFlowAnalyzer might return one result for liveness before the Pruner runs, and a different result after Pruner runs. If you don't loop inside of Pruner, then the entire optimization pass may have to run extra times. Great explanation! The last time we tried to naively remove the while loop from Pruner, the outer optimization loop went from ~7 passes to ~10 passes for Showcase, and the total optimization time went up. You can think of my patch as converging faster. With my patch in and the Pruner loop gone, you still only get ~7 outer optimization loops, and the total optimization time goes down. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Pruner runs only once. (issue1436802)
Sorry for the delay, spent most of today catching up after I/O, I will take care of these Friday. On Tue, May 10, 2011 at 6:44 PM, sco...@google.com wrote: This is ready for review now. http://gwt-code-reviews.appspot.com/1436802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
Can you clarify what you mean by Pruner runs only once? It looks like it will still be invoked multiple times by the optimizeLoop, etc. Can you provide some background on the jitter phenomena? http://gwt-code-reviews.appspot.com/1436802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
Some background - some optimizers run themselves in a loop on each pass of the optimizer. During some performance investigations last year, we tried to get some of these extra loops out, but Pruner was a tough nut to crack. The nasty thing about this looping behavior is that the last pass through is always wasted (you know to exit the loop when the optimizer doesn't change the tree.) I've been reading the code and talking to Scott about it. The loop being removed is the while() loop in execImpl(). The jitter is the fact that the ControlFlowAnalyzer might return one result for liveness before the Pruner runs, and a different result after Pruner runs. If you don't loop inside of Pruner, then the entire optimization pass may have to run extra times. On Fri, May 13, 2011 at 11:03 AM, jbrosenb...@google.com wrote: Can you clarify what you mean by Pruner runs only once? It looks like it will still be invoked multiple times by the optimizeLoop, etc. Can you provide some background on the jitter phenomena? http://gwt-code-reviews.appspot.com/1436802/ -- Eric Z. Ayers Google Web Toolkit, Atlanta, GA USA -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
On Fri, May 13, 2011 at 11:15 AM, Eric Ayers zun...@google.com wrote: I've been reading the code and talking to Scott about it. The loop being removed is the while() loop in execImpl(). The jitter is the fact that the ControlFlowAnalyzer might return one result for liveness before the Pruner runs, and a different result after Pruner runs. If you don't loop inside of Pruner, then the entire optimization pass may have to run extra times. Great explanation! The last time we tried to naively remove the while loop from Pruner, the outer optimization loop went from ~7 passes to ~10 passes for Showcase, and the total optimization time went up. You can think of my patch as converging faster. With my patch in and the Pruner loop gone, you still only get ~7 outer optimization loops, and the total optimization time goes down. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
This is ready for review now. http://gwt-code-reviews.appspot.com/1436802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
Actually, don't review this yet, I meant to just update the description rather than send an email out. http://gwt-code-reviews.appspot.com/1436802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
