Add a type kind to TypedSelector. A typed selector can either be

Add a type kind to TypedSelector. A typed selector can either be
- EXACT: it knows the exact type of the receiver (e.g. new Map()).
- SUBCLASS: the type is in the subclass hierarchy (e.g. 'this')
- INTERFACE: any type that implements the receiver type is a target.
Committed: https://code.google.com/p/dart/source/detail?r=18311

[karlklose, 2013-02-08 20:20:34]

Could you give an example where knowing the difference between implementing an
interface and being a subclass makes a difference with this CL?

[ngeoffray, 2013-02-08 22:07:51]

On 2013/02/08 20:20:34, karlklose wrote:
> Could you give an example where knowing the difference between implementing an
> interface and being a subclass makes a difference with this CL?

Currently, this CL improves two things:

- Compile times: function sets, that I plan on using more for our resolution
queries, do not always need to go in slow mode if a class is implemented by
another class

- We do better tree shaking: a call to this.foo() in class A does not generate
code for foo methods of  classes that implement A.

And there are many more to come, like more sends we know are monomorphic, no
overriding methods, etc. The fact that we were using interface types before made
the compiler correct, but we are missing tons of optimizations.

[ngeoffray, 2013-02-08 22:10:35]

On 2013/02/08 22:07:51, ngeoffray wrote:
> On 2013/02/08 20:20:34, karlklose wrote:
> > Could you give an example where knowing the difference between implementing
an
> > interface and being a subclass makes a difference with this CL?
> 
> Currently, this CL improves two things:
> 
> - Compile times: function sets, that I plan on using more for our resolution
> queries, do not always need to go in slow mode if a class is implemented by
> another class
> 
> - We do better tree shaking: a call to this.foo() in class A does not generate
> code for foo methods of  classes that implement A.

For example, this CL saves 11k on swarm.

> 
> And there are many more to come, like more sends we know are monomorphic, no
> overriding methods, etc. The fact that we were using interface types before
made
> the compiler correct, but we are missing tons of optimizations.

[sra1, 2013-02-08 23:25:16]

https://codereview.chromium.org/12207081/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12207081/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:430: if
(receiverType != null) type = ', type=$receiverType';
Include the typeKind in the string, e.g.

type = ', type=$typeKind $receiverType'

[kasperl, 2013-02-11 08:18:12]

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/enqueue.dart:359: new
TypedSelector(type, TypedSelectorKind.SUBCLASS, getter),
Wouldn't it be logical to have named constructors on the TypedSelector class for
these things? 

   new TypedSelector.subclass(type, getter)

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:435: isInterfaceType:
false);
This needs to take mixins into account. The type of this can be an interface
type if the method that reads this is in a class that is used as a mixin. A test
case for that would be good.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4030:
pushInvokeHelper1(backend.getMapMaker(), keyValuePairs,
A comment that explains why this type isn't exact would be nice.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4629: return new
HType.fromBoundedType(
How many different variants of this constructor are actually used? Would it make
sense to just have them like:

   new HType.exactClass(type, compiler)
   new HType.nonNullExactClass(type, compiler)
   new HType.subclass(type, compiler)
   new HType.nonNullSubclass(type, compiler)
   new HType.subtype(type, compiler)
   new HType.nonNullSubtype(type, compiler)

To me that's a lot more readable.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1155: HType
convertedType = new HType.fromBoundedType(
This one would have to be:

   new HType.subtype(type, compiler, nonNull: !canBeNull)

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:7: abstract class HType
{
I wonder how many HType instances we create at runtime? How hard would it be to
canonicalize these at runtime? They are immutable.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/universe/function_set.dart
(right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/universe/function_set.dart:40: bool
shouldDoVisitAll(Selector selector) {
shouldVisitAll

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:453:
assert(!identical(receiverType.kind, TypeKind.MALFORMED_TYPE));
Assert that typeKind isn't unknown?

[ahe, 2013-02-11 09:35:03]

DBC, I think this functionality should be tracked by the type system. This way,
the more type inference we add, the better code is generated.

[ahe, 2013-02-11 09:35:05]

DBC, I think this functionality should be tracked by the type system. This way,
the more type inference we add, the better code is generated.

[ahe, 2013-02-11 09:35:08]

DBC, I think this functionality should be tracked by the type system. This way,
the more type inference we add, the better code is generated.

[ngeoffray, 2013-02-11 10:20:55]

Thanks Kasper. PTAL

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/enqueue.dart:359: new
TypedSelector(type, TypedSelectorKind.SUBCLASS, getter),
On 2013/02/11 08:18:12, kasperl wrote:
> Wouldn't it be logical to have named constructors on the TypedSelector class
for
> these things? 
> 
>    new TypedSelector.subclass(type, getter)

Done.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:435: isInterfaceType:
false);
On 2013/02/11 08:18:12, kasperl wrote:
> This needs to take mixins into account. The type of this can be an interface
> type if the method that reads this is in a class that is used as a mixin. A
test
> case for that would be good.

Done.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4030:
pushInvokeHelper1(backend.getMapMaker(), keyValuePairs,
On 2013/02/11 08:18:12, kasperl wrote:
> A comment that explains why this type isn't exact would be nice.

Done.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4629: return new
HType.fromBoundedType(
On 2013/02/11 08:18:12, kasperl wrote:
> How many different variants of this constructor are actually used? Would it
make
> sense to just have them like:
> 
>    new HType.exactClass(type, compiler)
>    new HType.nonNullExactClass(type, compiler)
>    new HType.subclass(type, compiler)
>    new HType.nonNullSubclass(type, compiler)
>    new HType.subtype(type, compiler)
>    new HType.nonNullSubtype(type, compiler)
> 
> To me that's a lot more readable.

Done.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1155: HType
convertedType = new HType.fromBoundedType(
On 2013/02/11 08:18:12, kasperl wrote:
> This one would have to be:
> 
>    new HType.subtype(type, compiler, nonNull: !canBeNull)

Done.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:7: abstract class HType
{
On 2013/02/11 08:18:12, kasperl wrote:
> I wonder how many HType instances we create at runtime? How hard would it be
to
> canonicalize these at runtime? They are immutable. 

I don't think it's difficult, they are indeed immutable. On the other hand, they
get deleted after each compilation of an element, so it's not like it should be
a big memory hit right now. Once we start sharing a type system with another
component of the compiler (e.g. removing the SSA type system with an extended
Dart type system?), I believe we will have to canonicalize them.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/universe/function_set.dart
(right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/universe/function_set.dart:40: bool
shouldDoVisitAll(Selector selector) {
On 2013/02/11 08:18:12, kasperl wrote:
> shouldVisitAll

Done.

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12207081/diff/11/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:453:
assert(!identical(receiverType.kind, TypeKind.MALFORMED_TYPE));
On 2013/02/11 08:18:12, kasperl wrote:
> Assert that typeKind isn't unknown?

Done.

[ngeoffray, 2013-02-11 10:24:26]

On 2013/02/11 09:35:08, ahe wrote:
> DBC, I think this functionality should be tracked by the type system. This
way,
> the more type inference we add, the better code is generated.

Could not agree more. Did you feel so strongly about it that you had to write it
three times? :-) With my work on type inference, the need for sharing that
functionality is kind of stressed out. I hope I'll be able to get to that point.

[kasperl, 2013-02-11 13:11:50]

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:359: new
TypedSelector.subclass(type, getter),
Aren't we throwing away information at this point? Couldn't this take a selector
instead of a getterName, a library, and a type? The same goes for the setter.

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1573:
optimizedSelector = new TypedSelector.exact(
How about just returning it here and get rid of the optimizedSelector local?

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:73: factory
HType.exactClass(DartType type, Compiler compiler) {
Do you actually use this? In which cases?

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:91: factory
HType.subclass(DartType type, Compiler compiler) {
Do you actually use this? In which cases?

[ngeoffray, 2013-02-11 14:20:29]

PTAL

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:359: new
TypedSelector.subclass(type, getter),
On 2013/02/11 13:11:50, kasperl wrote:
> Aren't we throwing away information at this point? Couldn't this take a
selector
> instead of a getterName, a library, and a type? The same goes for the setter.

Actually, we should just take an element. Done.

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1573:
optimizedSelector = new TypedSelector.exact(
On 2013/02/11 13:11:50, kasperl wrote:
> How about just returning it here and get rid of the optimizedSelector local?

Done.

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:73: factory
HType.exactClass(DartType type, Compiler compiler) {
On 2013/02/11 13:11:50, kasperl wrote:
> Do you actually use this? In which cases?

No(t yet, I believe). Removed.

https://codereview.chromium.org/12207081/diff/1014/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:91: factory
HType.subclass(DartType type, Compiler compiler) {
On 2013/02/11 13:11:50, kasperl wrote:
> Do you actually use this? In which cases?

Done.

[kasperl, 2013-02-11 14:30:34]

LGTM.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:256: if
(compiler.world.isUsedAsMixin(cls)) {
Add comment to explain why this is done?

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:429: HType
cachedTypeOfThis;
Could the cachedTypeOfThis reading/writing be moved to computeTypeOfThis
instead?

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4631:
compiler.mapLiteralClass.computeType(compiler), compiler);
It's very common that you call computeType as part of calling one of the HType
constructors? Maybe the constructors could do that for you instead? So instead
of passing a DartType you would pass an element? Not sure if this is a good idea
or not.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4648: HType
mapNativeType(type) {
What is the type of type?

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:827: bool canBeNull()
=> _canBeNull;
Consider changing these to be properties/getters in a different CL.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/universe/function_set.dart
(right):

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/universe/function_set.dart:50: //
TODO(kasperl): For now, we use a different implementation for
Move the TODO comment to shouldVisitAll?

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/universe/function_set.dart:62: //
TODO(kasperl): For now, we use a different implementation for
Ditto.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/world.dart:28: bool
isUsedAsMixin(ClassElement cls) {
Move closer to registerMixinUse.

[ngeoffray, 2013-02-11 14:53:29]

Thanks Kasper

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:256: if
(compiler.world.isUsedAsMixin(cls)) {
On 2013/02/11 14:30:34, kasperl wrote:
> Add comment to explain why this is done?

Done.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:429: HType
cachedTypeOfThis;
On 2013/02/11 14:30:34, kasperl wrote:
> Could the cachedTypeOfThis reading/writing be moved to computeTypeOfThis
> instead?

Done.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4631:
compiler.mapLiteralClass.computeType(compiler), compiler);
On 2013/02/11 14:30:34, kasperl wrote:
> It's very common that you call computeType as part of calling one of the HType
> constructors? Maybe the constructors could do that for you instead? So instead
> of passing a DartType you would pass an element? Not sure if this is a good
idea
> or not.

Sometimes, you do have the DartType available and call new HType with it. So we
would need to add new constructors like: Htype.nonNullSubTypeWithElement(...).
I'd prefer not to.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4648: HType
mapNativeType(type) {
On 2013/02/11 14:30:34, kasperl wrote:
> What is the type of type?

It can be of different classes (DartType or ad-hoc types for JsObject and
JsArray), so I cannot type it. I hope this is temporary and a better type system
will fix it.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/universe/function_set.dart
(right):

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/universe/function_set.dart:50: //
TODO(kasperl): For now, we use a different implementation for
On 2013/02/11 14:30:34, kasperl wrote:
> Move the TODO comment to shouldVisitAll?

Done.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/universe/function_set.dart:62: //
TODO(kasperl): For now, we use a different implementation for
On 2013/02/11 14:30:34, kasperl wrote:
> Ditto.

Done.

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12207081/diff/13/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/world.dart:28: bool
isUsedAsMixin(ClassElement cls) {
On 2013/02/11 14:30:34, kasperl wrote:
> Move closer to registerMixinUse.

Done.