Some cleanup of recompilation

Some cleanup of recompilation

Addressed additional comments to https://chromiumcodereview.appspot.com/10537025.

R=ahe@google.com

BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=8766

[ahe, 2012-06-15 14:33:16]

Brilliant! LGTM

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:186: int phase;
I agree, this field is a compiler.phase. Perhaps we should add one more phase:

SCANNING, RESOLVING, COMPILING, RECOMPILING.

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:547: log('Resolved
${enqueuer.resolution.resolvedElements.length} elements.');
Nice, but the line is too long :-)

[ngeoffray, 2012-06-15 20:12:59]

LGTM

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/optimize.dart:595: work.element);
I think putting such considerations into the new SSA phase you introduce in
another CL might be good.

Also, since you are unconditionally recompiling once a method is in the
recompliation queue, I think this phase should return a bool telling whether the
method is a candidate for recompilation. If it says true, then compilation is
aborted. This should save some time in total compile-time.

[Søren Gjesse, 2012-06-18 06:40:36]

Thanks for the review.

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:186: int phase;
On 2012/06/15 14:33:16, ahe wrote:
> I agree, this field is a compiler.phase. Perhaps we should add one more phase:
> 
> SCANNING, RESOLVING, COMPILING, RECOMPILING.

Added scanning.

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:547: log('Resolved
${enqueuer.resolution.resolvedElements.length} elements.');
On 2012/06/15 14:33:16, ahe wrote:
> Nice, but the line is too long :-)

Done.

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10532158/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/optimize.dart:595: work.element);
On 2012/06/15 20:12:59, ngeoffray wrote:
> I think putting such considerations into the new SSA phase you introduce in
> another CL might be good.
> 
> Also, since you are unconditionally recompiling once a method is in the
> recompliation queue, I think this phase should return a bool telling whether
the
> method is a candidate for recompilation. If it says true, then compilation is
> aborted. This should save some time in total compile-time.

I will move it to the new SSA phase when it is in through another CL.

Regarding aborting compilation, then I am not sure. Most of the information used
for feedback into the recompilation is currently collected in the code
generator, so if compilation is aborted then this information will not be
collected. Maybe we should move this gathering of information out of the codegen
and have a separate step for that. We can then skip the actual codegen for
recompilation candidates. Another option will be to have some predicates on the
recompilation candidates and check these before the actual recompilation. That
way we only recompile for profit.

[Søren Gjesse, 2012-06-18 07:00:29]

https://chromiumcodereview.appspot.com/10532158/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10532158/diff/7002/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:101: element.getEnclosingClass() !=
compiler.closureClass;
Turned out that comparing with compiler.closureClass did not work.

[ahe, 2012-06-18 07:38:50]

https://chromiumcodereview.appspot.com/10532158/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10532158/diff/7002/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:101: element.getEnclosingClass() !=
compiler.closureClass;
On 2012/06/18 07:00:29, Søren Gjesse wrote:
> Turned out that comparing with compiler.closureClass did not work.

It MUST work. If it doesn't, then something is wrong. For example, perhaps you
have to look at:

element.getEnclosingClass().superclass

[Søren Gjesse, 2012-06-18 07:44:10]

https://chromiumcodereview.appspot.com/10532158/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10532158/diff/7002/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:101: element.getEnclosingClass() !=
compiler.closureClass;
On 2012/06/18 07:38:50, ahe wrote:
> On 2012/06/18 07:00:29, Søren Gjesse wrote:
> > Turned out that comparing with compiler.closureClass did not work.
> 
> It MUST work. If it doesn't, then something is wrong. For example, perhaps you
> have to look at:
> 
> element.getEnclosingClass().superclass

Then something is wrong. Comparing with compiler.closureClass does not detect
the classes created for local functions and closures. The reason for skipping
them here is that the call function on then cannot be recompiled as they do not
exist as separate top level elements. Maybe they are constructed the wrong way.

[ahe, 2012-06-18 07:55:00]

https://chromiumcodereview.appspot.com/10532158/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10532158/diff/7002/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:101: element.getEnclosingClass() !=
compiler.closureClass;
On 2012/06/18 07:44:10, Søren Gjesse wrote:
> On 2012/06/18 07:38:50, ahe wrote:
> > On 2012/06/18 07:00:29, Søren Gjesse wrote:
> > > Turned out that comparing with compiler.closureClass did not work.
> > 
> > It MUST work. If it doesn't, then something is wrong. For example, perhaps
you
> > have to look at:
> > 
> > element.getEnclosingClass().superclass
> 
> Then something is wrong. Comparing with compiler.closureClass does not detect
> the classes created for local functions and closures. The reason for skipping
> them here is that the call function on then cannot be recompiled as they do
not
> exist as separate top level elements. Maybe they are constructed the wrong
way.

I don't know. All I know is testing for names does not work. Do you need help
debugging this?