Make ClassMirror.members internal-native and lazy.

Make ClassMirror.members internal-native and lazy.

R=asiva@google.com

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

[rmacnak, 2013-07-23 01:15:37]

Starting to improve currentMirrorSystem(), approx. 195 -> 104 ms.

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1311: DEFINE_NATIVE_ENTRY(ClassMirror_members, 2) {
I intend to adapt this to be shared with LibraryMirror.

[siva, 2013-07-23 21:14:39]

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1320: // not wind up as the reflectees of ClassMirrors.
One way to avoid this could be to store empty_array() as fields for the dynamic
type.

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1327: ASSERT(!functions.IsNull());
Ditto about storing empty_array() for functions.

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1332: GrowableObjectArray::New(num_fields +
num_functions));
Since you now the exact number of fields and functions can this be a plain Array
instead of a GrowableObjectArray ?

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

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:595: native "ClassMirror_members";
The indentation here doesn't seem to match up with the lines above.

[rmacnak, 2013-07-23 22:57:34]

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1320: // not wind up as the reflectees of ClassMirrors.
On 2013/07/23 21:14:39, siva wrote:
> One way to avoid this could be to store empty_array() as fields for the
dynamic
> type.

Hm. This irregularity also affects LookupField/LookupFunction. I tried adding
null_class().InitEmptyFields() in Object::Init, by which point the empty array
should be available, but this somehow ruins snapshot generation, even with a
clean build.

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1332: GrowableObjectArray::New(num_fields +
num_functions));
On 2013/07/23 21:14:39, siva wrote:
> Since you now the exact number of fields and functions can this be a plain
Array
> instead of a GrowableObjectArray ?

We know an upper bound. Some of the functions are synthetic and won't be added.
Hence a growable array with a capacity hint.

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

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:595: native "ClassMirror_members";
On 2013/07/23 21:14:39, siva wrote:
> The indentation here doesn't seem to match up with the lines above.

Fixed.

[siva, 2013-07-23 23:37:07]

lgtm

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1320: // not wind up as the reflectees of ClassMirrors.
Need to debug this.

On 2013/07/23 22:57:34, Ryan Macnak wrote:
> On 2013/07/23 21:14:39, siva wrote:
> > One way to avoid this could be to store empty_array() as fields for the
> dynamic
> > type.
> 
> Hm. This irregularity also affects LookupField/LookupFunction. I tried adding
> null_class().InitEmptyFields() in Object::Init, by which point the empty array
> should be available, but this somehow ruins snapshot generation, even with a
> clean build.

https://codereview.chromium.org/19496003/diff/8001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1332: GrowableObjectArray::New(num_fields +
num_functions));
True but the GrowableArray is going to allocate a backing array of the specified
capacity hint. A growable array in general has a higher memory footprint than a
regular array. Access (set/get) has an extra indirection. It is really useful
only if it actually grows.

On 2013/07/23 22:57:34, Ryan Macnak wrote:
> On 2013/07/23 21:14:39, siva wrote:
> > Since you now the exact number of fields and functions can this be a plain
> Array
> > instead of a GrowableObjectArray ?
> 
> We know an upper bound. Some of the functions are synthetic and won't be
added.
> Hence a growable array with a capacity hint.

[rmacnak, 2013-07-23 23:54:20]

Committed patchset #2 manually as r25384 (presubmit successful).

[ahe, 2013-07-24 10:11:53]

DBC

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

https://codereview.chromium.org/19496003/diff/14001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:23: Map<Symbol, dynamic> result = new Map();
I think it is more important to have the type on the right-hand side. This type
is reified and used in checked mode.

https://codereview.chromium.org/19496003/diff/14001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:638: Map<Symbol, Mirror> get members => const {};
Add type arguments.