Introduce typed selectors to do better tree shaking based on calls on 'this'. Getters and setters w…

Introduce typed selectors to do better tree shaking based on calls on 'this'. Getters and setters will be next.
Committed: https://code.google.com/p/dart/source/detail?r=6847

[ngeoffray, 2012-04-23 12:16:11]

FYI: from 883250 to 875200 bytes generated for swarm.

[kasperl, 2012-04-23 12:34:45]

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:850: * Returns true if [cls]
is a subclass of [this].
Shouldn't this be: if [this] is a subclass of [cls]?

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:856: for (ClassElement s =
superclass; s != null; s = s.superclass) {
It is a bit surprising that you don't start at [this]. Document it or fix it?

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/universe.dart:81: bool applies(FunctionElement
element, Compiler compiler) {
Not sure I understand the name of this method.

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/universe.dart:237: class TypedInvocation extends
Invocation {
So this is a special invocation where we know something about the type of the
receiver? Maybe add that as a comment.

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/universe.dart:238: final Type type;
type -> receiverType?

[ahe, 2012-04-23 12:37:53]

FYI

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:850: * Returns true if [cls]
is a subclass of [this].
On 2012/04/23 12:34:45, kasperl wrote:
> Shouldn't this be: if [this] is a subclass of [cls]?

Good catch.

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:856: for (ClassElement s =
superclass; s != null; s = s.superclass) {
On 2012/04/23 12:34:45, kasperl wrote:
> It is a bit surprising that you don't start at [this]. Document it or fix it?

Good catch.

I think this is a copy-and-paste error that Karl and I caused :-)

[ngeoffray, 2012-04-23 13:48:18]

Thanks for the comments, PTAL

http://codereview.chromium.org/10180001/diff/1/lib/compiler/implementation/el...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/10180001/diff/1/lib/compiler/implementation/el...
lib/compiler/implementation/elements/elements.dart:850: * Returns true if [cls]
is a subclass of [this].
On 2012/04/23 12:34:45, kasperl wrote:
> Shouldn't this be: if [this] is a subclass of [cls]?

Done.

http://codereview.chromium.org/10180001/diff/1/lib/compiler/implementation/el...
lib/compiler/implementation/elements/elements.dart:856: for (ClassElement s =
superclass; s != null; s = s.superclass) {
On 2012/04/23 12:34:45, kasperl wrote:
> It is a bit surprising that you don't start at [this]. Document it or fix it?

The method should start at this. Code fixed.

http://codereview.chromium.org/10180001/diff/1/lib/compiler/implementation/un...
File lib/compiler/implementation/universe.dart (right):

http://codereview.chromium.org/10180001/diff/1/lib/compiler/implementation/un...
lib/compiler/implementation/universe.dart:81: bool applies(FunctionElement
element, Compiler compiler) {
On 2012/04/23 12:34:45, kasperl wrote:
> Not sure I understand the name of this method.

The method is whether the selector applies to this element. I'm of course open
to other names.

http://codereview.chromium.org/10180001/diff/1/lib/compiler/implementation/un...
lib/compiler/implementation/universe.dart:237: class TypedInvocation extends
Invocation {
On 2012/04/23 12:34:45, kasperl wrote:
> So this is a special invocation where we know something about the type of the
> receiver? Maybe add that as a comment.

Done.

http://codereview.chromium.org/10180001/diff/1/lib/compiler/implementation/un...
lib/compiler/implementation/universe.dart:238: final Type type;
On 2012/04/23 12:34:45, kasperl wrote:
> type -> receiverType?

Done.

[floitsch, 2012-04-23 13:58:30]

LGTM.

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:852: * This method is not to
be used for checking type hierarchy and
Add explanation why, or contrast to it ("whereas a assignment check would
need..., this function only ...")

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/emitter.dart:171: FunctionParameters parameters =
member.computeParameters(compiler);
unused?

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1034: Element method =
cls.lookupMember(node.name);
Is this just to reduce the number of typed invocations?
Unless I'm wrong it yields more compiled methods:
class A { foo() => 42; }
class B extends A { ... foo() ... } // => cls = A.
class C extends A { foo() => 499; }
Since C is a subclass of 'cls' (A) we will compile C.foo, even though we could
have told that nobody actually invokes it.

[ngeoffray, 2012-04-23 14:09:32]

PTAL

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:852: * This method is not to
be used for checking type hierarchy and
On 2012/04/23 13:58:30, floitsch wrote:
> Add explanation why, or contrast to it ("whereas a assignment check would
> need..., this function only ...")

Done.

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/emitter.dart:171: FunctionParameters parameters =
member.computeParameters(compiler);
On 2012/04/23 13:58:30, floitsch wrote:
> unused?

Done.

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1034: Element method =
cls.lookupMember(node.name);
On 2012/04/23 13:58:30, floitsch wrote:
> Is this just to reduce the number of typed invocations?

Yes.

> Unless I'm wrong it yields more compiled methods:
> class A { foo() => 42; }
> class B extends A { ... foo() ... } // => cls = A.
> class C extends A { foo() => 499; }
> Since C is a subclass of 'cls' (A) we will compile C.foo, even though we could
> have told that nobody actually invokes it.

Ok, so we need A.foo() to be compiled and actually just set the typed selector
to B. Does that make sense?

[floitsch, 2012-04-23 15:01:51]

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1034: Element method =
cls.lookupMember(node.name);
On 2012/04/23 14:09:33, ngeoffray wrote:
> On 2012/04/23 13:58:30, floitsch wrote:
> > Is this just to reduce the number of typed invocations?
> 
> Yes.
> 
> > Unless I'm wrong it yields more compiled methods:
> > class A { foo() => 42; }
> > class B extends A { ... foo() ... } // => cls = A.
> > class C extends A { foo() => 499; }
> > Since C is a subclass of 'cls' (A) we will compile C.foo, even though we
could
> > have told that nobody actually invokes it.
> 
> Ok, so we need A.foo() to be compiled and actually just set the typed selector
> to B. Does that make sense?

Yes. I think that would be the more "correct" version. However reducing the
typed invocations might be quite beneficial. For now either version is ok with
me, but should be documented. If it doesn't cost too much I would prefer to go
with the one that prunes more first, and only if we have too many different
typed invocations switch to sharing.
Note that not sharing also simplifies the code.

[ngeoffray, 2012-04-23 15:22:47]

Thanks Florian, Kasper and Peter.

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

https://chromiumcodereview.appspot.com/10180001/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1034: Element method =
cls.lookupMember(node.name);
On 2012/04/23 15:01:51, floitsch wrote:
> On 2012/04/23 14:09:33, ngeoffray wrote:
> > On 2012/04/23 13:58:30, floitsch wrote:
> > > Is this just to reduce the number of typed invocations?
> > 
> > Yes.
> > 
> > > Unless I'm wrong it yields more compiled methods:
> > > class A { foo() => 42; }
> > > class B extends A { ... foo() ... } // => cls = A.
> > > class C extends A { foo() => 499; }
> > > Since C is a subclass of 'cls' (A) we will compile C.foo, even though we
> could
> > > have told that nobody actually invokes it.
> > 
> > Ok, so we need A.foo() to be compiled and actually just set the typed
selector
> > to B. Does that make sense?
> 
> Yes. I think that would be the more "correct" version. However reducing the
> typed invocations might be quite beneficial. For now either version is ok with
> me, but should be documented. If it doesn't cost too much I would prefer to go
> with the one that prunes more first, and only if we have too many different
> typed invocations switch to sharing.
> Note that not sharing also simplifies the code.

Added a comment.