Prototype re-compiling methods in dart2js

Prototype re-compiling methods in dart2js

This is a prototype with the ability for dart2js to retry the
compilation of a method after everything has been compiled
once. Information collected during the first compilation can then be
used when retrying the compilation.

During compilation information on whether setters are used on specific
fields is collected. When compilation is retried and there are no
setters for a field it is treated as if it was declared final.

As this is a prototype there are some issues we need to discuss, e.g.

* The two queues should probably be generalized some way - maybe with
  listeners on specific changes to the collected information

* Throwing Pass2BailoutException when retrying compilation is a bit of
  a hack. We should probably not try to retry something we can't.

* Where to store the imformation collected during compilation

This is one way of improving Delta Blue with 25-30%.

R=ahe@google.com, ngeoffray@google.com, kasperl@google.com, ager@google.com

BUG=
TEST=

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

[ricow1, 2012-06-07 06:35:49]

DBC

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

https://chromiumcodereview.appspot.com/10537025/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:390: WorkItem work =
world.queue2.removeLast();
should we remove it from the set as well for consistency here? (for now that
will not have any functional difference, but if we start doing more passes I
think we should remove it)

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

https://chromiumcodereview.appspot.com/10537025/diff/1/lib/compiler/implement...
lib/compiler/implementation/enqueue.dart:26: final Queue<WorkItem> queue2;
I would probably call this recompilationCandidates?

[Mads Ager (google), 2012-06-07 07:56:21]

I think this looks promising.

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

https://chromiumcodereview.appspot.com/10537025/diff/1/lib/compiler/implement...
lib/compiler/implementation/world.dart:76: Set<SourceString> setters =
I find the name clash cofusing here. Call this settersForClass or something like
that?

https://chromiumcodereview.appspot.com/10537025/diff/1/lib/compiler/implement...
lib/compiler/implementation/world.dart:82: Set<SourceString> setters =
setters[type.element];
Ditto.

https://chromiumcodereview.appspot.com/10537025/diff/4002/lib/compiler/implem...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/4002/lib/compiler/implem...
lib/compiler/implementation/universe.dart:14: final Map<SourceString,
ClassElement> invokedFieldSetters;
The changes in this file are not used at this point?

[Søren Gjesse, 2012-06-07 08:06:12]

Refactored "queue2" into datastructure. Addressed review comments.

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

https://chromiumcodereview.appspot.com/10537025/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:390: WorkItem work =
world.queue2.removeLast();
On 2012/06/07 06:35:49, ricow1 wrote:
> should we remove it from the set as well for consistency here? (for now that
> will not have any functional difference, but if we start doing more passes I
> think we should remove it)

Done in the RecompilationQueue class.

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

https://chromiumcodereview.appspot.com/10537025/diff/1/lib/compiler/implement...
lib/compiler/implementation/enqueue.dart:26: final Queue<WorkItem> queue2;
On 2012/06/07 06:35:49, ricow1 wrote:
> I would probably call this recompilationCandidates?

Done.

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

https://chromiumcodereview.appspot.com/10537025/diff/1/lib/compiler/implement...
lib/compiler/implementation/world.dart:76: Set<SourceString> setters =
On 2012/06/07 07:56:21, Mads Ager wrote:
> I find the name clash cofusing here. Call this settersForClass or something
like
> that? 

Done.

https://chromiumcodereview.appspot.com/10537025/diff/1/lib/compiler/implement...
lib/compiler/implementation/world.dart:82: Set<SourceString> setters =
setters[type.element];
On 2012/06/07 07:56:21, Mads Ager wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10537025/diff/4002/lib/compiler/implem...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/4002/lib/compiler/implem...
lib/compiler/implementation/universe.dart:14: final Map<SourceString,
ClassElement> invokedFieldSetters;
On 2012/06/07 07:56:21, Mads Ager wrote:
> The changes in this file are not used at this point?

True, removed. I thing I wanted to put the information in the universe first...

[floitsch, 2012-06-07 10:59:39]

LGTM.

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

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:28: void add(Element element,
[TreeElements elements]) {
not that it matters, but afaics the second argument doesn't need to be optional.

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:30: if (element.kind !==
ElementKind.GENERATIVE_CONSTRUCTOR_BODY &&
Explain why these are not recompilation canditates.

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:66: if (compiler.pass == 2) return;
assert that the element has already been compiled?

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1510: if (compiler.pass == 1) {
Shouldn't be needed: we bailout earlier, and even if we didn't the enqueuer
should deal gracefully with multiple addToWorkList.

[Søren Gjesse, 2012-06-13 10:55:09]

PTAL

This is now cleaned up and rebased to ToT.

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

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:28: void add(Element element,
[TreeElements elements]) {
On 2012/06/07 10:59:39, floitsch wrote:
> not that it matters, but afaics the second argument doesn't need to be
optional.

Done.

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:30: if (element.kind !==
ElementKind.GENERATIVE_CONSTRUCTOR_BODY &&
On 2012/06/07 10:59:39, floitsch wrote:
> Explain why these are not recompilation canditates.

Turned out that it is only for generative constructors, not generative
constructor bodies. Not sure why exactly - added a TODO.

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:66: if (compiler.pass == 2) return;
On 2012/06/07 10:59:39, floitsch wrote:
> assert that the element has already been compiled?

Done.

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/8001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1510: if (compiler.pass == 1) {
On 2012/06/07 10:59:39, floitsch wrote:
> Shouldn't be needed: we bailout earlier, and even if we didn't the enqueuer
> should deal gracefully with multiple addToWorkList.

Removed. Actually the bailout is also removed but the check is still not needed.

[floitsch, 2012-06-13 12:51:10]

LGTM.

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
lib/compiler/implementation/enqueue.dart:91: void
addToRecompilationCandidates(Element element, [TreeElements elements]) {
maybe
addRecompilationCandidate
registerRecompilationCandidate
addToRecompilationCandidatesQueue

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
lib/compiler/implementation/enqueue.dart:93: element.getEnclosingClass().name ==
const SourceString("Closure")) {
explain why.

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
lib/compiler/implementation/ssa/optimize.dart:584: // If field is not final or
const but no setters are used then the field
move this comment down to the "isFinalOrConst = true;" ?

[ngeoffray, 2012-06-13 22:02:26]

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
lib/compiler/implementation/ssa/optimize.dart:588:
!compiler.codegenWorld.hasInvokedSetter(field, compiler) &&
Why don't you just use the resolverWorld here?

[Søren Gjesse, 2012-06-14 06:37:10]

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
lib/compiler/implementation/enqueue.dart:91: void
addToRecompilationCandidates(Element element, [TreeElements elements]) {
On 2012/06/13 12:51:10, floitsch wrote:
> maybe
> addRecompilationCandidate
> registerRecompilationCandidate
> addToRecompilationCandidatesQueue

Changed to registerRecompilationCandidate.

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
lib/compiler/implementation/enqueue.dart:93: element.getEnclosingClass().name ==
const SourceString("Closure")) {
On 2012/06/13 12:51:10, floitsch wrote:
> explain why.

Added a function canByRecompiled and placed the explanation there.

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
lib/compiler/implementation/ssa/optimize.dart:584: // If field is not final or
const but no setters are used then the field
On 2012/06/13 12:51:10, floitsch wrote:
> move this comment down to the "isFinalOrConst = true;" ?

Done.

https://chromiumcodereview.appspot.com/10537025/diff/13001/lib/compiler/imple...
lib/compiler/implementation/ssa/optimize.dart:588:
!compiler.codegenWorld.hasInvokedSetter(field, compiler) &&
On 2012/06/13 22:02:26, ngeoffray wrote:
> Why don't you just use the resolverWorld here?

Because I don't know how. If that is possible we should get rid of collection
this information in the universe as well. Lets look at this as a separate
change.

[Søren Gjesse, 2012-06-14 08:31:27]

As discussed offline used the universe from the resolver to check for fields
which are only initialized in the initializer list.

Kept the recompilation framework.

[ngeoffray, 2012-06-14 12:42:24]

LGTM!

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:172: int pass = 1;
This field name is a little too generic for my taste. Maybe a boolean named
isRecompiling?

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

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:30: // TODO(sgjesse): Make this handle
constructor bodies as well.
Why don't we handle them right now?

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:92: // Only member functions can be
recompiled. An exception to this is members
Why not static methods?

[ahe, 2012-06-14 13:23:54]

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:553: log('Compiled
${codegenWorld.generatedCode.length} methods.');
I think you may have broken this message. I think the count remains the same
when recompiling.

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

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:97: return element.isMember() &&
element.getEnclosingClass().name != closure;
This should be:

element.getEnclosingClass() !== compiler.closureClass.

[Søren Gjesse, 2012-06-15 09:25:02]

Final comments addressed in https://chromiumcodereview.appspot.com/10532158.

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:172: int pass = 1;
On 2012/06/14 12:42:24, ngeoffray wrote:
> This field name is a little too generic for my taste. Maybe a boolean named
> isRecompiling?

Changed it to phase and added constants for each phase.

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:553: log('Compiled
${codegenWorld.generatedCode.length} methods.');
On 2012/06/14 13:23:54, ahe wrote:
> I think you may have broken this message. I think the count remains the same
> when recompiling.

Updated to report something more useful. Made the resolver report progress as
well.

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

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:30: // TODO(sgjesse): Make this handle
constructor bodies as well.
On 2012/06/14 12:42:24, ngeoffray wrote:
> Why don't we handle them right now?

I can't remember the exact problem. I will get back to it.

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:92: // Only member functions can be
recompiled. An exception to this is members
On 2012/06/14 12:42:24, ngeoffray wrote:
> Why not static methods?

Doesn't isMember cover static functions as well? And top level functions which
have enclosingClass null?

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:97: return element.isMember() &&
element.getEnclosingClass().name != closure;
On 2012/06/14 13:23:54, ahe wrote:
> This should be:
> 
> element.getEnclosingClass() !== compiler.closureClass.

Done.

[ngeoffray, 2012-06-15 20:06:14]

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

https://chromiumcodereview.appspot.com/10537025/diff/7003/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:92: // Only member functions can be
recompiled. An exception to this is members
On 2012/06/15 09:25:02, Søren Gjesse wrote:
> On 2012/06/14 12:42:24, ngeoffray wrote:
> > Why not static methods?
> 
> Doesn't isMember cover static functions as well? And top level functions which
> have enclosingClass null?

you are right that isMember covers static functions, but it does not cover top
level functions.