Introduce IKG into kernel-service to support incremental compilation.

utils/kernel-service/kernel-service.dart

 // Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /// This is an interface to the Dart Kernel parser and Kernel binary generator.
 ///
 /// It is used by the kernel-isolate to load Dart source code and generate
 /// Kernel binary format.
 ///
 /// This is either invoked as the root script of the Kernel isolate when used
 /// as a part of
 ///
 ///         dart --dfe=utils/kernel-service/kernel-service.dart ...
 ///
 /// invocation or it is invoked as a standalone script to perform training for
 /// the app-jit snapshot
 ///
 ///         dart utils/kernel-service/kernel-service.dart --train <source-file>
 ///
 ///
 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';
 import 'package:front_end/src/testing/hybrid_file_system.dart';
 import 'package:kernel/kernel.dart' show Program;
 import 'package:kernel/target/targets.dart' show TargetFlags;
 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 {
   if (verbose) print("DFE: request: $request");
 
   int tag = request[0];
   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;
+  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.

 
-  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}");
-  }
-
-  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}");
+    }
 
-    if (errors.isNotEmpty) {
+    Program program = await compiler.compile(script);
+
+    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(
           serializeProgram(program, filter: (lib) => !lib.isExternal));
     }
   } catch (error, stack) {
     result = new CompilationResult.crash(error, stack);
   }
 
   if (verbose) print("DFE:> ${result}");
@@ skipping 39 matching lines @@
     if (response[0] == tag) {
       // Success.
       responsePort.close();
     } else if (response[0] == -tag) {
       // Compilation error.
       throw response[4];
     } else {
       throw "Unexpected response: $response";
     }
   };
-  var request = [tag, responsePort.sendPort, scriptUri];
+  var request = [
+    tag,
+    responsePort.sendPort,
+    scriptUri,
+    1 /* isolateId chosen randomly */,
+    false /* incremental */
+  ];
   _processLoadRequest(request);
 }
 
 main([args]) {
   if (args?.length == 2 && args[0] == '--train') {
     // This entry point is used when creating an app snapshot. The argument
     // provides a script to compile to warm-up generated code.
     train(args[1]);
   } else {
     // Entry point for the Kernel isolate.
@@ skipping 78 matching lines @@
   _CompilationCrash(this.exception, this.stack);
 
   @override
   Status get status => Status.crash;
 
   @override
   String get errorString => "${exception}\n${stack}";
 
   String toString() => "_CompilationCrash(${errorString})";
 }