First step for modular output in fasta.

First step for modular output in fasta.

Some details:
 - Adds the tree-shaker logic and unit tests for it,

   Note: I'm still not clear on what degree of tree-shaking we can get away
   with. For now I have a mode to delete anything that is not seen explicitly.
   This passes verification for all of our tests so far, but breaks both the
   mixin and the async/await transformer in some scenarios.

 - Writes a prototype of kernelForBuildUnit (although no automated tests for it
   yet).

 - Moves a few more pieces to ProcessedOptions

R=scheglov@google.com

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

[Siggi Cherem (dart-lang), 2017-05-18 04:50:34]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2017-05-18 04:51:12]

sigmund@google.com changed reviewers:
+ ahe@google.com, paulberry@google.com, scheglov@google.com

[Siggi Cherem (dart-lang), 2017-05-18 04:51:13]

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/kerne...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:48: var pOptions = new
ProcessedOptions(options);
Using processedOptions for a variable felt just very long :) - other ideas?

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:160: // in a specific order.
Konstantin - I want to sync up with you on this a bit. It is highly related to
what you had to do for the incremental generator, so I'd like to pick your brain
on it

[scheglov, 2017-05-18 15:54:08]

LGTM

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/kerne...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:160: // in a specific order.
On 2017/05/18 04:51:13, Siggi Cherem (dart-lang) wrote:
> Konstantin - I want to sync up with you on this a bit. It is highly related to
> what you had to do for the incremental generator, so I'd like to pick your
brain
> on it

There might be a chicken and egg problem here - in order to use dill files to
determine the right order we would need to load them to use import/export
directives (when and if they have import/export information). IKG get around
this by using Dart files directly to compute the graph first. We could have
additional files with graph information or make dill files lazily loaded so that
we could load only unresolved imports/exports first.

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart (right):

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:211:
_markClass(coreTypes.iterableClass);
These classes are already included in _markRequired().

[Siggi Cherem (dart-lang), 2017-05-19 04:28:33]

Patchset #2 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-19 04:37:33]

PTAL - after our conversations this morning I did a few updates:

 - I split the recursive visitor out, so that all of the tree-shaking logic is
orthogonal to how we mark the roots of the program that need to be preserved. In
the future we can replace that with something based on the builders.

There are a couple of TODOs highlighting this. I tried looking into how to do
this, but I didn't figure out how that quickly, so I'd like to work on that as a
separate change later on.

- async is now gone from outlines, so the hack related to that is gone.

- the issue I thought I had with the mixin transformer was a misunderstanding. I
actually had a crash because of something else: implicit super calls to the
default constructor were not modeled in the IR so I had to explicitly mark those
constructors. The new patch does this and doesn't retain any other members. It
is just  the minimum needed to avoid dangling references.

A mixin transformer will not see the members it needs to copy, but I believe
that's no longer our goal, correct? I don't at this time, but let me know if you
believe that's a reason to run the tree-shaker after we run the transformers.

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart (right):

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:211:
_markClass(coreTypes.iterableClass);
On 2017/05/18 15:54:08, scheglov wrote:
> These classes are already included in _markRequired().

good point. I removed them from here. Also - now that async is not in outlines,
we never hit this case.

[scheglov, 2017-05-19 17:19:05]

lgtm

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart (right):

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:130: if
(q.contains("Converter")) print(q);
Debug code?

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:251: if
(s.contains("Converter")) print(s);
Should we remove this debug code?

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:410: // the
rootMarker is meant to detect this.
What is "rootMarker"?

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:709:
trimDependenciesHelper(Program program, List<Library> extraLibraries) {
1. Helper is usually an object, YMMV though.
2. Could you add a comment? It is not obvious what is "extractLibraries" and how
it is used.

[Siggi Cherem (dart-lang), 2017-05-19 19:41:16]

Patchset #3 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-19 19:41:22]

Patchset #3 (id:100001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-19 19:43:31]

thanks Konstantin!

Peter or Paul -  I'd be happy to come back to address any additional comments
you may have afterwards.

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart (right):

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:130: if
(q.contains("Converter")) print(q);
On 2017/05/19 17:19:05, scheglov wrote:
> Debug code?

ha, yes, thank you.

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:251: if
(s.contains("Converter")) print(s);
On 2017/05/19 17:19:05, scheglov wrote:
> Should we remove this debug code?

yes, thank you

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:410: // the
rootMarker is meant to detect this.
On 2017/05/19 17:19:05, scheglov wrote:
> What is "rootMarker"?

fixed - should be [RootsMarker] the class defined above.

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2893493004/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:709:
trimDependenciesHelper(Program program, List<Library> extraLibraries) {
On 2017/05/19 17:19:05, scheglov wrote:
> 1. Helper is usually an object, YMMV though.

indeed - I very commonly use "helper" for helper functions :), but I renamed it
here.

> 2. Could you add a comment? It is not obvious what is "extractLibraries" and
how
> it is used.

done. Also renamed extraLibraries, the name was carried from before I refactored
this code, I agree it didn't make sense here.

[Siggi Cherem (dart-lang), 2017-05-19 20:59:20]

Description was changed from

==========
First step for modular output in fasta.

Some details:
 - Adds the tree-shaker logic and unit tests for it,

   Note: I'm still not clear on what degree of tree-shaking we can get away
   with. For now I have a mode to delete anything that is not seen explicitly.
   This passes verification for all of our tests so far, but breaks both the
   mixin and the async/await transformer in some scenarios.

 - Writes a prototype of kernelForBuildUnit (although no automated tests for it
   yet).

 - Moves a few more pieces to ProcessedOptions
==========

to

==========
First step for modular output in fasta.

Some details:
 - Adds the tree-shaker logic and unit tests for it,

   Note: I'm still not clear on what degree of tree-shaking we can get away
   with. For now I have a mode to delete anything that is not seen explicitly.
   This passes verification for all of our tests so far, but breaks both the
   mixin and the async/await transformer in some scenarios.

 - Writes a prototype of kernelForBuildUnit (although no automated tests for it
   yet).

 - Moves a few more pieces to ProcessedOptions

R=scheglov@google.com

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

[Siggi Cherem (dart-lang), 2017-05-19 20:59:22]

Committed patchset #3 (id:120001) manually as
d9e9382373598a953ac184e87454dff78e611111 (presubmit successful).

[ahe, 2017-05-22 12:04:24]

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart (right):

https://codereview.chromium.org/2893493004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_outline_shaker.dart:14: /// Removes
from [program] unnecessary libraries, classes, and members.
Removes unnecessary libraries, classes, and members from [program].