Widen big concrete types to dynamic.

Widen big concrete types to dynamic.

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

[polux, 2012-11-29 13:51:01]

Peter: I'm putting you as a reviewer as well in case you have time to review it
before Karl comes back.

[ahe, 2012-11-29 14:07:36]

Could you think of a way to avoid static state?

https://chromiumcodereview.appspot.com/11348297/diff/1/sdk/lib/_internal/comp...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://chromiumcodereview.appspot.com/11348297/diff/1/sdk/lib/_internal/comp...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:79:
static int MAX_CONCRETE_TYPE_SIZE = 5;
As far as I know, we have no static mutable state in dart2js.  I'd like to keep
it that way.  The static state in the analyzer (nee dartc) is a continuing
source of flakiness on our build bots.

[karlklose, 2012-12-13 14:55:11]

The concrete type are actually dependent types (on the dynamic type and the
maximal size before widening), so perhaps you should make that explicit by
having each type have a reference to a 'type system'?

https://codereview.chromium.org/11348297/diff/1/sdk/lib/_internal/compiler/im...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://codereview.chromium.org/11348297/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:81:
factory ConcreteType.empty() => new UnionType(new Set<BaseType>());
This could be a constant.

[polux, 2012-12-13 15:40:36]

https://chromiumcodereview.appspot.com/11348297/diff/1/sdk/lib/_internal/comp...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://chromiumcodereview.appspot.com/11348297/diff/1/sdk/lib/_internal/comp...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:79:
static int MAX_CONCRETE_TYPE_SIZE = 5;
On 2012/11/29 14:07:36, ahe wrote:
> As far as I know, we have no static mutable state in dart2js.  I'd like to
keep
> it that way.  The static state in the analyzer (nee dartc) is a continuing
> source of flakiness on our build bots.

Done.

https://chromiumcodereview.appspot.com/11348297/diff/1/sdk/lib/_internal/comp...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:81:
factory ConcreteType.empty() => new UnionType(new Set<BaseType>());
On 2012/12/13 14:55:11, karlklose wrote:
> This could be a constant.

Not anymore :) But it is now in ConcreteTypesInferrer.

[ahe, 2012-12-20 10:18:22]

LGTM, but I think you should try to see if you can extend the framework to
handle "abstract" types that are a compact representation of all subclasses in a
subtree.

For example, if you have these classes:

class A {}
class B extends A {}
class C extends A {}

And maxConcreteTypeSize is one, then instead of returning:

new UnknownConcreteType(inferrer)

You could return:

new AllSubclassesOfButNotNull(A) // pseudo code.

https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/compiler.dart:118: final int
maxConcreteTypeSize;
Please document this variable.

https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:173:
? new UnknownConcreteType(inferrer)
You're discarding information here.  The abstract type which is an upper bound
of newBaseTypes holds more information.

[polux, 2012-12-20 10:31:50]

I've answered your toplevel suggestion in the corresponding comment in
concrete_types_inferrer.dart.

https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/compiler.dart:118: final int
maxConcreteTypeSize;
On 2012/12/20 10:18:22, ahe wrote:
> Please document this variable.

Done.

https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
File
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
(right):

https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:173:
? new UnknownConcreteType(inferrer)
On 2012/12/20 10:18:22, ahe wrote:
> You're discarding information here.  The abstract type which is an upper bound
> of newBaseTypes holds more information.

For now I've been assuming concrete types only hold non-abstract classes. I'm
not sure this assumption has been used in the codebase but I'm sticking to
dynamic for the time being to be on te safe side.

It was my intent to consider abstract types (especially because of the DOM
bindings) but I have to think about the consequences.

[polux, 2012-12-20 11:14:14]

PTAL

The tests were failing because TypesTask was still calling new
Singleton(baseType) instead of inferrer.singleton(baseType). For simplicity, I
have fixed that by always creating an inferrer in TypesTask but only using it
when the flag was set.

This might have a (very) minor performance impact since the constructor of
ConcreteTypesInferrer allocates a bunch of maps and lists. Until now, my changes
had 0 impact on dart2js unless --enable-concrete-type-inference was set. That's
why I want you to take another look.

On 2012/12/20 10:31:50, polux wrote:
> I've answered your toplevel suggestion in the corresponding comment in
> concrete_types_inferrer.dart.
> 
>
https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
> File sdk/lib/_internal/compiler/implementation/compiler.dart (right):
> 
>
https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
> sdk/lib/_internal/compiler/implementation/compiler.dart:118: final int
> maxConcreteTypeSize;
> On 2012/12/20 10:18:22, ahe wrote:
> > Please document this variable.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
> File
> sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart
> (right):
> 
>
https://chromiumcodereview.appspot.com/11348297/diff/8001/sdk/lib/_internal/c...
>
sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart:173:
> ? new UnknownConcreteType(inferrer)
> On 2012/12/20 10:18:22, ahe wrote:
> > You're discarding information here.  The abstract type which is an upper
bound
> > of newBaseTypes holds more information.
> 
> For now I've been assuming concrete types only hold non-abstract classes. I'm
> not sure this assumption has been used in the codebase but I'm sticking to
> dynamic for the time being to be on te safe side.
> 
> It was my intent to consider abstract types (especially because of the DOM
> bindings) but I have to think about the consequences.

[ahe, 2012-12-20 12:26:21]

I think it would be simpler and cleaner if you passed in an instance of inferrer
when you need it.  As far as I can tell, there is only a few operations that
need the inferrer.

[polux, 2012-12-20 14:06:53]

On 2012/12/20 12:26:21, ahe wrote:
> I think it would be simpler and cleaner if you passed in an instance of
inferrer
> when you need it.  As far as I can tell, there is only a few operations that
> need the inferrer.

Done.

[ahe, 2012-12-20 14:28:07]

LGTM