Remove signature classes from the VM.

Remove signature classes from the VM.
They were used as the class of closure instances and as the type class of
function types.
All closure instances now have class _Closure and function types are represented
by a new class FunctionType extending AbstractType.
Fix issue 24567 and add regression test.

R=asiva@google.com, rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/7f57ebcfa187fbff0a3138423f7ead35632d0c2c

[regis, 2016-01-15 22:01:01]

regis@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com

[rmacnak, 2016-01-19 19:27:12]

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/mirrors_impl.da...
runtime/lib/mirrors_impl.dart:964: MethodMirror get callMethod {
return declarations[#call];

and delete last line from method_mirror_location_test

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/mirrors_impl.da...
runtime/lib/mirrors_impl.dart:1559: return _location(_reflectee);
This isn't right. It makes the location of every parameter be the position of
its function.

https://codereview.chromium.org/1584223006/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/graph_test.dart (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/graph_test.dart:74: const int fixedSizeListCid
= 64;  // TODO(rmacnak): Compute from kArrayCid.
Library corelib = isolate.libraries.singleWhere((lib) => lib.uri ==
'dart:core');
await corelib.load();
Class _List = corelib.classes.singleWhere((cls) =>
cls.vmName.startsWith('_List'));
int kArrayCid = _List.vmCid;
// startsWith to ignore the private mangling

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/profiler_test.cc
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/profiler_test.cc...
runtime/vm/profiler_test.cc:856: TEST_CASE(Profiler_ClassAllocation) {
ClosureAllocation

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/profiler_test.cc...
runtime/vm/profiler_test.cc:883: // Invoke "foo" which during compilation,
triggers a closure class allocation.
Doesn't allocate a class anymore.

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/profiler_test.cc...
runtime/vm/profiler_test.cc:907: // Invoke "bar" which during compilation,
triggers a closure class allocation.
Doesn't allocate a class anymore.

[siva, 2016-01-19 21:11:00]

LGTM with some comments.

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/function.cc#new...
runtime/lib/function.cc:58: zone, arguments->NativeArgAt(0));
Is it right to Fatal out here if the receiver is not a closure instead of
throwing an exception? i.e. use

GET_NATIVE_ARGUMENT(Closure, receiver, arguments->NativeArgAt(0));

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/raw_object_snaps...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/raw_object_snaps...
runtime/vm/raw_object_snapshot.cc:318: if (!defer_canonicalization &&
RawObject::IsCanonical(tags)) {
if (!defer_canonicalization && is_canonical) {

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/raw_object_snaps...
runtime/vm/raw_object_snapshot.cc:358: // fields inline and not as references.
The comment says all the objects need to be written inline and not as references
but the actual write is being called with kAsReference

[rmacnak, 2016-01-19 22:00:10]

lgtm w/c

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/mirrors_impl.da...
runtime/lib/mirrors_impl.dart:964: MethodMirror get callMethod {
On 2016/01/19 19:27:11, rmacnak wrote:
> return declarations[#call];
> 
> and delete last line from method_mirror_location_test

Nevermind, sorry. Keep your original change. I think my suggestion would not
work correctly with typedefs. I'll look at this later in a separate CL.

[regis, 2016-01-19 23:15:30]

Thank you both!

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/function.cc#new...
runtime/lib/function.cc:58: zone, arguments->NativeArgAt(0));
On 2016/01/19 21:11:00, siva wrote:
> Is it right to Fatal out here if the receiver is not a closure instead of
> throwing an exception? i.e. use
> 
> GET_NATIVE_ARGUMENT(Closure, receiver, arguments->NativeArgAt(0));

I think a fatal is OK, because the receiver should match (as in equals above and
clone below). Note that the previous version had an UNREACHABLE in case the
instance was not a closure.

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/mirrors_impl.da...
runtime/lib/mirrors_impl.dart:964: MethodMirror get callMethod {
On 2016/01/19 22:00:10, rmacnak wrote:
> On 2016/01/19 19:27:11, rmacnak wrote:
> > return declarations[#call];
> > 
> > and delete last line from method_mirror_location_test
> 
> Nevermind, sorry. Keep your original change. I think my suggestion would not
> work correctly with typedefs. I'll look at this later in a separate CL.

Acknowledged.

https://codereview.chromium.org/1584223006/diff/1/runtime/lib/mirrors_impl.da...
runtime/lib/mirrors_impl.dart:1559: return _location(_reflectee);
On 2016/01/19 19:27:11, rmacnak wrote:
> This isn't right. It makes the location of every parameter be the position of
> its function.

OK, I will leave it unimplemented, then.

https://codereview.chromium.org/1584223006/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/graph_test.dart (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/graph_test.dart:74: const int fixedSizeListCid
= 64;  // TODO(rmacnak): Compute from kArrayCid.
On 2016/01/19 19:27:11, rmacnak wrote:
> Library corelib = isolate.libraries.singleWhere((lib) => lib.uri ==
> 'dart:core');
> await corelib.load();
> Class _List = corelib.classes.singleWhere((cls) =>
> cls.vmName.startsWith('_List'));
> int kArrayCid = _List.vmCid;
> // startsWith to ignore the private mangling

Done. Thanks!

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/profiler_test.cc
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/profiler_test.cc...
runtime/vm/profiler_test.cc:856: TEST_CASE(Profiler_ClassAllocation) {
On 2016/01/19 19:27:12, rmacnak wrote:
> ClosureAllocation

Done.

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/profiler_test.cc...
runtime/vm/profiler_test.cc:883: // Invoke "foo" which during compilation,
triggers a closure class allocation.
On 2016/01/19 19:27:12, rmacnak wrote:
> Doesn't allocate a class anymore.

Removed "class".

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/profiler_test.cc...
runtime/vm/profiler_test.cc:907: // Invoke "bar" which during compilation,
triggers a closure class allocation.
On 2016/01/19 19:27:11, rmacnak wrote:
> Doesn't allocate a class anymore.

Removed "class".

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/raw_object_snaps...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/raw_object_snaps...
runtime/vm/raw_object_snapshot.cc:318: if (!defer_canonicalization &&
RawObject::IsCanonical(tags)) {
On 2016/01/19 21:11:00, siva wrote:
> if (!defer_canonicalization && is_canonical) {

Done here, in TypeArguments::ReadFrom, and in Type::ReadFrom,

https://codereview.chromium.org/1584223006/diff/1/runtime/vm/raw_object_snaps...
runtime/vm/raw_object_snapshot.cc:358: // fields inline and not as references.
On 2016/01/19 21:11:00, siva wrote:
> The comment says all the objects need to be written inline and not as
references
> but the actual write is being called with kAsReference

Removed the second sentence, here and in Type::WriteTo, from where it was
copy-pasted.

[regis, 2016-01-20 00:33:03]

Description was changed from

==========
Remove signature classes from the VM.
They were used as the class of closure instances and as the type class of
function types.
All closure instances now have class _Closure and function types are represented
by a new class FunctionType extending AbstractType.
Fix issue 24567 and add regression test.
==========

to

==========
Remove signature classes from the VM.
They were used as the class of closure instances and as the type class of
function types.
All closure instances now have class _Closure and function types are represented
by a new class FunctionType extending AbstractType.
Fix issue 24567 and add regression test.

R=asiva@google.com, rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7f57ebcfa187fbff0a3138423f7ead35632d0c2c
==========

[regis, 2016-01-20 00:33:04]

Committed patchset #3 (id:40001) manually as
7f57ebcfa187fbff0a3138423f7ead35632d0c2c (presubmit successful).