Start actually adding incrementality into incremental kernel generator.

Start actually adding incrementality into incremental kernel generator.

We now store and reuse resolved kernel libraries.
Existing tests are not yet check this, I will add this in future CLs.

Also returned ProgramDelta(s) are still not deltas, but full bundles.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Paul Berry, 2017-05-12 22:24:57]

Assuming issues below are addressed, lgtm.

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:178: /// imported into
`dart:core`.
I assume this is a temporary workaround, and we should modify Fasta to only load
VM libraries that are referenced?  If so, please add a TODO comment.

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:221: ///
TODO(scheglov) document
Please document this before committing.

[scheglov, 2017-05-13 02:32:59]

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:178: /// imported into
`dart:core`.
On 2017/05/12 22:24:57, Paul Berry wrote:
> I assume this is a temporary workaround, and we should modify Fasta to only
load
> VM libraries that are referenced?  If so, please add a TODO comment.

I added TODO that we need to ask VM for requirements here.

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:221: ///
TODO(scheglov) document
On 2017/05/12 22:24:57, Paul Berry wrote:
> Please document this before committing.

Done.

[Paul Berry, 2017-05-13 02:56:50]

lgtm

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2877193003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:178: /// imported into
`dart:core`.
On 2017/05/13 02:32:58, scheglov wrote:
> On 2017/05/12 22:24:57, Paul Berry wrote:
> > I assume this is a temporary workaround, and we should modify Fasta to only
> load
> > VM libraries that are referenced?  If so, please add a TODO comment.
> 
> I added TODO that we need to ask VM for requirements here.

sgtm, thanks.

[scheglov, 2017-05-13 02:58:46]

Description was changed from

==========
Start actually adding incrementality into incremental kernel generator.

We now store and reuse resolved kernel libraries.
Existing tests are not yet check this, I will add this in future CLs.

Also returned ProgramDelta(s) are still not deltas, but full bundles.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Start actually adding incrementality into incremental kernel generator.

We now store and reuse resolved kernel libraries.
Existing tests are not yet check this, I will add this in future CLs.

Also returned ProgramDelta(s) are still not deltas, but full bundles.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-05-13 02:58:49]

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

[Siggi Cherem (dart-lang), 2017-05-16 21:37:01]

lgtm

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:49: /**
nit: let's only use "///" for dartdocs

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:130: //
TODO(scheglov) add salt
might be worth expanding what this means :) - I have no idea what you mean by
salt here.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:137: for (FileState
file in transitiveFiles) {
nit: `var` (here it does seem clear from context)

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:146: String
kernelKey = '$signature.kernel';
ditto (`var`)

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:149: Set<Uri>
libraryUris = new Set<Uri>();
here too

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:168: await
dillTarget.writeOutline(null);
We might need to do something to make `writeOutline` a noop for libraries that
have already been filled before. In particular, we will call writeOutline once
per cycle that we are compiling, so if you have:

   A   -- imports -->   B   -- imports -->   C

you might be filling the outline of C twice by the time you compile A. This
might be an issue (the builders might have the contents of C twice), or if
that's ignored, it appears to be wasteful. So it's probably worth taking a
closer look.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:205: for (Uri
fileUri in _invalidatedFiles) {
here too (`var`)

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:206: FileState file
= await _fsState.getFile(fileUri);
and here.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:207: await
file.refresh();
do you need the refresh to happen one file at a time or is parallelism allowed
here?

If parallelism is OK, you could run each of these iterations separately and
merge them with Future.wait, or you could even do them in 2 stages:

   List<FileState> files = await
Future.wait(_invalidatedFiles.map(_fsState.getFile).toList());
   await Future.wait(files.map((f) => f.refresh()).toList());

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:235: /// included,
they were returned as results for preceding cycles.
minor dartdoc suggestion: consider rephrasing this a bit to clarify that the
*body* of dependencies is not included, but references to those dependencies are
included.

[scheglov, 2017-05-16 22:03:39]

I will send a new CL with fixes.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:49: /**
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> nit: let's only use "///" for dartdocs

Fixed.
I try to follow this style.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:130: //
TODO(scheglov) add salt
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> might be worth expanding what this means :) - I have no idea what you mean by
> salt here.

Yeah, it's more a reminder for myself. Paul and I are familiar with
AnalysisDriver internals.

Salt something we mix into the signatures and it is a combination of the
compiler version, compilation options, etc.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:137: for (FileState
file in transitiveFiles) {
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> nit: `var` (here it does seem clear from context)

Done.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:146: String
kernelKey = '$signature.kernel';
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> ditto (`var`)

Done.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:149: Set<Uri>
libraryUris = new Set<Uri>();
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> here too

Done.

We probably should write a tool to remove all types for all local variables
then.
I see a lot of well typed code in Fasta.
It would be nice to have similar code style in all places of the package.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:168: await
dillTarget.writeOutline(null);
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> We might need to do something to make `writeOutline` a noop for libraries that
> have already been filled before. In particular, we will call writeOutline once
> per cycle that we are compiling, so if you have:
> 
>    A   -- imports -->   B   -- imports -->   C
> 
> you might be filling the outline of C twice by the time you compile A. This
> might be an issue (the builders might have the contents of C twice), or if
> that's ignored, it appears to be wasteful. So it's probably worth taking a
> closer look.

It works fine, we fill only once.

Loader.buildOutlines() removes from unparsedLibraries.
We fill outlines only for new libraries for which read() was invoked.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:205: for (Uri
fileUri in _invalidatedFiles) {
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> here too (`var`)

Done.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:206: FileState file
= await _fsState.getFile(fileUri);
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> and here.

Done.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:207: await
file.refresh();
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> do you need the refresh to happen one file at a time or is parallelism allowed
> here?
> 
> If parallelism is OK, you could run each of these iterations separately and
> merge them with Future.wait, or you could even do them in 2 stages:
> 
>    List<FileState> files = await
> Future.wait(_invalidatedFiles.map(_fsState.getFile).toList());
>    await Future.wait(files.map((f) => f.refresh()).toList());

I think it will work.
But I'd like to see how it works on real app to measure performance difference.

https://codereview.chromium.org/2877193003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:235: /// included,
they were returned as results for preceding cycles.
On 2017/05/16 21:37:01, Siggi Cherem (dart-lang) wrote:
> minor dartdoc suggestion: consider rephrasing this a bit to clarify that the
> *body* of dependencies is not included, but references to those dependencies
are
> included. 

Done.