dart2js: mark FunctionElements as dependencies always

dart2js: mark FunctionElements as dependencies always

We were only marking FunctionElements as dependencies if they were
closurized class members. This prevented us from adding functions that
were top level.

BUG=
R=floitsch@google.com, sigurdm@google.com

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

[Harry Terkelsen, 2015-10-12 20:51:46]

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

[Harry Terkelsen, 2015-10-12 20:51:46]

I'm not sure why the condition is there in the first place... but the tests are
passing

[Siggi Cherem (dart-lang), 2015-10-12 22:53:28]

+sigurdm - who is probably more familiar with this than I am.

https://codereview.chromium.org/1395183004/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (left):

https://codereview.chromium.org/1395183004/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:345:
compiler.resolverWorld.closurizedMembers.contains(element)) {
mmm - would this pull in more than needed? Seems like in your test case though,
we were including B, but we were missing A, right? In that case, could it be
that the function dependency (foo -> B) is correct, but we are missing is the
dependency from a type to its super class (B -> A)?

https://codereview.chromium.org/1395183004/diff/1/tests/compiler/dart2js/defe...
File tests/compiler/dart2js/deferred_not_in_main_test.dart (right):

https://codereview.chromium.org/1395183004/diff/1/tests/compiler/dart2js/defe...
tests/compiler/dart2js/deferred_not_in_main_test.dart:103: print(foo is F);
Interesting that we need `A` in the main chunk mainly for RTI here. 
Eventually, it might be nice to recognize this, maybe by tracking what kind of
dependency we have. That would allow us in the future to keep the body of the
classes in the deferred library and only include RTI in the main unit if we can.

[sigurdm, 2015-10-13 07:39:06]

sigurdm@google.com changed reviewers:
+ sigurdm@google.com

[sigurdm, 2015-10-13 07:39:07]

LGTM

https://codereview.chromium.org/1395183004/diff/20001/pkg/compiler/lib/src/de...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/1395183004/diff/20001/pkg/compiler/lib/src/de...
pkg/compiler/lib/src/deferred_load.dart:268: // rti from types that are needed
for their members.
I think johnniwinther is working on this.

[floitsch, 2015-10-13 12:59:05]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2015-10-13 12:59:05]

LGTM.
Please follow up with some of our customers to see if this impacts size.

[Harry Terkelsen, 2015-10-13 17:27:34]

https://codereview.chromium.org/1395183004/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (left):

https://codereview.chromium.org/1395183004/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:345:
compiler.resolverWorld.closurizedMembers.contains(element)) {
On 2015/10/12 22:53:28, Siggi Cherem (dart-lang) wrote:
> mmm - would this pull in more than needed? Seems like in your test case
though,
> we were including B, but we were missing A, right? In that case, could it be
> that the function dependency (foo -> B) is correct, but we are missing is the
> dependency from a type to its super class (B -> A)?

The class to superclass dependency is handled here:
https://github.com/dart-lang/sdk/blob/master/pkg/compiler/lib/src/deferred_lo...

https://codereview.chromium.org/1395183004/diff/1/tests/compiler/dart2js/defe...
File tests/compiler/dart2js/deferred_not_in_main_test.dart (right):

https://codereview.chromium.org/1395183004/diff/1/tests/compiler/dart2js/defe...
tests/compiler/dart2js/deferred_not_in_main_test.dart:103: print(foo is F);
On 2015/10/12 22:53:28, Siggi Cherem (dart-lang) wrote:
> Interesting that we need `A` in the main chunk mainly for RTI here. 
> Eventually, it might be nice to recognize this, maybe by tracking what kind of
> dependency we have. That would allow us in the future to keep the body of the
> classes in the deferred library and only include RTI in the main unit if we
can.

agreed, added a TODO

[Harry Terkelsen, 2015-10-13 17:55:10]

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

[Siggi Cherem (dart-lang), 2015-10-13 20:16:29]

https://codereview.chromium.org/1395183004/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (left):

https://codereview.chromium.org/1395183004/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:345:
compiler.resolverWorld.closurizedMembers.contains(element)) {
On 2015/10/13 17:27:34, Harry Terkelsen wrote:
> On 2015/10/12 22:53:28, Siggi Cherem (dart-lang) wrote:
> > mmm - would this pull in more than needed? Seems like in your test case
> though,
> > we were including B, but we were missing A, right? In that case, could it be
> > that the function dependency (foo -> B) is correct, but we are missing is
the
> > dependency from a type to its super class (B -> A)?
> 
> The class to superclass dependency is handled here:
>
https://github.com/dart-lang/sdk/blob/master/pkg/compiler/lib/src/deferred_lo...

Thanks for the pointer. Sorry for the confusion - I was seeing something
strange, the out.js file before your fix included foo and B's declaration, but
it appears that B was added later and not here, which would explain that we
weren't treating B as an actual dependency and adding A as well.