Use sdk summaries in front_end/kernel_generator.

Use sdk summaries in front_end/kernel_generator.

This CL adds support for using sdk summaries when building kernel using the
front_end entrypoint. I didn't expose this functionality in the dartk binary,
I'm hoping we can delay doing so until we integrate dartk and front_end.

I also switched front_end/tool/perf.dart to use kernel_generator directly.

This makes some observable difference for small scripts, but not so much with large apps like dart2js. Makes sense considering that the sdk is about 2Mb, and dart2js is 6Mb of sources.

The most interesting number I got is building the a library kernel file (not the whole program) for a small hello-world script on a warmed up vm:
no summaries: 500ms
with summaries: 30ms

R=asgerf@google.com, paulberry@google.com

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

[Siggi Cherem (dart-lang), 2016-12-09 05:15:37]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2016-12-09 05:34:09]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2016-12-09 05:38:12]

Description was changed from

==========
Use sdk summaries in front_end/kernel_generator.

This adds support for using sdk summaries when building kernel using the
front_end entrypoint. I didn't expose this functionality in the dartk binary,
I'm hoping we can delay doing so until we integrate dartk and front_end.
==========

to

==========
Use sdk summaries in front_end/kernel_generator.

This adds support for using sdk summaries when building kernel using the
front_end entrypoint. I didn't expose this functionality in the dartk binary,
I'm hoping we can delay doing so until we integrate dartk and front_end.

This makes some observable difference for small scripts, but not so much with
large apps like dart2js. Makes sense considering that the sdk is about 2Mb, and
dart2js is 6Mb of sources. 

The most interesting number I got is building the a library kernel file (not the
whole program) for a small hello-world script on a warmed up vm:
no summaries: 500ms
with summaries: 30ms
==========

[Siggi Cherem (dart-lang), 2016-12-09 05:40:09]

Description was changed from

==========
Use sdk summaries in front_end/kernel_generator.

This adds support for using sdk summaries when building kernel using the
front_end entrypoint. I didn't expose this functionality in the dartk binary,
I'm hoping we can delay doing so until we integrate dartk and front_end.

This makes some observable difference for small scripts, but not so much with
large apps like dart2js. Makes sense considering that the sdk is about 2Mb, and
dart2js is 6Mb of sources. 

The most interesting number I got is building the a library kernel file (not the
whole program) for a small hello-world script on a warmed up vm:
no summaries: 500ms
with summaries: 30ms
==========

to

==========
Use sdk summaries in front_end/kernel_generator.

This CL adds support for using sdk summaries when building kernel using the
front_end entrypoint. I didn't expose this functionality in the dartk binary,
I'm hoping we can delay doing so until we integrate dartk and front_end.

I also switched front_end/tool/perf.dart to use kernel_generator directly.
 
This makes some observable difference for small scripts, but not so much with
large apps like dart2js. Makes sense considering that the sdk is about 2Mb, and
dart2js is 6Mb of sources. 

The most interesting number I got is building the a library kernel file (not the
whole program) for a small hello-world script on a warmed up vm:
no summaries: 500ms
with summaries: 30ms
==========

[Siggi Cherem (dart-lang), 2016-12-09 05:42:08]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2016-12-09 05:42:12]

Patchset #1 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2016-12-09 05:42:45]

sigmund@google.com changed reviewers:
+ asgerf@google.com, paulberry@google.com

[asgerf, 2016-12-09 09:43:52]

LGTM!!

[Paul Berry, 2016-12-09 13:41:40]

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

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:73: repository: repository, entry:
sources.first);
It seems weird to treat the first source any differently than the others.

Would it be better if we just said that the caller is required to set
CompilerOptions.packagesFilePath when calling `kernelForBuildUnit`?  Then we
wouldn't need to make an arbitrary decision about which file to use for package
discovery.

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:85: // summary file.
This is actually undesirable behavior IMHO.  It contradicts the comment above
("The compilation process is hermetic, meaning that the only files which will be
read are those listed in [sources], [CompilerOptions.inputSummaries], and
[CompilerOptions.sdkSummary].")  Keeping the build process hermetic makes the
code simpler, has desirable properties for Bazel integration, and is consistent
with what we're doing for DDC.

I think we should replace this loop with some error checking that verifies that
all referenced libraries either come from summaries or are included in
`sources`.

On a related note, we should also have some error checking to verify that for
each "part" declaration in a library in `sources`,   the corresponding part file
is also in `sources`.

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:86: for (int i = 0; i <
repository.libraries.length; ++i) {
Nit: since `i` is not used, this would be clearer as:

for (var lib in repository.libraries)

But I guess it's moot if we get rid of this loop :)

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/tool/perf...
File pkg/front_end/tool/perf.dart (right):

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:426: // default?
I was assuming we should just always use strong mode for this code; any work we
do on spec mode in the front end seems like wasted effort.

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:433: options.sdkSummary =
'out/ReleaseX64/dart-sdk/lib/_internal/spec.sum';
Same here--why not use "strong.sum"?

[asgerf, 2016-12-09 13:56:53]

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

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:86: for (int i = 0; i <
repository.libraries.length; ++i) {
On 2016/12/09 13:41:40, Paul Berry wrote:
> Nit: since `i` is not used, this would be clearer as:
> 
> for (var lib in repository.libraries)
> 
> But I guess it's moot if we get rid of this loop :)

I think the number of libraries will grow during iteration as more libraries are
loaded, so maybe that should be clarified in a comment.

[Siggi Cherem (dart-lang), 2016-12-09 16:58:21]

thanks for the reviews!

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

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:73: repository: repository, entry:
sources.first);
On 2016/12/09 13:41:40, Paul Berry wrote:
> It seems weird to treat the first source any differently than the others.
> 
> Would it be better if we just said that the caller is required to set
> CompilerOptions.packagesFilePath when calling `kernelForBuildUnit`?  Then we
> wouldn't need to make an arbitrary decision about which file to use for
package
> discovery.

Good point, will do once we decide on the other aspects below

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:85: // summary file.
On 2016/12/09 13:41:40, Paul Berry wrote:
> This is actually undesirable behavior IMHO.  It contradicts the comment above
> ("The compilation process is hermetic, meaning that the only files which will
be
> read are those listed in [sources], [CompilerOptions.inputSummaries], and
> [CompilerOptions.sdkSummary].")  Keeping the build process hermetic makes the
> code simpler, has desirable properties for Bazel integration, and is
consistent
> with what we're doing for DDC.
> 
> I think we should replace this loop with some error checking that verifies
that
> all referenced libraries either come from summaries or are included in
> `sources`.
> 
> On a related note, we should also have some error checking to verify that for
> each "part" declaration in a library in `sources`,   the corresponding part
file
> is also in `sources`.

Let's chat more about this.

I initially was kin to keeping it hermetic too, but then realized this makes it
much easier to use outside of the context of blaze, e.g. from the command line.
I believe DDC also supports this mode of operation on the command line after
some user feedback. IIUC, there is a preference to list just a few entry-points
and not all the reachable files.

For this specific CL, I'm happy to change it back, but I'd like to figure out
how to support the following use cases:
(a) generate kernel for whole program (including sdk)
(b) generate kernel for whole program user code (no sdk is compiled)
(c) generate kernel for whole program user code + attach kernel for the sdk (sdk
is not compiled, but it is attached)
(d) generate kernel for a build unit.
(e) generate kernel for the sdk

Currently we do (a) via generateForProgram, and (d) via generateForBuildUnit.

This CL is sort of doing (b) by changing generateForBuildUnit to make it
non-hermetic. An alternative is to do it like (a) with an option to exclude the
SDK. I'd be happy to do that for now. Let's chat offline about what we do with
(c) and (e) as well. (c) is needed short term to test how to run a program until
the VM supports loading the sdk from a snapshot.

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:86: for (int i = 0; i <
repository.libraries.length; ++i) {
On 2016/12/09 13:56:53, asgerf wrote:
> On 2016/12/09 13:41:40, Paul Berry wrote:
> > Nit: since `i` is not used, this would be clearer as:
> > 
> > for (var lib in repository.libraries)
> > 
> > But I guess it's moot if we get rid of this loop :)
> 
> I think the number of libraries will grow during iteration as more libraries
are
> loaded, so maybe that should be clarified in a comment.

Asger is correct - the list is growing as more libraries are discovered, so the
list of libraries behaves like a work-queue.

[Siggi Cherem (dart-lang), 2016-12-09 20:46:40]

PTAL

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

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:73: repository: repository, entry:
sources.first);
On 2016/12/09 16:58:21, Siggi Cherem (dart-lang) wrote:
> On 2016/12/09 13:41:40, Paul Berry wrote:
> > It seems weird to treat the first source any differently than the others.
> > 
> > Would it be better if we just said that the caller is required to set
> > CompilerOptions.packagesFilePath when calling `kernelForBuildUnit`?  Then we
> > wouldn't need to make an arbitrary decision about which file to use for
> package
> > discovery.
> 
> Good point, will do once we decide on the other aspects below

Done.

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:85: // summary file.
On 2016/12/09 16:58:21, Siggi Cherem (dart-lang) wrote:
> On 2016/12/09 13:41:40, Paul Berry wrote:
> > This is actually undesirable behavior IMHO.  It contradicts the comment
above
> > ("The compilation process is hermetic, meaning that the only files which
will
> be
> > read are those listed in [sources], [CompilerOptions.inputSummaries], and
> > [CompilerOptions.sdkSummary].")  Keeping the build process hermetic makes
the
> > code simpler, has desirable properties for Bazel integration, and is
> consistent
> > with what we're doing for DDC.
> > 
> > I think we should replace this loop with some error checking that verifies
> that
> > all referenced libraries either come from summaries or are included in
> > `sources`.
> > 
> > On a related note, we should also have some error checking to verify that
for
> > each "part" declaration in a library in `sources`,   the corresponding part
> file
> > is also in `sources`.
> 
> Let's chat more about this.
> 
> I initially was kin to keeping it hermetic too, but then realized this makes
it
> much easier to use outside of the context of blaze, e.g. from the command
line.
> I believe DDC also supports this mode of operation on the command line after
> some user feedback. IIUC, there is a preference to list just a few
entry-points
> and not all the reachable files.
> 
> For this specific CL, I'm happy to change it back, but I'd like to figure out
> how to support the following use cases:
> (a) generate kernel for whole program (including sdk)
> (b) generate kernel for whole program user code (no sdk is compiled)
> (c) generate kernel for whole program user code + attach kernel for the sdk
(sdk
> is not compiled, but it is attached)
> (d) generate kernel for a build unit.
> (e) generate kernel for the sdk
> 
> Currently we do (a) via generateForProgram, and (d) via generateForBuildUnit.
> 
> This CL is sort of doing (b) by changing generateForBuildUnit to make it
> non-hermetic. An alternative is to do it like (a) with an option to exclude
the
> SDK. I'd be happy to do that for now. Let's chat offline about what we do with
> (c) and (e) as well. (c) is needed short term to test how to run a program
until
> the VM supports loading the sdk from a snapshot.
> 

Updated the code based on an offline discussion. Here is the summary:
- I made it hermetic by default.
- I added CompilerOptions.chaseDependencies to let you override the behavior and
include extra files. 
- kernelForProgram chases dependencies by default, it handles use-case #a and #b
right now.
- use case #c is expected to be temporary and we might not submit that to the
code (it wont be necessary as soon as the VM can load kernel with external
references to the sdk)

I agree about checking that [part] files are covered, but made it a TODO for
now.

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:86: for (int i = 0; i <
repository.libraries.length; ++i) {
On 2016/12/09 16:58:21, Siggi Cherem (dart-lang) wrote:
> On 2016/12/09 13:56:53, asgerf wrote:
> > On 2016/12/09 13:41:40, Paul Berry wrote:
> > > Nit: since `i` is not used, this would be clearer as:
> > > 
> > > for (var lib in repository.libraries)
> > > 
> > > But I guess it's moot if we get rid of this loop :)
> > 
> > I think the number of libraries will grow during iteration as more libraries
> are
> > loaded, so maybe that should be clarified in a comment.
> 
> Asger is correct - the list is growing as more libraries are discovered, so
the
> list of libraries behaves like a work-queue.

Done, added some documentation.

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/tool/perf...
File pkg/front_end/tool/perf.dart (right):

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:426: // default?
On 2016/12/09 13:41:40, Paul Berry wrote:
> I was assuming we should just always use strong mode for this code; any work
we
> do on spec mode in the front end seems like wasted effort.

discussed offline: we added another option for this.

The reason is that we need to support spec mode for a while longer in order to:
 - continue to have coverage with dartk of all language and co19 tests (until
language and co19 are strong clean)
 - make comparisons with existing tool chains more apples to apples.
The option will be deprecated once we are further along.

https://codereview.chromium.org/2562923002/diff/80001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:433: options.sdkSummary =
'out/ReleaseX64/dart-sdk/lib/_internal/spec.sum';
On 2016/12/09 13:41:40, Paul Berry wrote:
> Same here--why not use "strong.sum"?

Ditto

[Paul Berry, 2016-12-09 21:20:13]

lgtm

[Siggi Cherem (dart-lang), 2016-12-09 21:29:47]

Description was changed from

==========
Use sdk summaries in front_end/kernel_generator.

This CL adds support for using sdk summaries when building kernel using the
front_end entrypoint. I didn't expose this functionality in the dartk binary,
I'm hoping we can delay doing so until we integrate dartk and front_end.

I also switched front_end/tool/perf.dart to use kernel_generator directly.
 
This makes some observable difference for small scripts, but not so much with
large apps like dart2js. Makes sense considering that the sdk is about 2Mb, and
dart2js is 6Mb of sources. 

The most interesting number I got is building the a library kernel file (not the
whole program) for a small hello-world script on a warmed up vm:
no summaries: 500ms
with summaries: 30ms
==========

to

==========
Use sdk summaries in front_end/kernel_generator.

This CL adds support for using sdk summaries when building kernel using the
front_end entrypoint. I didn't expose this functionality in the dartk binary,
I'm hoping we can delay doing so until we integrate dartk and front_end.

I also switched front_end/tool/perf.dart to use kernel_generator directly.

This makes some observable difference for small scripts, but not so much with
large apps like dart2js. Makes sense considering that the sdk is about 2Mb, and
dart2js is 6Mb of sources.

The most interesting number I got is building the a library kernel file (not the
whole program) for a small hello-world script on a warmed up vm:
no summaries: 500ms
with summaries: 30ms

R=asgerf@google.com, paulberry@google.com

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

[Siggi Cherem (dart-lang), 2016-12-09 21:29:48]

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