Make the ClassMirrors created through reflectClass() find their owners (libraries) lazily.

Make the ClassMirrors created through reflectClass() find their owners (libraries) lazily.

Remove an unused parameter that was threaded through a number of mirror creation functions.

R=asiva@google.com

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

[rmacnak, 2013-07-16 00:55:20]

Will help with improving reflectClass() performance.

[rmacnak, 2013-07-16 17:17:52]

Should improve performance of heavy users of reflectClass(), provided they do
not also use classMirror.owner or classMirror.qualifiedName.  Will wait for
benchmark before committing.

[rmacnak, 2013-07-16 17:23:40]

Adding Ivan.

[siva, 2013-07-16 21:06:45]

This change does make a significant improvement
for reflectClass
(before 1150500.0 us after 315714.2 us)

Their benchmark however uses a cache and doesn't call reflectClass as often, I
however noticed that with the cache turned on this change makes the benchmark
worse:
171076.9 us Vs 129375.0 us

So yes I think it is best to wait till I am able to check in the benchmark and
you can measure the improvements as you make each change.

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

https://codereview.chromium.org/19188004/diff/9001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1261: const MirrorReference& klass_ref =
why not call it klass_mirror instead of klass_ref

https://codereview.chromium.org/19188004/diff/9001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1270: Api::NewHandle(isolate, klass.library()))));
Please add a TODO on top stating that this API call will get replaced by direct
VM code (maybe use the same bug you have opened for that).

[rmacnak, 2013-07-17 20:15:58]

Base:                    133 ms
This CL:                 168 ms
This CL + caching off*:  324 ms
Base w/o reflectClass**: 127 ms

*This CL isn't as effective as caching because reflectClass() still involves
building the class's members, etc.

**The only use is basically reflectClass(type).simpleName, which can be replaced
with new Symbol(type.toString()). Even completely getting rid of reflectClass()
isn't that big an improvement. 

On 2013/07/16 21:06:45, siva wrote:
> This change does make a significant improvement
> for reflectClass
> (before 1150500.0 us after 315714.2 us)
> 
> Their benchmark however uses a cache and doesn't call reflectClass as often, I
> however noticed that with the cache turned on this change makes the benchmark
> worse:
> 171076.9 us Vs 129375.0 us
> 
> So yes I think it is best to wait till I am able to check in the benchmark and
> you can measure the improvements as you make each change.
> 
> https://codereview.chromium.org/19188004/diff/9001/runtime/lib/mirrors.cc
> File runtime/lib/mirrors.cc (right):
> 
>
https://codereview.chromium.org/19188004/diff/9001/runtime/lib/mirrors.cc#new...
> runtime/lib/mirrors.cc:1261: const MirrorReference& klass_ref =
> why not call it klass_mirror instead of klass_ref
> 
>
https://codereview.chromium.org/19188004/diff/9001/runtime/lib/mirrors.cc#new...
> runtime/lib/mirrors.cc:1270: Api::NewHandle(isolate, klass.library()))));
> Please add a TODO on top stating that this API call will get replaced by
direct
> VM code (maybe use the same bug you have opened for that).

[rmacnak, 2013-07-18 02:50:02]

Now that we've discovered the slowdown was because of a handle leak and fixed
it, I see a minor improvement from 120 ms to 110 ms.

https://chromiumcodereview.appspot.com/19188004/diff/23001/runtime/lib/mirror...
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/19188004/diff/23001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1194: DARTSCOPE(isolate);
Is this needed, or is Dart_EnterScope fine by itself?

[rmacnak, 2013-07-18 17:27:29]

https://chromiumcodereview.appspot.com/19188004/diff/23001/runtime/lib/mirror...
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/19188004/diff/23001/runtime/lib/mirror...
runtime/lib/mirrors.cc:1194: DARTSCOPE(isolate);
On 2013/07/18 02:50:02, Ryan Macnak wrote:
> Is this needed, or is Dart_EnterScope fine by itself?

It appears not. Without it, tests pass and performance is the same. The older
mirror natives that traffic in API and internal handles do not include it.

[siva, 2013-07-18 20:53:30]

lgtm

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

https://codereview.chromium.org/19188004/diff/29001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1199: return result.raw();
If you integrate with new changes from Micheal wouldn't this just be
return CreateLibaryMirror(Library::Handle(klass.library()));

[rmacnak, 2013-07-18 22:55:00]

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

https://codereview.chromium.org/19188004/diff/29001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1199: return result.raw();
On 2013/07/18 20:53:30, siva wrote:
> If you integrate with new changes from Micheal wouldn't this just be
> return CreateLibaryMirror(Library::Handle(klass.library()));

Right.

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

https://codereview.chromium.org/19188004/diff/29003/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:972: Instance& retvalue = Instance::Handle();
Merging with Michael's changes, we discovered it is important this handle (and
its cousins in CreateLibraryMirror and CreateMethodMirror) be created outside
the Dart_Enter/ExitScope.

[rmacnak, 2013-07-19 00:44:42]

Committed patchset #6 manually as r25193 (presubmit successful).