Support fast noSuchMethod dispatch for any number of arguments.

Support fast noSuchMethod dispatch for any number of arguments.

Until now, the fast dispatch was only available with getters and
zero argument methods.

This CL generalizes the approach to any number of arguments. The
dispatcher functions that are auto-generated by the compiler
are not attached to the class anymore. Instead the dispatcher
is only stored in the IC data of each call site. Each class
contains a cache of dispatcher function that map
(name, arguments descriptor) => dispatcher.

This also fixes a bug where the VM report a wrong error message
when throwing a NoSuchMethodError.

BUG=https://code.google.com/p/dart/issues/detail?id=11528
BUG=https://code.google.com/p/dart/issues/detail?id=11223
TEST=tests/language/no_such_method_dispatcher_test.dart
R=srdjan@google.com

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

[Florian Schneider, 2013-07-09 14:53:09]

Added per-class cache of dispatcher function. Please review.

[srdjan, 2013-07-09 15:20:27]

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1222: if (ic_data.num_args_tested() == 1) {
Please add comments when can we expect num_args_tested to be 1 or > 1
respectively.

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1769: if (name.IsNull()) break;  // Reached last entry.
WHy do you terminate on name being null AND on cache.Length? Why not only on
length?

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1776: break;
I believe this is simpler:
if (desc.raw() == args_desc.raw()) {
  dispatcher = ..
  ASSERT(..);
  break;
}

[srdjan, 2013-07-09 15:35:40]

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1782: intptr_t new_len = cache.Length() == 0 ? kEntrySize :
cache.Length() * 2;
Is it necessary to grow that aggressively? I would imagine that growing just by
1 should be enough as adding new noSuchMethods should be very rare? Opinions?
Does it matter?

[srdjan, 2013-07-09 16:26:50]

LGTM

[Florian Schneider, 2013-07-10 09:05:59]

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1222: if (ic_data.num_args_tested() == 1) {
On 2013/07/09 15:20:28, srdjan wrote:
> Please add comments when can we expect num_args_tested to be 1 or > 1
> respectively.

Done.

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1769: if (name.IsNull()) break;  // Reached last entry.
On 2013/07/09 15:20:28, srdjan wrote:
> WHy do you terminate on name being null AND on cache.Length? Why not only on
> length?

When growing by more than 1 element, the cache array can be larger than the
number of elements.

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1776: break;
On 2013/07/09 15:20:28, srdjan wrote:
> I believe this is simpler:
> if (desc.raw() == args_desc.raw()) {
>   dispatcher = ..
>   ASSERT(..);
>   break;
> }

Done.

https://codereview.chromium.org/18097004/diff/24001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1782: intptr_t new_len = cache.Length() == 0 ? kEntrySize :
cache.Length() * 2;
On 2013/07/09 15:35:40, srdjan wrote:
> Is it necessary to grow that aggressively? I would imagine that growing just
by
> 1 should be enough as adding new noSuchMethods should be very rare? Opinions?
> Does it matter?

I don't think it matters, For the common case of few noSuchMethod dispatchers,
doubling from 1 to 2, 4, 8 etc. is reasonable and also avoids allocating a new
backing store on each addition. I'll leave it as is for now, but if it turns out
to be a problem, we can change it.

[Florian Schneider, 2013-07-10 10:10:38]

Committed patchset #5 manually as r24876 (presubmit successful).

[ahe, 2013-07-20 17:24:26]

https://codereview.chromium.org/18097004/diff/33001/tests/language/no_such_me...
File tests/language/no_such_method_dispatcher_test.dart (right):

https://codereview.chromium.org/18097004/diff/33001/tests/language/no_such_me...
tests/language/no_such_method_dispatcher_test.dart:26:
Expect.isTrue(e.toString().indexOf("method not found") != -1);
I'm working on improving dart2js' handling of NoSuchMethod errors.

Based on feedback from Lars, the error message has been changed to for example:

  NoSuchMethodError: Cannot call "bar$0" on "5" (Object 5 has no method 'bar$0')

The latter part is the message from the JavaScript VM, and the first part
represents what we have extracted from that message.

However, this varies a lot depending on how much information the browser
provides.  For example, if we cannot identify the method name, it could just be:

  NoSuchMethodError: Browser-specific error message.

Looking at the VM implementation of NoSuchMethodError.toString, I can see that
this particular message only occur in certain situations, and this might be
important for this test.

Can you elaborate on why this test is expecting this particular message?