Introduce IKG into kernel-service to support incremental compilation.

utils/kernel-service/kernel-service.dart

Index: utils/kernel-service/kernel-service.dart
diff --git a/utils/kernel-service/kernel-service.dart b/utils/kernel-service/kernel-service.dart
index 92cb34eb2da696525bf4b9982f17e9839399e0c0..8e59203da7a5224d908ca8192bb6ee22436542be 100644
--- a/utils/kernel-service/kernel-service.dart
+++ b/utils/kernel-service/kernel-service.dart
@@ -21,12 +21,14 @@
 library runtime.tools.kernel_service;
 
 import 'dart:async' show Future;
+import 'dart:convert' show JSON;
 import 'dart:io' show Platform hide FileSystemEntity;
 import 'dart:isolate';
 import 'dart:typed_data' show Uint8List;
 
 import 'package:front_end/file_system.dart';
 import 'package:front_end/front_end.dart';
+import 'package:front_end/incremental_kernel_generator.dart';
 import 'package:front_end/memory_file_system.dart';
 import 'package:front_end/physical_file_system.dart';
 import 'package:front_end/src/fasta/kernel/utils.dart';
@@ -38,6 +40,129 @@ import 'package:kernel/target/vm_fasta.dart' show VmFastaTarget;
 const bool verbose = const bool.fromEnvironment('DFE_VERBOSE');
 const bool strongMode = const bool.fromEnvironment('DFE_STRONG_MODE');
 
+abstract class Compiler {
+  FileSystem fs;

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

- 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

[aam, 2017/08/08 16:27:33]

Done.

+  CompilerOptions options;

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

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()`.

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

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);
}

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

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

+  List<String> errors = new List<String>();
+
+  Compiler(this.fs) {
+    Uri packagesUri = (Platform.packageConfig != null)
+        ? Uri.parse(Platform.packageConfig)
+        : null;
+
+    Uri sdkSummary = Uri.base
+        .resolveUri(new Uri.file(Platform.resolvedExecutable))
+        .resolveUri(new Uri.directory("patched_sdk"))
+        // TODO(sigmund): use outline.dill when the mixin transformer is modular.

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

nit: rewrap comments to 80 col (here and below)

[aam, 2017/08/08 16:27:33]

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.

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

thanks, they are off limits unfortunately

+        .resolve('platform.dill');
+
+    if (verbose) {
+      print("DFE: Platform.packageConfig: ${Platform.packageConfig}");
+      print("DFE: packagesUri: ${packagesUri}");
+      print("DFE: Platform.resolvedExecutable: ${Platform.resolvedExecutable}");
+      print("DFE: sdkSummary: ${sdkSummary}");
+    }
+
+    options = new CompilerOptions()
+      ..strongMode = strongMode
+      ..fileSystem = fs
+      ..target = new VmFastaTarget(new TargetFlags(strongMode: strongMode))
+      ..packagesFileUri = packagesUri
+      ..sdkSummary = sdkSummary
+      ..verbose = verbose
+      ..throwOnErrors = false
+      ..reportMessages = true
+      ..onError = (CompilationMessage e) {
+        if (e.severity == Severity.error) {
+          // TODO(sigmund): support emitting code with errors as long as they are
+          // handled in the generated code (issue #30194).
+          errors.add(e.message);
+        }
+      };
+  }
+
+  Future<Program> compile(Uri script);
+}
+
+class IncrementalCompiler extends Compiler {
+  Map<String, Uri> loadDartLibraries(Uri libraries, dynamic librariesJson) {
+    final dynamic map = JSON.decode(librariesJson)['libraries'];

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

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.

[aam, 2017/08/08 16:27:33]

Done.
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.

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

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

+    final Map<String, Uri> dartLibraries = <String, Uri>{};
+    map.forEach(
+        (String k, String v) => dartLibraries[k] = libraries.resolve(v));
+    return dartLibraries;
+  }
+
+  IncrementalCompiler(FileSystem fs, Uri libraries, dynamic librariesJson)
+      : super(fs) {
+    options..dartLibraries = loadDartLibraries(libraries, librariesJson);

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

nit: no need to use cascades here.

[aam, 2017/08/08 16:27:33]

Acknowledged.

+  }
+
+  IncrementalKernelGenerator generator;
+  @override
+  Future<Program> compile(Uri script) async {
+    if (generator == null) {
+      generator = await IncrementalKernelGenerator.newInstance(options, script);
+    }
+    DeltaProgram deltaProgram = await generator.computeDelta();
+    return deltaProgram.newProgram;
+  }
+
+  void invalidate(Uri uri) {
+    generator.invalidate(uri);
+  }
+}
+
+class SingleShotCompiler extends Compiler {
+  bool requireMain;
+
+  SingleShotCompiler(FileSystem fs, this.requireMain) : super(fs);
+
+  @override
+  Future<Program> compile(Uri script) async {
+    return requireMain
+        ? kernelForProgram(script, options)
+        : kernelForBuildUnit([script], options..chaseDependencies = true);
+  }
+}
+
+final Map<int, Compiler> isolateCompilers = new Map<int, Compiler>();
+
+Future<Compiler> lookupOrBuildNewIncrementalCompiler(
+    int isolateId, List sourceFiles) async {
+  IncrementalCompiler compiler;
+  if (isolateCompilers.containsKey(isolateId)) {
+    compiler = isolateCompilers[isolateId];
+    if (sourceFiles != null) {
+      for (int i = 0; i < sourceFiles.length ~/ 2; i++) {
+        Uri uri = Uri.parse(sourceFiles[i * 2]);
+        (compiler.fs as HybridFileSystem)

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

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:33]

Done.

+            .memory
+            .entityForUri(uri)
+            .writeAsBytesSync(sourceFiles[i * 2 + 1]);
+        compiler.invalidate(uri);
+      }
+    }
+  } else {
+    Uri librariesUri = Uri.base
+        .resolveUri(new Uri.file(Platform.resolvedExecutable))
+        .resolveUri(new Uri.directory("patched_sdk"))
+        .resolveUri(new Uri.directory("lib"))
+        .resolve("libraries.json");
+    final librariesJson = await PhysicalFileSystem.instance
+        .entityForUri(librariesUri)
+        .readAsString();
+
+    final FileSystem fs = sourceFiles == null
+        ? PhysicalFileSystem.instance
+        : _buildFileSystem(sourceFiles);
+
+    compiler = new IncrementalCompiler(fs, librariesUri, librariesJson);
+    isolateCompilers[isolateId] = compiler;

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

When is the incremental compiler object deleted? I guess when the isolate shuts
down we need a way to clear this object out.

[aam, 2017/08/08 16:27:33]

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.

+  }
+  return compiler;
+}
+
 // Process a request from the runtime. See KernelIsolate::CompileToKernel in
 // kernel_isolate.cc and Loader::SendKernelRequest in loader.cc.
 Future _processLoadRequest(request) async {
@@ -47,66 +172,40 @@ Future _processLoadRequest(request) async {
   final SendPort port = request[1];
   final String inputFileUri = request[2];
   final Uri script = Uri.base.resolve(inputFileUri);
+  final bool incremental = request[3];
 
-  FileSystem fileSystem = PhysicalFileSystem.instance;
-  bool requireMain = true;
+  final List sourceFiles = request.length > 5 ? request[5] : null;
 
-  if (request.length > 3) {
-    fileSystem = _buildFileSystem(request[3]);
-    requireMain = false;
-  }
-
-  Uri packagesUri = (Platform.packageConfig != null)
-      ? Uri.parse(Platform.packageConfig)
-      : null;
-
-  Uri sdkSummary = Uri.base
-      .resolveUri(new Uri.file(Platform.resolvedExecutable))
-      .resolveUri(new Uri.directory("patched_sdk"))
-      // TODO(sigmund): use outline.dill when the mixin transformer is modular.
-      .resolve('platform.dill');
-
-  if (verbose) {
-    print("DFE: scriptUri: ${script}");
-    print("DFE: Platform.packageConfig: ${Platform.packageConfig}");
-    print("DFE: packagesUri: ${packagesUri}");
-    print("DFE: Platform.resolvedExecutable: ${Platform.resolvedExecutable}");
-    print("DFE: sdkSummary: ${sdkSummary}");
+  Compiler compiler;
+  if (incremental) {
+    final int isolateId = request[4];
+    compiler =
+        await lookupOrBuildNewIncrementalCompiler(isolateId, sourceFiles);
+  } else {
+    final FileSystem fs = sourceFiles == null
+        ? PhysicalFileSystem.instance
+        : _buildFileSystem(sourceFiles);
+    compiler =
+        new SingleShotCompiler(fs, sourceFiles == null /* requireMain */);
   }

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

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.

[aam, 2017/08/08 16:27:33]

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.

 
-  var errors = <String>[];
-  var options = new CompilerOptions()
-    ..strongMode = strongMode
-    ..fileSystem = fileSystem
-    ..target = new VmFastaTarget(new TargetFlags(strongMode: strongMode))
-    ..packagesFileUri = packagesUri
-    ..sdkSummary = sdkSummary
-    ..verbose = verbose
-    ..throwOnErrors = false
-    ..reportMessages = true
-    ..onError = (CompilationMessage e) {
-      if (e.severity == Severity.error) {
-        // TODO(sigmund): support emitting code with errors as long as they are
-        // handled in the generated code (issue #30194).
-        errors.add(e.message);
-      }
-    };
-
   CompilationResult result;
   try {
-    Program program = requireMain
-        ? await kernelForProgram(script, options)
-        : await kernelForBuildUnit([script], options..chaseDependencies = true);
+    if (verbose) {
+      print("DFE: scriptUri: ${script}");
+    }
+
+    Program program = await compiler.compile(script);
 
-    if (errors.isNotEmpty) {
+    if (compiler.errors.isNotEmpty) {
       // TODO(sigmund): the compiler prints errors to the console, so we
       // shouldn't print those messages again here.
-      result = new CompilationResult.errors(errors);
+      result = new CompilationResult.errors(compiler.errors);
     } else {
       // We serialize the program excluding platform.dill because the VM has
       // these sources built-in. Everything loaded as a summary in
       // [kernelForProgram] is marked `external`, so we can use that bit to
-      // decide what to excluce.
+      // decide what to exclude.
       // TODO(sigmund): remove the following line (Issue #30111)
       program.libraries.forEach((e) => e.isExternal = false);
       result = new CompilationResult.ok(
@@ -166,7 +265,13 @@ train(String scriptUri) {
       throw "Unexpected response: $response";
     }
   };
-  var request = [tag, responsePort.sendPort, scriptUri];
+  var request = [
+    tag,
+    responsePort.sendPort,
+    scriptUri,
+    1 /* isolateId chosen randomly */,
+    false /* incremental */
+  ];
   _processLoadRequest(request);
 }