[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)
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/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)
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 wrote: > 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 wrote: > > On Fri, May 13, 2011 at 11:15 AM, Eric Ayers 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)
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 wrote: > On Fri, May 13, 2011 at 11:15 AM, Eric Ayers 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
[gwt-contrib] Re: Pruner runs only once. (issue1436802)
On Fri, May 13, 2011 at 11:15 AM, Eric Ayers 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)
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, 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)
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
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, 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)
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