Introduce IKG into kernel-service to support incremental compilation.

Introduce IKG into kernel-service to support incremental compilation.

This CL also adds few hot-reload incremental kernel files tests(some pass, one is failing), which are useful to track progress of hot reload functionality in Flutter setting.

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

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

[aam, 2017-08-04 01:15:06]

aam@google.com changed reviewers:
+ asiva@google.com, sigmund@google.com

[aam, 2017-08-04 01:15:06]

PTAL, thanks!

[siva, 2017-08-07 23:59:29]

https://codereview.chromium.org/2993013002/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2993013002/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:3132: Dart_SourceFile source_files[]);
Could we potentially merge the two API calls
Dart_CompileSourcesToKernel and Dart_IncrementallyCompileSourcesToKernel by
changing the signature
 
Dart_CompileSourcesToKernel(const char* script_uri, int source_files_count,
Dart_SourceFile source_files[], bool incremental_compile);

https://codereview.chromium.org/2993013002/diff/1/runtime/vm/unit_test.cc
File runtime/vm/unit_test.cc (right):

https://codereview.chromium.org/2993013002/diff/1/runtime/vm/unit_test.cc#new...
runtime/vm/unit_test.cc:178: sourcefiles, kernel_pgm, incrementally);
Can we get rid of this version of CompileTestScriptWithDFE by moving the setup
of sourcefiles[] to the one call point in the LibraryTagHandler.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:161: isolateCompilers[isolateId] =
compiler;
When is the incremental compiler object deleted? I guess when the isolate shuts
down we need a way to clear this object out.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:190: }
Do you plan on switching this to always use the incremental compiler even when
the incremental flag is not specified?

It looks like now we are not clearing the incremental state when a non
incremental request is made.

[Siggi Cherem (dart-lang), 2017-08-08 00:08:07]

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:44: FileSystem fs;
- make final
- dart style nit: for fields, try to avoid abbreviations (fileSystem instead of
`fs`). For local variables is more accepted, but still avoided sometimes

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:45: CompilerOptions options;
might be nice to make this final too.

To do that, consider using a factory method that then calls a private
constructor with no body, or move the initialization out of the constructor body
in the code below that calls `new *Compiler()`.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:56: // TODO(sigmund): use outline.dill
when the mixin transformer is modular.
nit: rewrap comments to 80 col (here and below)

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:89: final dynamic map =
JSON.decode(librariesJson)['libraries'];
Instead of loadingDartLibraries explicitly, simply set the sdkRoot location in
CompilerOptions. Then fasta will be able to load the code it needs.

Basically, you should only need to use the dartLibraries option I you must
specify a different set of libraries than the default. Note also that I just
landed a change that simplify doing so: I got rid of
CompilerOptions.dartLibraries and added Compiler.librariesSpecificationUri
(which is a URI to a libraries.json file).

Once Konstantin submits his support to load the sdk-outline in IKG, we should be
able to use the exact same options here and in the batch compiler.

Given all these changes, I think it will make sense to move the creation of
CompilerOptions back to _processLoadRequest.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:98: options..dartLibraries =
loadDartLibraries(libraries, librariesJson);
nit: no need to use cascades here.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:139: (compiler.fs as HybridFileSystem)
possibly the VM can be smart about removing this check after one loop iteration,
but it probably will help to mark `fs` as final.

Alternative, move the cast up to line 135:

HybridFileSystem fs = compiler.fs;
...

[aam, 2017-08-08 16:27:34]

https://codereview.chromium.org/2993013002/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2993013002/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:3132: Dart_SourceFile source_files[]);
On 2017/08/07 23:59:29, siva wrote:
> Could we potentially merge the two API calls
> Dart_CompileSourcesToKernel and Dart_IncrementallyCompileSourcesToKernel by
> changing the signature
>  
> Dart_CompileSourcesToKernel(const char* script_uri, int source_files_count,
> Dart_SourceFile source_files[], bool incremental_compile);

Done.

https://codereview.chromium.org/2993013002/diff/1/runtime/vm/unit_test.cc
File runtime/vm/unit_test.cc (right):

https://codereview.chromium.org/2993013002/diff/1/runtime/vm/unit_test.cc#new...
runtime/vm/unit_test.cc:178: sourcefiles, kernel_pgm, incrementally);
On 2017/08/07 23:59:29, siva wrote:
> Can we get rid of this version of CompileTestScriptWithDFE by moving the setup
> of sourcefiles[] to the one call point in the LibraryTagHandler.

Original version that takes const char* source is used in few other places:
dart_api_impl_test, isolate_reload_test. I hesitated to copy this boilerplate
code there as well.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:44: FileSystem fs;
On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> - make final
> - dart style nit: for fields, try to avoid abbreviations (fileSystem instead
of
> `fs`). For local variables is more accepted, but still avoided sometimes

Done.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:56: // TODO(sigmund): use outline.dill
when the mixin transformer is modular.
On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> nit: rewrap comments to 80 col (here and below)

Hmm... With formatting I prefer to rely on automated tools, and dartfmt didn't
wrap the comments. Are the comments off limits for dartfmt?
Done.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:89: final dynamic map =
JSON.decode(librariesJson)['libraries'];
On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> Instead of loadingDartLibraries explicitly, simply set the sdkRoot location in
> CompilerOptions. Then fasta will be able to load the code it needs.
> 
> Basically, you should only need to use the dartLibraries option I you must
> specify a different set of libraries than the default. Note also that I just
> landed a change that simplify doing so: I got rid of
> CompilerOptions.dartLibraries and added Compiler.librariesSpecificationUri
> (which is a URI to a libraries.json file).
> 
> Once Konstantin submits his support to load the sdk-outline in IKG, we should
be
> able to use the exact same options here and in the batch compiler.
Done.

> 
> Given all these changes, I think it will make sense to move the creation of
> CompilerOptions back to _processLoadRequest.

I don't see how I can do this cleanly given that for incremental compiler mode I
might or might not need compiler options created depending on whether compiler
for particular isolate id already exists.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:98: options..dartLibraries =
loadDartLibraries(libraries, librariesJson);
On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> nit: no need to use cascades here.

Acknowledged.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:139: (compiler.fs as HybridFileSystem)
On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> possibly the VM can be smart about removing this check after one loop
iteration,
> but it probably will help to mark `fs` as final.
> 
> Alternative, move the cast up to line 135:
> 
> HybridFileSystem fs = compiler.fs;
> ...
> 

Done.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:161: isolateCompilers[isolateId] =
compiler;
On 2017/08/07 23:59:29, siva wrote:
> When is the incremental compiler object deleted? I guess when the isolate
shuts
> down we need a way to clear this object out.

Before we enable IKG flow for anything besides vm unit tests we will have to add
kernel isolate shutdown listener, which will send a message  that would be
handled by kernel-isolate.dart that would destroy corresponding isolateCompiler
object.
Hopefully we can do this in follow-up cl.

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:190: }
On 2017/08/07 23:59:29, siva wrote:
> Do you plan on switching this to always use the incremental compiler even when
> the incremental flag is not specified?
> 
> It looks like now we are not clearing the incremental state when a non
> incremental request is made.

Yeah, currently incremental compilers are not cleaned up, I was hoping to do
that in follow up cl while we can start reaping benefits of having some vm tests
that exercise hot reload from incremental kernels.

The prerequisite to always use incremental compiler was the ability for IKG to
load core libraries from platform/outline.dill rather than from sources. I think
this was recently implemented, so we should be able to follow-up shortly with cl
that does the switch. This cl doesn't do that.

[siva, 2017-08-09 00:08:15]

lgtm

Maybe add TODO for the items that are planned for the next CL.

[aam, 2017-08-09 01:16:29]

On 2017/08/09 00:08:15, siva wrote:
> lgtm
> 
> Maybe add TODO for the items that are planned for the next CL.

Done.

[aam, 2017-08-09 01:16:32]

On 2017/08/09 00:08:15, siva wrote:
> lgtm
> 
> Maybe add TODO for the items that are planned for the next CL.

Done.

[Siggi Cherem (dart-lang), 2017-08-09 15:57:58]

lgtm (after deleting the old allocation of the IncrementalCompiler)

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:45: CompilerOptions options;
On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> might be nice to make this final too.
> 
> To do that, consider using a factory method that then calls a private
> constructor with no body, or move the initialization out of the constructor
body
> in the code below that calls `new *Compiler()`.

Another idea for this: if you move the entire body into a static function, you
can also make the options final:

CompilerOptions createOptions(FileSystem fileSystem) {
  ...
}

class Compiler {
  final CompilerOptions options;

  Compiler(this.fileSystem)
     : options = createOptions(fileSystem);
}

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:56: // TODO(sigmund): use outline.dill
when the mixin transformer is modular.
On 2017/08/08 16:27:33, aam wrote:
> On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> > nit: rewrap comments to 80 col (here and below)
> 
> Hmm... With formatting I prefer to rely on automated tools, and dartfmt didn't
> wrap the comments. Are the comments off limits for dartfmt?
> Done.

thanks, they are off limits unfortunately

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:89: final dynamic map =
JSON.decode(librariesJson)['libraries'];
On 2017/08/08 16:27:33, aam wrote:
> On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> > Instead of loadingDartLibraries explicitly, simply set the sdkRoot location
in
> > CompilerOptions. Then fasta will be able to load the code it needs.
> > 
> > Basically, you should only need to use the dartLibraries option I you must
> > specify a different set of libraries than the default. Note also that I just
> > landed a change that simplify doing so: I got rid of
> > CompilerOptions.dartLibraries and added Compiler.librariesSpecificationUri
> > (which is a URI to a libraries.json file).
> > 
> > Once Konstantin submits his support to load the sdk-outline in IKG, we
should
> be
> > able to use the exact same options here and in the batch compiler.
> Done.
> 
> > 
> > Given all these changes, I think it will make sense to move the creation of
> > CompilerOptions back to _processLoadRequest.
> 
> I don't see how I can do this cleanly given that for incremental compiler mode
I
> might or might not need compiler options created depending on whether compiler
> for particular isolate id already exists.

I see - it would mean we would create and discard the options object in that
case.

https://codereview.chromium.org/2993013002/diff/60001/utils/kernel-service/ke...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2993013002/diff/60001/utils/kernel-service/ke...
utils/kernel-service/kernel-service.dart:148: compiler = new
IncrementalCompiler(fs, librariesUri, librariesJson);
delete this line?

[aam, 2017-08-09 16:17:04]

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2993013002/diff/1/utils/kernel-service/kernel...
utils/kernel-service/kernel-service.dart:45: CompilerOptions options;
On 2017/08/09 15:57:58, Siggi Cherem (dart-lang) wrote:
> On 2017/08/08 00:08:07, Siggi Cherem (dart-lang) wrote:
> > might be nice to make this final too.
> > 
> > To do that, consider using a factory method that then calls a private
> > constructor with no body, or move the initialization out of the constructor
> body
> > in the code below that calls `new *Compiler()`.
> 
> Another idea for this: if you move the entire body into a static function, you
> can also make the options final:
> 
> CompilerOptions createOptions(FileSystem fileSystem) {
>   ...
> }
> 
> class Compiler {
>   final CompilerOptions options;
> 
>   Compiler(this.fileSystem)
>      : options = createOptions(fileSystem);
> }

Yeah, I looked at that. What stopped me then was reference to instance property
'errors' in that code.

https://codereview.chromium.org/2993013002/diff/60001/utils/kernel-service/ke...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2993013002/diff/60001/utils/kernel-service/ke...
utils/kernel-service/kernel-service.dart:148: compiler = new
IncrementalCompiler(fs, librariesUri, librariesJson);
On 2017/08/09 15:57:58, Siggi Cherem (dart-lang) wrote:
> delete this line?

Oops, done. Thanks!

[aam, 2017-08-11 13:11:40]

PTAL.
I had to fix few things before submitting this:
- I was not checking for "no thread/no isolate" situation, so compilation from
command line was failing(dart --kernel_binaries.. --dfe... main.dart)
- Frontend got new required api call "acceptLastDelta".
- New vm unit test that incrementally compiles application with two files is
failing under debug build. I tried without much success getting to the bottom of
this, so created new issue instead and marked test as failing. This way, once I
submit this CL, hopefully more folks can take a look at it.

[Siggi Cherem (dart-lang), 2017-08-11 15:22:39]

changes to kernel-service.dart lgtm

[siva, 2017-08-14 20:50:32]

lgtm

https://codereview.chromium.org/2993013002/diff/100001/runtime/vm/kernel_isol...
File runtime/vm/kernel_isolate.cc (right):

https://codereview.chromium.org/2993013002/diff/100001/runtime/vm/kernel_isol...
runtime/vm/kernel_isolate.cc:348: }
As discussed offline we should be able to assert that an Isolate must exist once
we fix the code in main to not invoke the compiler before the isolate is
created. Maybe add a TODO here so we remember to change this code once that bug
is fixed.

[aam, 2017-08-14 21:01:27]

https://codereview.chromium.org/2993013002/diff/100001/runtime/vm/kernel_isol...
File runtime/vm/kernel_isolate.cc (right):

https://codereview.chromium.org/2993013002/diff/100001/runtime/vm/kernel_isol...
runtime/vm/kernel_isolate.cc:348: }
On 2017/08/14 20:50:32, siva wrote:
> As discussed offline we should be able to assert that an Isolate must exist
once
> we fix the code in main to not invoke the compiler before the isolate is
> created. Maybe add a TODO here so we remember to change this code once that
bug
> is fixed.

Done.

[aam, 2017-08-14 21:06:21]

Description was changed from

==========
Introduce IKG into kernel-service to support incremental compilation.

This CL also adds few hot-reload incremental kernel files tests(some pass, one
is failing), which are useful to track progress of hot reload functionality in
Flutter setting.

BUG=
==========

to

==========
Introduce IKG into kernel-service to support incremental compilation.

This CL also adds few hot-reload incremental kernel files tests(some pass, one
is failing), which are useful to track progress of hot reload functionality in
Flutter setting.

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

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

[aam, 2017-08-14 21:06:23]

Committed patchset #7 (id:120001) manually as
f596d440f5ffd81f2cfd386c316f3a03b0d7c69b (presubmit successful).