Invoke native methods directly

Invoke native methods directly

In DDC, if the receiver is typed and the target method simply forwards to a JS call, invoke that call directly instead of via a symbol.

This is a step toward #28307.

Paul: can you look the the analyzer / summary code change?

dart:html uses "native".  other libraries (e.g., typed_data) use "external" for more or less the same thing.  The analyzer element model doesn't provide a way to test for "native" methods, but it does for "external".  I map native to external here.  Not sure if it's better to treat them as separate (I believe native predated external).  Longer term, it may make sense to try to remove "native" in favor of "external".

R=brianwilkerson@google.com, jacobr@google.com, jmesserly@google.com, paulberry@google.com

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

[vsm, 2017-03-13 13:21:12]

Description was changed from

==========
Invoke native methods directly

This is a step toward #28307.  Removing the symbol wrappers is a
little trickier.
==========

to

==========
Invoke native methods directly

In DDC, if the receiver is typed and the target method simply forwards to a JS
call, invoke that call directly instead of via a symbol.

This is a step toward #28307.

Paul: can you look the the analyzer / summary code change?

dart:html uses "native".  other libraries (e.g., typed_data) use "external" for
more or less the same thing.  The analyzer element model doesn't provide a way
to test for "native" methods, but it does for "external".  I map native to
external here.  Not sure if it's better to treat them as separate (I believe
native predated external).  Longer term, it may make sense to try to remove
"native" in favor of "external".
==========

[vsm, 2017-03-13 13:21:12]

vsm@google.com changed reviewers:
+ jacobr@google.com, jmesserly@google.com, paulberry@google.com

[Jacob, 2017-03-13 15:51:00]

lgtm

[Paul Berry, 2017-03-13 16:32:20]

paulberry@google.com changed reviewers:
+ brianwilkerson@google.com

[Paul Berry, 2017-03-13 16:32:21]

Analyzer changes lgtm, but I would like to hear Brian's opinion.  Brian,
essentially the change makes any method declared with "native" be considered by
analyzer to be an external method.  Can you think of any reason not to do that?

[Brian Wilkerson, 2017-03-13 16:36:57]

lgtm

Native support is only for SDK use, so this shouldn't have any impact on users.

[Jennifer Messerly, 2017-03-13 17:20:11]

lgtm w/ some code factoring suggestions. In particular there's a big perf trap
due to analyzer API

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5666:
_extensionTypes.hasNativeSubtype(baseType) &&
you might want to fold this check into isForwardedNative member somehow? it will
check again that isNativeClass, and hasNativeSubtype here is basically just:

  bool hasNativeSubtype(DartType type) =>
      _isNativeInterface(type.element) || isNativeClass(type.element);

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5677: // Don't call
extension members that just forward to the underlying
this might be nicer as a doc comment?

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5684: if (type is
InterfaceType) {
this check could be moved to line 5665: `useExtension = baseType != null &&`

really we shouldn't get here if we don't have an InterfaceType; nothing else
would be a native class.

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5687: var member =
element.lookUpMethod(name, currentLibrary) ??
this is a pretty slow lookup. it'll linear scan the type hierarchy 3 times. we
may want to cache more info about native types.

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5694: member =
(member.isSynthetic && member is PropertyAccessorElement) ? member.variable :
member;
long line

[vsm, 2017-03-13 20:55:26]

thanks, ptal.

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5666:
_extensionTypes.hasNativeSubtype(baseType) &&
On 2017/03/13 17:20:10, Jennifer Messerly wrote:
> you might want to fold this check into isForwardedNative member somehow? it
will
> check again that isNativeClass, and hasNativeSubtype here is basically just:
> 
>   bool hasNativeSubtype(DartType type) =>
>       _isNativeInterface(type.element) || isNativeClass(type.element);
> 

Done.

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5677: // Don't call
extension members that just forward to the underlying
On 2017/03/13 17:20:10, Jennifer Messerly wrote:
> this might be nicer as a doc comment?

Done - see below.

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5684: if (type is
InterfaceType) {
On 2017/03/13 17:20:11, Jennifer Messerly wrote:
> this check could be moved to line 5665: `useExtension = baseType != null &&`
> 
> really we shouldn't get here if we don't have an InterfaceType; nothing else
> would be a native class.

Done.

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5687: var member =
element.lookUpMethod(name, currentLibrary) ??
On 2017/03/13 17:20:11, Jennifer Messerly wrote:
> this is a pretty slow lookup. it'll linear scan the type hierarchy 3 times. we
> may want to cache more info about native types.
> 

Done.

https://codereview.chromium.org/2748713002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5694: member =
(member.isSynthetic && member is PropertyAccessorElement) ? member.variable :
member;
On 2017/03/13 17:20:10, Jennifer Messerly wrote:
> long line

Done.

[Jennifer Messerly, 2017-03-13 21:21:33]

LGTM, really like the new code flow!

https://codereview.chromium.org/2748713002/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2748713002/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:5674: var _forwardingCache
= <Element, Map<String, ExecutableElement>>{};
micro-opt:

    var _forwardingCache = new HashMap<Element, Map<String,
ExecutableElement>>();

if you don't care about order, it saves some linked list overhead

[vsm, 2017-03-13 22:11:48]

Description was changed from

==========
Invoke native methods directly

In DDC, if the receiver is typed and the target method simply forwards to a JS
call, invoke that call directly instead of via a symbol.

This is a step toward #28307.

Paul: can you look the the analyzer / summary code change?

dart:html uses "native".  other libraries (e.g., typed_data) use "external" for
more or less the same thing.  The analyzer element model doesn't provide a way
to test for "native" methods, but it does for "external".  I map native to
external here.  Not sure if it's better to treat them as separate (I believe
native predated external).  Longer term, it may make sense to try to remove
"native" in favor of "external".
==========

to

==========
Invoke native methods directly

In DDC, if the receiver is typed and the target method simply forwards to a JS
call, invoke that call directly instead of via a symbol.

This is a step toward #28307.

Paul: can you look the the analyzer / summary code change?

dart:html uses "native".  other libraries (e.g., typed_data) use "external" for
more or less the same thing.  The analyzer element model doesn't provide a way
to test for "native" methods, but it does for "external".  I map native to
external here.  Not sure if it's better to treat them as separate (I believe
native predated external).  Longer term, it may make sense to try to remove
"native" in favor of "external".

R=brianwilkerson@google.com, jacobr@google.com, jmesserly@google.com,
paulberry@google.com

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

[vsm, 2017-03-13 22:11:49]

Committed patchset #2 (id:20001) manually as
f331ecea201156bd9f1e8e2584ba9d818d927971 (presubmit successful).