Initial version of Fasta based IncrementalKernelGenerator.

Initial version of Fasta based IncrementalKernelGenerator.

No incrementality yet.
In a separate experiment I can load SDK kernel and resolve against it.
So, I will add it in a following CL, or maybe even generalize to any library.


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

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

[scheglov, 2017-05-07 01:32:21]

Description was changed from

==========
Initial version of Fasta based IncrementalKernelGenerator.

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

to

==========
Initial version of Fasta based IncrementalKernelGenerator.

No incrementality yet.
In a separate experiment I can load SDK kernel and resolve against it.
So, I will add it in a following CL, or maybe even generalize to any library.


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

[Paul Berry, 2017-05-07 14:17:39]

paulberry@google.com changed reviewers:
+ kmillikin@google.com

[Paul Berry, 2017-05-07 14:17:40]

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:59: new
IncrementalKernelGeneratorImpl(source, options);
Why are you removing ProcessedOptions?  Is there some option that isn't exposed
through its API?  If so, I would rather that we keep using ProcessedOptions and
just have it expose the option you need.

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

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:50: // Maybe rework
and restore ProcessedOptions?
Moving creation of TranslateUri to ProcessedOptions SGTM

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:34: /// with code and
some with only outlines.
My understanding from talking to Dan Rubel and the VM folks is that for hot
reload, the VM wanted to be given multiple kernel files, one for each library
URI.  But that may have changed.  Siggi and Kevin probably have the latest
information.

[scheglov, 2017-05-07 17:38:49]

PTAL

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:59: new
IncrementalKernelGeneratorImpl(source, options);
On 2017/05/07 14:17:40, Paul Berry wrote:
> Why are you removing ProcessedOptions?  Is there some option that isn't
exposed
> through its API?  If so, I would rather that we keep using ProcessedOptions
and
> just have it expose the option you need.

It seemed to me initially that ProcessedOptions is too Analyzer specific.
So, I speculatively removed it.
Now, when it also has TranslateUri, it is more useful.

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

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:50: // Maybe rework
and restore ProcessedOptions?
On 2017/05/07 14:17:40, Paul Berry wrote:
> Moving creation of TranslateUri to ProcessedOptions SGTM

Done.

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:34: /// with code and
some with only outlines.
On 2017/05/07 14:17:40, Paul Berry wrote:
> My understanding from talking to Dan Rubel and the VM folks is that for hot
> reload, the VM wanted to be given multiple kernel files, one for each library
> URI.  But that may have changed.  Siggi and Kevin probably have the latest
> information.

FWIW, having a kernel file per affected library would be very inefficient.

Each libraries should be serialized with it all dependencies (even if just
outlines, and after tree shake).

Imagine that libraries A, B and C transitively depend on 1000 of other
libraries, and A and B depend on C. If C changes, we need to send A, B and C. If
we do this in separate kernel files, we would need to serialize A + 1000 other
libraries, B + 1000, C + 1000. And If there are not 3, but 300 of affected
libraries (not unusual), then 300 * 1000 libraries to serialize. Ouch.

[Paul Berry, 2017-05-08 12:55:14]

lgtm

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2869563002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:34: /// with code and
some with only outlines.
On 2017/05/07 17:38:49, scheglov wrote:
> On 2017/05/07 14:17:40, Paul Berry wrote:
> > My understanding from talking to Dan Rubel and the VM folks is that for hot
> > reload, the VM wanted to be given multiple kernel files, one for each
library
> > URI.  But that may have changed.  Siggi and Kevin probably have the latest
> > information.
> 
> FWIW, having a kernel file per affected library would be very inefficient.
> 
> Each libraries should be serialized with it all dependencies (even if just
> outlines, and after tree shake).
> 
> Imagine that libraries A, B and C transitively depend on 1000 of other
> libraries, and A and B depend on C. If C changes, we need to send A, B and C.
If
> we do this in separate kernel files, we would need to serialize A + 1000 other
> libraries, B + 1000, C + 1000. And If there are not 3, but 300 of affected
> libraries (not unusual), then 300 * 1000 libraries to serialize. Ouch.

Agreed.  The way I originally wanted to implement it was more along the lines of
what you are thinking too.  Hopefully Siggi or Kevin can weigh in on this
question today.

[scheglov, 2017-05-08 16:19:31]

Description was changed from

==========
Initial version of Fasta based IncrementalKernelGenerator.

No incrementality yet.
In a separate experiment I can load SDK kernel and resolve against it.
So, I will add it in a following CL, or maybe even generalize to any library.


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

to

==========
Initial version of Fasta based IncrementalKernelGenerator.

No incrementality yet.
In a separate experiment I can load SDK kernel and resolve against it.
So, I will add it in a following CL, or maybe even generalize to any library.


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

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

[scheglov, 2017-05-08 16:19:32]

Committed patchset #3 (id:40001) manually as
9acca5f46423c35fea5002bf09f951ba6b9ac9ea (presubmit successful).

[Siggi Cherem (dart-lang), 2017-05-08 17:44:29]

a couple drive-by comments

https://codereview.chromium.org/2869563002/diff/40001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2869563002/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:10: import
'package:front_end/src/fasta/translate_uri.dart';
a very low priority comment - nothing to do on this CL:

At some point we should unify the two APIs.

I propose that we change translate_uri into uri_resolver. UriResolver is more
robust and has a better name. Eventually TranslateUri.parse should not be there
because all of fasta's APIs will get the library map from the options (like you
do here).

To reduce the churn, I think it will be easier to first change how we pass
around options, then get rid of the need of TranslateUri.parse, then do the
rename.

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

https://codereview.chromium.org/2869563002/diff/40001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:58: // Firstly, we
don't "write" anything here.
+1 :) it should not be called 'write'.

https://codereview.chromium.org/2869563002/diff/40001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:60: // This is too
interactive and not API-clients friendly.
agree - we need to add a diagnostic API and pass it around.

Initially we had an 'onError' handler in compiler_options, but I think we might
need something more tailored to diagnostics.