Optimize needNoSuchMethodHandling computation

Optimize needNoSuchMethodHandling computation

This greatly improves the number of lookups needed to establish the relation.
For the two most prolific tests we have:

html/element_test: 44939 -> 6557
html/mirrors_js_typed_interop_test (mirrors+jsinterop!): 1638455 -> 171326

R=sigmund@google.com

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

Reverted: https://github.com/dart-lang/sdk/commit/5397c5427e9102a8ea7602174be2664d4c15bb8b
Committed: https://github.com/dart-lang/sdk/commit/b262be749e4f7e05b3cbed1e69425ca98aa597ee

[Johnni Winther, 2016-10-17 12:08:26]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2016-10-17 19:30:34]

lgtm, very cool!

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:1562: /// If [stopAtSuperclass], no
member from [stopAtSuperclass] or its
If [stopAtSuperclass]... is specified?

ALternatively, "this recursively visits superclasses until the member is found
or [stopAt] is reached"?

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:1564: Element lookupByName(Name
memberName, {ClassElement stopAtSuperclass});
nit: rename to just stopAt?

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:782: return IterationStep.STOP;
should we also check that subclass is not abstract?

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:797: for (ClassHierarchyNode subtypeNode in
classSet.subtypeNodes) {
remind me - do subtype nodes include just the immediate set of subtypes or the
recursive set? (do we need to use needsNoSuchMethod below, instead of
subclassesNeedNoSuchMethod)

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:798: if
(subclassesNeedNoSuchMethod(subtypeNode)) return true;
Q: are we guaranteed that subtypes here can't be subclasses too? If not, should
we add an extra optinoal arg here to pass "node" as a stopAtSuperclass in line
768?

https://codereview.chromium.org/2428543002/diff/1/tests/compiler/dart2js/need...
File tests/compiler/dart2js/needs_no_such_method_test.dart (right):

https://codereview.chromium.org/2428543002/diff/1/tests/compiler/dart2js/need...
tests/compiler/dart2js/needs_no_such_method_test.dart:174: check(superclass,
ClassQuery.EXACT, bar, false);
consider updating the description of the CL to indicate that this is also fixing
this previously incorrect result

[Johnni Winther, 2016-10-18 08:13:51]

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:1562: /// If [stopAtSuperclass], no
member from [stopAtSuperclass] or its
On 2016/10/17 19:30:33, Siggi Cherem (dart-lang) wrote:
> If [stopAtSuperclass]... is specified?
> 
> ALternatively, "this recursively visits superclasses until the member is found
> or [stopAt] is reached"?

Done.

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:1564: Element lookupByName(Name
memberName, {ClassElement stopAtSuperclass});
On 2016/10/17 19:30:33, Siggi Cherem (dart-lang) wrote:
> nit: rename to just stopAt?

Done.

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/world....
File pkg/compiler/lib/src/world.dart (right):

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:782: return IterationStep.STOP;
On 2016/10/17 19:30:34, Siggi Cherem (dart-lang) wrote:
> should we also check that subclass is not abstract?

Wasn't needed above either (now that type inference behaves wrt. super calls)

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:797: for (ClassHierarchyNode subtypeNode in
classSet.subtypeNodes) {
On 2016/10/17 19:30:34, Siggi Cherem (dart-lang) wrote:
> remind me - do subtype nodes include just the immediate set of subtypes or the
> recursive set? (do we need to use needsNoSuchMethod below, instead of
> subclassesNeedNoSuchMethod)

It contains the roots for subclass trees that implement [base]. For instance

class A {}
class B implements A {}
class C extends B {}
class D implements C {}

for A the subtypeNodes will have roots B and D. C is in the subclass tree of B.

https://codereview.chromium.org/2428543002/diff/1/pkg/compiler/lib/src/world....
pkg/compiler/lib/src/world.dart:798: if
(subclassesNeedNoSuchMethod(subtypeNode)) return true;
On 2016/10/17 19:30:34, Siggi Cherem (dart-lang) wrote:
> Q: are we guaranteed that subtypes here can't be subclasses too? If not,
should
> we add an extra optinoal arg here to pass "node" as a stopAtSuperclass in line
> 768?

Yes. The are not subclasses of [base] (then they would be in [node]). They can,
though, extend superclasses of [base] and thus we still have some potential
repetition. This happens in two ways:

class A {}
class B extends A {}
class C extends B {}
class D extends B {}

when checking D the stopAt class is A (the root of the subclass tree) instead of
B.

class A {}
class B extends A {}
class C extends A implements B {}

when checking the subtypes of B we will search A (and Object) for both B and C.

We could add a set of stop classes but I'm not sure it's worthwhile.

[Johnni Winther, 2016-10-18 08:52:32]

Description was changed from

==========
Optimize needNoSuchMethodHandling computation

This greatly improves the number of lookups needed to establish the relation.
For the two most prolific tests we have:

html/element_test: 44939 -> 6557
html/mirrors_js_typed_interop_test (mirrors+jsinterop!): 1638455 -> 171326
==========

to

==========
Optimize needNoSuchMethodHandling computation

This greatly improves the number of lookups needed to establish the relation.
For the two most prolific tests we have:

html/element_test: 44939 -> 6557
html/mirrors_js_typed_interop_test (mirrors+jsinterop!): 1638455 -> 171326

R=sigmund@google.com

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

[Johnni Winther, 2016-10-18 08:52:34]

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

[Johnni Winther, 2016-10-18 14:01:14]

Description was changed from

==========
Optimize needNoSuchMethodHandling computation

This greatly improves the number of lookups needed to establish the relation.
For the two most prolific tests we have:

html/element_test: 44939 -> 6557
html/mirrors_js_typed_interop_test (mirrors+jsinterop!): 1638455 -> 171326

R=sigmund@google.com

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

to

==========
Optimize needNoSuchMethodHandling computation

This greatly improves the number of lookups needed to establish the relation.
For the two most prolific tests we have:

html/element_test: 44939 -> 6557
html/mirrors_js_typed_interop_test (mirrors+jsinterop!): 1638455 -> 171326

R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/5397c5427e9102a8ea7602174be2664d4c15bb8b
==========

[Johnni Winther, 2016-10-18 14:06:10]

Description was changed from

==========
Optimize needNoSuchMethodHandling computation

This greatly improves the number of lookups needed to establish the relation.
For the two most prolific tests we have:

html/element_test: 44939 -> 6557
html/mirrors_js_typed_interop_test (mirrors+jsinterop!): 1638455 -> 171326

R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/5397c5427e9102a8ea7602174be2664d4c15bb8b
==========

to

==========
Optimize needNoSuchMethodHandling computation

This greatly improves the number of lookups needed to establish the relation.
For the two most prolific tests we have:

html/element_test: 44939 -> 6557
html/mirrors_js_typed_interop_test (mirrors+jsinterop!): 1638455 -> 171326

R=sigmund@google.com

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

Reverted:
https://github.com/dart-lang/sdk/commit/5397c5427e9102a8ea7602174be2664d4c15bb8b
Committed:
https://github.com/dart-lang/sdk/commit/b262be749e4f7e05b3cbed1e69425ca98aa597ee
==========

[Johnni Winther, 2016-10-18 14:06:11]

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