Better tracking of provided types at call sites

Better tracking of provided types at call sites

The tracking of provided types at call sites now take both the
type of named arguments from selectors and the type of the
defualt values for optional paramters into account.

To know the type of the default values for optional parameters
the constant expressions for the default values are always
compiled before the function.

In order to check whether a new selector invalidates the current
provided types for a function the default values for the optional
parameters needs to be stored with all registered optimized
functions.

R=kasperl@google.com

BUG=

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

[kasperl, 2012-09-04 09:18:45]

Looks pretty good but I thing you can simplify it a bit. Comments:

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

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:50: void insert(int index,
SourceString name, HType type) {
Insert makes it sound like you're growing the list -- maybe change to update?

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:95: node.inputs.length - 1,
selector.namedArguments);
Maybe cache node.inputs.length - 1 in a local to make this more readable?

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:165: HTypeList
unionWithOptionalParameters(
This is fairly complicated. Can you add unit tests for this function or would
that be too painful?

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:186: // If some optional
parameters where passed positional these have
positional -> positionally

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:186: // If some optional
parameters where passed positional these have
where -> were

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:188: int
positionalArgumentCount = selector.positionalArgumentCount;
This local isn't used.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:193: for (int i = 0; i <
namedArguments.length; i++) {
Could you use namedArguments.indexOf here? Maybe you don't even have to check
for namedArgumentCount > 0 and you can just do something like:

   if (hasNamedArguments &&
       selector.namedArguments.indexOf(element.name) >= 0)

Maybe you wouldn't need the break and the type === null check in that case (an
else would do).

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:201: for (int i = 0; i <
defaultValueTypes.length; i++) {
Maybe add indexOf to OptionalParameterTypes?

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:203: type =
defaultValueTypes.type(i);
If you end up not going for indexOf, you should probably add a break here.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:309: assert(newTypes.length
== oldTypes.length);
This is great!

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:339: if (element.isGetter())
return HTypeList.ALL_UNKNOWN;
Add a comment that explains why getters should lead to ALL_UNKNOWN.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:348: signature,
Indentation is slightly off.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:354: return
!found.allUnknown;
This should be shareable now. Something like:

   found = (found === null) ? types : found.union(types);
   return !found.allUnknown;

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:621: HTypeList
parameterTypes,
I wonder if we should have an abstraction over all the parameter types -- and
not have two separate parts?

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

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/universe.dart:325: toString() => 'Selector($kind,
$name, $argumentCount $namedArgumentCount)';
Maybe this needs better formatting? Maybe something like:

  "...arity=$argumentCount/$namedArgumentCount..."

https://chromiumcodereview.appspot.com/10908068/diff/1/tests/compiler/dart2js...
File tests/compiler/dart2js/call_site_type_inferer_test.dart (right):

https://chromiumcodereview.appspot.com/10908068/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/call_site_type_inferer_test.dart:162: //new A().x(1, p3:
"x");
Hmm.

[Søren Gjesse, 2012-09-04 14:55:07]

PTAL

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

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:50: void insert(int index,
SourceString name, HType type) {
On 2012/09/04 09:18:45, kasperl wrote:
> Insert makes it sound like you're growing the list -- maybe change to update?

Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:95: node.inputs.length - 1,
selector.namedArguments);
On 2012/09/04 09:18:45, kasperl wrote:
> Maybe cache node.inputs.length - 1 in a local to make this more readable?

Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:165: HTypeList
unionWithOptionalParameters(
On 2012/09/04 09:18:45, kasperl wrote:
> This is fairly complicated. Can you add unit tests for this function or would
> that be too painful?

I tried to do it in tests/compiler/dart2js/backend_htype_list_test.dart.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:186: // If some optional
parameters where passed positional these have
On 2012/09/04 09:18:45, kasperl wrote:
> positional -> positionally

Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:186: // If some optional
parameters where passed positional these have
On 2012/09/04 09:18:45, kasperl wrote:
> where -> were

Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:188: int
positionalArgumentCount = selector.positionalArgumentCount;
On 2012/09/04 09:18:45, kasperl wrote:
> This local isn't used.

Removed.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:193: for (int i = 0; i <
namedArguments.length; i++) {
On 2012/09/04 09:18:45, kasperl wrote:
> Could you use namedArguments.indexOf here? Maybe you don't even have to check
> for namedArgumentCount > 0 and you can just do something like:
> 
>    if (hasNamedArguments &&
>        selector.namedArguments.indexOf(element.name) >= 0)
> 
> Maybe you wouldn't need the break and the type === null check in that case (an
> else would do).

Nice simplification. Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:201: for (int i = 0; i <
defaultValueTypes.length; i++) {
On 2012/09/04 09:18:45, kasperl wrote:
> Maybe add indexOf to OptionalParameterTypes?

Added typeFor(SourceString name) instead.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:203: type =
defaultValueTypes.type(i);
On 2012/09/04 09:18:45, kasperl wrote:
> If you end up not going for indexOf, you should probably add a break here.

See above.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:309: assert(newTypes.length
== oldTypes.length);
On 2012/09/04 09:18:45, kasperl wrote:
> This is great!

Thanks.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:339: if (element.isGetter())
return HTypeList.ALL_UNKNOWN;
On 2012/09/04 09:18:45, kasperl wrote:
> Add a comment that explains why getters should lead to ALL_UNKNOWN.

Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:348: signature,
On 2012/09/04 09:18:45, kasperl wrote:
> Indentation is slightly off.

Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:348: signature,
On 2012/09/04 09:18:45, kasperl wrote:
> Indentation is slightly off.

Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:354: return
!found.allUnknown;
On 2012/09/04 09:18:45, kasperl wrote:
> This should be shareable now. Something like:
> 
>    found = (found === null) ? types : found.union(types);
>    return !found.allUnknown;

Done.

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/js_backend/backend.dart:621: HTypeList
parameterTypes,
On 2012/09/04 09:18:45, kasperl wrote:
> I wonder if we should have an abstraction over all the parameter types -- and
> not have two separate parts?

That might be - I will look at that as a separate change.

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

https://chromiumcodereview.appspot.com/10908068/diff/1/lib/compiler/implement...
lib/compiler/implementation/universe.dart:325: toString() => 'Selector($kind,
$name, $argumentCount $namedArgumentCount)';
On 2012/09/04 09:18:45, kasperl wrote:
> Maybe this needs better formatting? Maybe something like:
> 
>   "...arity=$argumentCount/$namedArgumentCount..."

Included the actual names of the named arguments in the toString result.

https://chromiumcodereview.appspot.com/10908068/diff/1/tests/compiler/dart2js...
File tests/compiler/dart2js/call_site_type_inferer_test.dart (right):

https://chromiumcodereview.appspot.com/10908068/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/call_site_type_inferer_test.dart:162: //new A().x(1, p3:
"x");
On 2012/09/04 09:18:45, kasperl wrote:
> Hmm.

Removed.

[kasperl, 2012-09-05 07:06:29]

LGTM. Very happy to see such great tests!

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
File lib/compiler/implementation/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/js_backend/backend.dart:191: int next =
selector.positionalArgumentCount;  // Next parameter of interest.
Maybe add a newline and a new comment that explains how you run through the
optional parameters (to better break up the fairly long method)?

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/js_backend/backend.dart:337: // Getters have no
parameters.
Maybe move this up before the slightly weird !isMember check?

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

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:326: StringBuffer result = new
StringBuffer();
Don't do the allocation of this if namedArgumentCount == 0?

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:328: result.add('[');
How about not adding [ and ] and just returning "[$result]"?

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:410: String toString() {
How about getting rid of this and just use the one from the superclass -- and
add printing of the type up there if it isn't null?

https://chromiumcodereview.appspot.com/10908068/diff/6001/tests/compiler/dart...
File tests/compiler/dart2js/backend_htype_list_test.dart (right):

https://chromiumcodereview.appspot.com/10908068/diff/6001/tests/compiler/dart...
tests/compiler/dart2js/backend_htype_list_test.dart:51: HTypeList
createHTypeListWithNamed(List<HType> types, List<SourceString> namedArguments) {
Long lines.

[Søren Gjesse, 2012-09-05 08:02:59]

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
File lib/compiler/implementation/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/js_backend/backend.dart:191: int next =
selector.positionalArgumentCount;  // Next parameter of interest.
On 2012/09/05 07:06:29, kasperl wrote:
> Maybe add a newline and a new comment that explains how you run through the
> optional parameters (to better break up the fairly long method)?

Done.

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/js_backend/backend.dart:337: // Getters have no
parameters.
On 2012/09/05 07:06:29, kasperl wrote:
> Maybe move this up before the slightly weird !isMember check?

Done.

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

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:326: StringBuffer result = new
StringBuffer();
On 2012/09/05 07:06:29, kasperl wrote:
> Don't do the allocation of this if namedArgumentCount == 0?

Done.

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:328: result.add('[');
On 2012/09/05 07:06:29, kasperl wrote:
> How about not adding [ and ] and just returning "[$result]"?

Done.

https://chromiumcodereview.appspot.com/10908068/diff/6001/lib/compiler/implem...
lib/compiler/implementation/universe.dart:410: String toString() {
On 2012/09/05 07:06:29, kasperl wrote:
> How about getting rid of this and just use the one from the superclass -- and
> add printing of the type up there if it isn't null?

Good point, done.

https://chromiumcodereview.appspot.com/10908068/diff/6001/tests/compiler/dart...
File tests/compiler/dart2js/backend_htype_list_test.dart (right):

https://chromiumcodereview.appspot.com/10908068/diff/6001/tests/compiler/dart...
tests/compiler/dart2js/backend_htype_list_test.dart:51: HTypeList
createHTypeListWithNamed(List<HType> types, List<SourceString> namedArguments) {
On 2012/09/05 07:06:29, kasperl wrote:
> Long lines.

Done.