Collect call site information and use that for estimating parameter types

Collect call site information and use that for estimating parameter types

The propagated types at call sites for dynamic invocations are
collected. When a functions is compiled it is checked whether
there is type information available from call sites. If so the
function is compiled with the asumption that these types will
always be provided. If these expected types change during the
rest of the compilation process the generated code for the
function will be flushed and the function scheduled for
compilation again.

Note that the scheduling for being compiled again is not using
the re-compilation queue. The Re-compilation queue is used for
re-compilation functions when all functions have been compile
once and type information will not change.

R=floitsch@google.com, ahe@google.com

BUG=

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

[Søren Gjesse, 2012-08-06 14:03:01]

Corrected floitsch@google.com email address

[floitsch, 2012-08-06 15:01:06]

LGTM.

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

https://chromiumcodereview.appspot.com/10827181/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:363:
print(MessageKind.PLEASE_REPORT_THE_CRASH.message([buildId]));
why?

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

https://chromiumcodereview.appspot.com/10827181/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:288: List<HType>
optimisticParameterTypes(FunctionElement element) {
Rename. The name doesn't convey that the element is saved for recompilation if
the types change.
Alternatively provide a separate registration method:
registerForRecompilationOnTypeChange.

https://chromiumcodereview.appspot.com/10827181/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:302: if (foundCount == 1) {
&& found.hasTypeInformation.
Move this test up. No need to compute the signature if we don't have any types.

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

https://chromiumcodereview.appspot.com/10827181/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:177: // If there is an estimate of
the parameter types assume these types when
Optimistically use the parameter types that have been seen so far. The method
will be recompiled if the assumptions fail.

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

https://chromiumcodereview.appspot.com/10827181/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:1737: // through their selector?
Example?

[ahe, 2012-08-06 15:01:35]

I'll take a closer look tomorrow.

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

https://chromiumcodereview.appspot.com/10827181/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:468:
(MessageKind.PLEASE_REPORT_THE_CRASH.message([buildId]));
Bad edit?

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

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/co...
File lib/compiler/implementation/compiler.dart (left):

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/co...
lib/compiler/implementation/compiler.dart:363:
print(MessageKind.PLEASE_REPORT_THE_CRASH.message([buildId]));
On 2012/08/06 15:01:06, floitsch wrote:
> why?

Bad edit.

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/co...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/co...
lib/compiler/implementation/compiler.dart:288: List<HType>
optimisticParameterTypes(FunctionElement element) {
On 2012/08/06 15:01:06, floitsch wrote:
> Rename. The name doesn't convey that the element is saved for recompilation if
> the types change.
> Alternatively provide a separate registration method:
> registerForRecompilationOnTypeChange.

Renamed to optimisticParameterTypesWithRecompilationOnTypeChange. I know it is
long, but I did not like decoupling the registration as it should be bound to
retrieving the optimistic parameter types.

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/co...
lib/compiler/implementation/compiler.dart:302: if (foundCount == 1) {
On 2012/08/06 15:01:06, floitsch wrote:
> && found.hasTypeInformation.
> Move this test up. No need to compute the signature if we don't have any
types.

Done.

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/co...
lib/compiler/implementation/compiler.dart:468:
(MessageKind.PLEASE_REPORT_THE_CRASH.message([buildId]));
On 2012/08/06 15:01:35, ahe wrote:
> Bad edit?

Reverted.

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:177: // If there is an estimate of
the parameter types assume these types when
On 2012/08/06 15:01:06, floitsch wrote:
> Optimistically use the parameter types that have been seen so far. The method
> will be recompiled if the assumptions fail.

Much better, thanks.

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/10827181/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/codegen.dart:1737: // through their selector?
On 2012/08/06 15:01:06, floitsch wrote:
> Example?

I couldn't find one, so I removed the TODO.

[ahe, 2012-08-07 11:23:25]

Generally, this looks fine. However, I'm a little concerned about the level of
testing. I'll talk to you in person.

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:54: class InvocationInfo {
I don't think this class belongs in this file.

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:66: int parameterCount;
Please follow this sequence of members:

fields,
constructors/factories,
methods/accessors,

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:228: // Register a dynamic invocation
and collects the provided types for the
collects -> collect.

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:228: // Register a dynamic invocation
and collects the provided types for the
This should be a documentation comment, that is, use /** ... */, not //.

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:230: void
registerDynamicInvocation(HInvokeDynamicMethod node, Selector selector) {
It would be great if you could move this out of compiler.dart, but as we
discussed, perhaps in the next CL.

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:256: info.compiledFunctions.forEach(
info.compiledFunctions.forEach(compiler.enqueuer.codegen.eagerRecompile)

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:276: types == null;
This is a no-op.

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:288: List<HType>
optimisticParameterTypesWithRecompilationOnTypeChange(
Please document this method.

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
File lib/compiler/implementation/enqueue.dart (right):

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/enqueue.dart:91: void eagerRecompile(Element
element) {
How is this "eager"?

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1743:
world.registerDynamicInvocationOf(node.element);
I would prefer to incorporate backend.registerDynamicInvocation with
world.registerDynamicInvocationOf and world.registerDynamicInvocation.

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
File lib/compiler/implementation/universe.dart (right):

http://codereview.chromium.org/10827181/diff/5002/lib/compiler/implementation...
lib/compiler/implementation/universe.dart:357: toString() => '$kind on
$receiverType $argumentCount';
How about using this pattern:

toString() => 'TypedSelector($kind, $receiverType, $argumentCount)';

This makes it easier to figure out what is going on if we get a NoSuchMethod
exception.

http://codereview.chromium.org/10827181/diff/5002/tests/compiler/dart2js_opti...
File tests/compiler/dart2js_optimizer/call_site_types_1_test.dart (right):

http://codereview.chromium.org/10827181/diff/5002/tests/compiler/dart2js_opti...
tests/compiler/dart2js_optimizer/call_site_types_1_test.dart:5: // Test that
different types at call sites still handles parameters correct.
I would rather have a unit test. I have something you can use in
https://chromiumcodereview.appspot.com/10855020

http://codereview.chromium.org/10827181/diff/5002/tests/compiler/dart2js_opti...
File tests/compiler/dart2js_optimizer/dart2js_optimizer.status (right):

http://codereview.chromium.org/10827181/diff/5002/tests/compiler/dart2js_opti...
tests/compiler/dart2js_optimizer/dart2js_optimizer.status:1: # Copyright (c)
2012, the Dart project authors.  Please see the AUTHORS file
I think you're adding a new suite for this. Is that really necessary?

[Søren Gjesse, 2012-08-08 10:36:02]

Peter, PTAL

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
File lib/compiler/implementation/compiler.dart (right):

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/compiler.dart:54: class InvocationInfo {
On 2012/08/07 11:23:26, ahe wrote:
> I don't think this class belongs in this file.

I agree. I will move this and the JavaScriptBackend to a separate file in
another CL.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/compiler.dart:66: int parameterCount;
On 2012/08/07 11:23:26, ahe wrote:
> Please follow this sequence of members:
> 
> fields,
> constructors/factories,
> methods/accessors,

Done.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/compiler.dart:228: // Register a dynamic invocation
and collects the provided types for the
On 2012/08/07 11:23:26, ahe wrote:
> This should be a documentation comment, that is, use /** ... */, not //.

Done.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/compiler.dart:228: // Register a dynamic invocation
and collects the provided types for the
On 2012/08/07 11:23:26, ahe wrote:
> This should be a documentation comment, that is, use /** ... */, not //.

Done.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/compiler.dart:230: void
registerDynamicInvocation(HInvokeDynamicMethod node, Selector selector) {
On 2012/08/07 11:23:26, ahe wrote:
> It would be great if you could move this out of compiler.dart, but as we
> discussed, perhaps in the next CL.

Will do.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/compiler.dart:256: info.compiledFunctions.forEach(
On 2012/08/07 11:23:26, ahe wrote:
> info.compiledFunctions.forEach(compiler.enqueuer.codegen.eagerRecompile)

Done.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/compiler.dart:276: types == null;
On 2012/08/07 11:23:26, ahe wrote:
> This is a no-op.

Fixed.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/compiler.dart:288: List<HType>
optimisticParameterTypesWithRecompilationOnTypeChange(
On 2012/08/07 11:23:26, ahe wrote:
> Please document this method.

Done.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
File lib/compiler/implementation/enqueue.dart (right):

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/enqueue.dart:91: void eagerRecompile(Element
element) {
On 2012/08/07 11:23:26, ahe wrote:
> How is this "eager"?

It is eager in the sense that it is recompiled through the main enqueuer not the
re-compilation enqueuer.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/ssa/codegen.dart:1743:
world.registerDynamicInvocationOf(node.element);
On 2012/08/07 11:23:26, ahe wrote:
> I would prefer to incorporate backend.registerDynamicInvocation with
> world.registerDynamicInvocationOf and world.registerDynamicInvocation.

I see your point, but currently the world (Enqueuer) object has no access to the
JavaScript backend.

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
File lib/compiler/implementation/universe.dart (right):

http://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/impleme...
lib/compiler/implementation/universe.dart:357: toString() => '$kind on
$receiverType $argumentCount';
On 2012/08/07 11:23:26, ahe wrote:
> How about using this pattern:
> 
> toString() => 'TypedSelector($kind, $receiverType, $argumentCount)';
> 
> This makes it easier to figure out what is going on if we get a NoSuchMethod
> exception.

Done. Changed toString on Selector to:

toString() => 'Selector($kind, $argumentCount)';

http://chromiumcodereview.appspot.com/10827181/diff/5002/tests/compiler/dart2...
File tests/compiler/dart2js_optimizer/call_site_types_1_test.dart (right):

http://chromiumcodereview.appspot.com/10827181/diff/5002/tests/compiler/dart2...
tests/compiler/dart2js_optimizer/call_site_types_1_test.dart:5: // Test that
different types at call sites still handles parameters correct.
On 2012/08/07 11:23:26, ahe wrote:
> I would rather have a unit test. I have something you can use in
> https://chromiumcodereview.appspot.com/10855020

Done.

http://chromiumcodereview.appspot.com/10827181/diff/5002/tests/compiler/dart2...
File tests/compiler/dart2js_optimizer/dart2js_optimizer.status (right):

http://chromiumcodereview.appspot.com/10827181/diff/5002/tests/compiler/dart2...
tests/compiler/dart2js_optimizer/dart2js_optimizer.status:1: # Copyright (c)
2012, the Dart project authors.  Please see the AUTHORS file
On 2012/08/07 11:23:26, ahe wrote:
> I think you're adding a new suite for this. Is that really necessary?

No, changed to use dart2js unit test.

[ahe, 2012-08-08 13:00:27]

LGTM!

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

https://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:54: class InvocationInfo {
On 2012/08/08 10:36:02, Søren Gjesse wrote:
> On 2012/08/07 11:23:26, ahe wrote:
> > I don't think this class belongs in this file.
> 
> I agree. I will move this and the JavaScriptBackend to a separate file in
> another CL.

SGTM

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

https://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:91: void eagerRecompile(Element
element) {
On 2012/08/08 10:36:02, Søren Gjesse wrote:
> On 2012/08/07 11:23:26, ahe wrote:
> > How is this "eager"?
> 
> It is eager in the sense that it is recompiled through the main enqueuer not
the
> re-compilation enqueuer.

I find this name confusing. But I don't have a better suggestion.

https://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1743:
world.registerDynamicInvocationOf(node.element);
On 2012/08/08 10:36:02, Søren Gjesse wrote:
> On 2012/08/07 11:23:26, ahe wrote:
> > I would prefer to incorporate backend.registerDynamicInvocation with
> > world.registerDynamicInvocationOf and world.registerDynamicInvocation.
> 
> I see your point, but currently the world (Enqueuer) object has no access to
the
> JavaScript backend.

Inside the Enqueuer, you can access the backend using compiler.backend.

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

https://chromiumcodereview.appspot.com/10827181/diff/5002/lib/compiler/implem...
lib/compiler/implementation/universe.dart:357: toString() => '$kind on
$receiverType $argumentCount';
On 2012/08/08 10:36:02, Søren Gjesse wrote:
> On 2012/08/07 11:23:26, ahe wrote:
> > How about using this pattern:
> > 
> > toString() => 'TypedSelector($kind, $receiverType, $argumentCount)';
> > 
> > This makes it easier to figure out what is going on if we get a NoSuchMethod
> > exception.
> 
> Done. Changed toString on Selector to:
> 
> toString() => 'Selector($kind, $argumentCount)';

Nice!

https://chromiumcodereview.appspot.com/10827181/diff/10002/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10827181/diff/10002/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:290: * function. The types are
optimistic in the sense as thay are based on the
thay ->they

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

https://chromiumcodereview.appspot.com/10827181/diff/10002/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10827181/diff/10002/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:290: * function. The types are
optimistic in the sense as thay are based on the
On 2012/08/08 13:00:27, ahe wrote:
> thay ->they

Done.