Optimize getField by caching a closure generated from eval.

Optimize getField by caching a closure generated from eval.

R=iposva@google.com

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

[rmacnak, 2014-02-03 19:25:09]

Baseline    4960
Eval         499
Specialized  482

Smaller is better.


Eval
https://codereview.chromium.org/144383007/

Specialized
https://codereview.chromium.org/135913003/

[hausner, 2014-02-04 00:12:23]

I like this version much better. Adding more function types and specialized
parser entry points as you do in the other change is less desirable.

Both of your changes produce a lot of "waste" in the sense that they cached
functions hang on to a bunch of stuff that the GC can't collect. I'm not sure
which version is better in that respect.

Let's discuss with Ivan tomorrow.

[kasperl, 2014-02-04 07:37:22]

Thanks a lot for working on this, Ryan! I have a few tweaks that would make the
patterns Angular use even faster, but this is a fantastic starting point.

[rmacnak, 2014-02-05 01:08:33]

Use a non-identity hash map, and use the string behind the symbol as the key.
Improves from 499 to 299 (smaller is better).

Added todo to make the cache weak.

[kasperl, 2014-02-05 08:32:13]

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

https://codereview.chromium.org/144383007/diff/80001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:341: if (f == null) {
Consider splitting this method in two so it becomes more realistic to inline the
outer without the inner.

  InstanceMirror getField(Symbol memberName) {
    var closure = _getFieldClosures[_n(memberName)];
    return (closure == null) 
        ? _getFieldSlow(memberName)
        : reflect(closure(_reflectee));
  }

https://codereview.chromium.org/144383007/diff/80001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:342: var unwrapped = _n(memberName);
Cache _n(memberName) in a local?

https://codereview.chromium.org/144383007/diff/80001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:343: var at_pos = unwrapped.indexOf('@');
Follow Dart coding style: at_pos -> atPosition or atIndex.

https://codereview.chromium.org/144383007/diff/80001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:359: var f = _setFieldClosures[_n(memberName)];
Cache _n(memberName) in a local?

https://codereview.chromium.org/144383007/diff/80001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:362: var at_pos = unwrapped.indexOf('@');
Follow Dart coding style: at_pos -> atPosition or atIndex.

[kasperl, 2014-02-05 08:34:26]

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

https://codereview.chromium.org/144383007/diff/80001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:335: // TODO(16539): Make these weak.
I guess there's no need for complex weak handling here. You never hand out
references to these closures (you just call them) so you could just clear the
maps on GC.

[kasperl, 2014-02-05 09:05:39]

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

https://codereview.chromium.org/144383007/diff/80001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:342: var unwrapped = _n(memberName);
On 2014/02/05 08:32:13, kasperl wrote:
> Cache _n(memberName) in a local?

I guess what I meant was: Use unwrapped consistently below.

[rmacnak, 2014-02-05 18:53:41]

Apply Kasper's advice. Also avoid generating closures for one-off getField
calls. 248-255 => 183-197

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

https://codereview.chromium.org/144383007/diff/80001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:335: // TODO(16539): Make these weak.
On 2014/02/05 08:34:26, kasperl wrote:
> I guess there's no need for complex weak handling here. You never hand out
> references to these closures (you just call them) so you could just clear the
> maps on GC.

Soft would do as well. Though I wonder if this would still be a net win if there
are high levels of allocation and getField...

[rmacnak, 2014-02-05 22:13:16]

Maintain the call count on a per-selector basis. Only bump call count when the
invocation doesn't throw.

[Ivan Posva, 2014-02-05 22:23:44]

LGTMwC

-Ivan

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

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:1315: for (int i = 0; i < num_libraries; i++) {
If the private_key is null coming in, then you will not find it anywhere in this
list.

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:1322: }
With the current setup I would think that you can assert that you found the
library for a given key, correct?

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors_imp...
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors_imp...
runtime/lib/mirrors_impl.dart:348: if (callCount == 20) {
How about naming this arbitrary constant and pulling it out of the functions.

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors_imp...
runtime/lib/mirrors_impl.dart:376: return (f == null)
return (f == null) ?
      _getFieldSlow(unwrapped) : reflect(f(_reflectee));

[rmacnak, 2014-02-05 22:56:30]

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

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:1322: }
On 2014/02/05 22:23:44, Ivan Posva wrote:
> With the current setup I would think that you can assert that you found the
> library for a given key, correct?

Yes, added.

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors_imp...
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors_imp...
runtime/lib/mirrors_impl.dart:348: if (callCount == 20) {
On 2014/02/05 22:23:44, Ivan Posva wrote:
> How about naming this arbitrary constant and pulling it out of the functions.

Done.

https://codereview.chromium.org/144383007/diff/210001/runtime/lib/mirrors_imp...
runtime/lib/mirrors_impl.dart:376: return (f == null)
On 2014/02/05 22:23:44, Ivan Posva wrote:
> return (f == null) ?
>       _getFieldSlow(unwrapped) : reflect(f(_reflectee));

Done.

[rmacnak, 2014-02-05 23:03:51]

Committed patchset #8 manually as r32343 (presubmit successful).