[VM] Expression compilation when loaded from dill (part 2)

Previous version
(landed via https://dart-review.googlesource.com/c/sdk/+/135683)
only worked if the dill was loaded via
Dart_LoadScriptFromKernel where script_kernel_size was set,
and when the platform dill could be loaded from a file called the right
thing.

This CL makes it work for Dart_LoadLibraryFromKernel too and allows
the VM to send the platform along (explicitly or implicitly) and only
tries to load the platform from a file with the right name if no
platform is given in any of the inputs.

This should fix http://b/148776866

Change-Id: I62317400a932b7dcd9e126a5a88907d507f9d658
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/153609
Reviewed-by: Alexander Aprelev <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>

Author: jensjoha

pkg/vm/bin/kernel_service.dart

@@ -363,7 +363,8 @@ class IncrementalCompilerWrapper extends Compiler {
         suppressWarnings: suppressWarnings,
         enableAsserts: enableAsserts,
         experimentalFlags: experimentalFlags,
-        bytecode: bytecode);
+        bytecode: bytecode,
+        packageConfig: packageConfig);
     result.generator = new IncrementalCompiler.forExpressionCompilationOnly(
         component,
         result.options,
@@ -538,27 +539,28 @@ void invalidateSources(IncrementalCompilerWrapper compiler, List sourceFiles) {
 Future _processExpressionCompilationRequest(request) async {
   final SendPort port = request[1];
   final int isolateId = request[2];
-  final String expression = request[3];
-  final List<String> definitions = request[4].cast<String>();
-  final List<String> typeDefinitions = request[5].cast<String>();
-  final String libraryUri = request[6];
-  final String klass = request[7]; // might be null
-  final bool isStatic = request[8];
-  final List dillData = request[9];
-  final int hotReloadCount = request[10];
-  final bool suppressWarnings = request[11];
-  final bool enableAsserts = request[12];
+  final dynamic dart_platform_kernel = request[3];
+  final String expression = request[4];
+  final List<String> definitions = request[5].cast<String>();
+  final List<String> typeDefinitions = request[6].cast<String>();
+  final String libraryUri = request[7];
+  final String klass = request[8]; // might be null
+  final bool isStatic = request[9];
+  final List dillData = request[10];
+  final int blobLoadCount = request[11];
+  final bool suppressWarnings = request[12];
+  final bool enableAsserts = request[13];
   final List<String> experimentalFlags =
-      request[13] != null ? request[13].cast<String>() : null;
-  final bool bytecode = request[14];
+      request[14] != null ? request[14].cast<String>() : null;
+  final bool bytecode = request[15];
 
   IncrementalCompilerWrapper compiler = isolateCompilers[isolateId];
 
   _ExpressionCompilationFromDillSettings isolateLoadDillData =
       isolateLoadNotifies[isolateId];
   if (isolateLoadDillData != null) {
     // Check if we can reuse the compiler.
-    if (isolateLoadDillData.hotReloadCount != hotReloadCount ||
+    if (isolateLoadDillData.blobLoadCount != blobLoadCount ||
         isolateLoadDillData.prevDillCount != dillData.length) {
       compiler = isolateCompilers[isolateId] = null;
     }
@@ -571,43 +573,82 @@ Future _processExpressionCompilationRequest(request) async {
       }
       isolateLoadNotifies[isolateId] =
           new _ExpressionCompilationFromDillSettings(
-              hotReloadCount, dillData.length);
-
-      Uri platformUri =
-          computePlatformBinariesLocation().resolve('vm_platform_strong.dill');
-
-      List<List<int>> data = [];
-      data.add(new File.fromUri(platformUri).readAsBytesSync());
-      for (int i = 0; i < dillData.length; i++) {
-        data.add(dillData[i]);
-      }
+              blobLoadCount, dillData.length);
 
       // Create Component initialized from the bytes.
       Component component = new Component();
-      for (List<int> bytes in data) {
+
+      // First try to just load all "dillData". This *might* include the
+      // platform (and we might have the (same) platform both here and in
+      // dart_platform_kernel).
+      for (List<int> bytes in dillData) {
         // TODO(jensj): There might be an issue if main has changed.
         new BinaryBuilderWithMetadata(bytes, alwaysCreateNewNamedNodes: true)
             .readComponent(component);
       }
 
+      // Check if the loaded component has the platform.
+      // If it does not, try to load from dart_platform_kernel or from file.
+      bool foundDartCore = false;
+      for (Library library in component.libraries) {
+        if (library.importUri.scheme == "dart" &&
+            library.importUri.path == "core" &&
+            !library.isSynthetic) {
+          foundDartCore = true;
+          break;
+        }
+      }
+      if (!foundDartCore) {
+        List<int> platformKernel = null;
+        if (dart_platform_kernel is List<int>) {
+          platformKernel = dart_platform_kernel;
+        } else {
+          final Uri platformUri = computePlatformBinariesLocation()
+              .resolve('vm_platform_strong.dill');
+          final File platformFile = new File.fromUri(platformUri);
+          if (platformFile.existsSync()) {
+            platformKernel = platformFile.readAsBytesSync();
+          } else {
+            port.send(new CompilationResult.errors(
+                    ["No platform found to initialize incremental compiler."],
+                    null)
+                .toResponse());
+            return;
+          }
+        }
+
+        new BinaryBuilderWithMetadata(platformKernel,
+                alwaysCreateNewNamedNodes: true)
+            .readComponent(component);
+      }
+
       FileSystem fileSystem =
           _buildFileSystem([dotPackagesFile, <int>[]], null, null, null);
 
       // TODO(aam): IncrementalCompilerWrapper instance created below have to be
       // destroyed when corresponding isolate is shut down. To achieve that
       // kernel isolate needs to receive a message indicating that particular
       // isolate was shut down. Message should be handled here in this script.
-      compiler = new IncrementalCompilerWrapper.forExpressionCompilationOnly(
-          component, isolateId, fileSystem, null,
-          suppressWarnings: suppressWarnings,
-          enableAsserts: enableAsserts,
-          experimentalFlags: experimentalFlags,
-          bytecode: bytecode,
-          packageConfig: dotPackagesFile);
-      isolateCompilers[isolateId] = compiler;
-      await compiler.compile(
-          component.mainMethod?.enclosingLibrary?.importUri ??
-              component.libraries.last.importUri);
+      try {
+        compiler = new IncrementalCompilerWrapper.forExpressionCompilationOnly(
+            component, isolateId, fileSystem, null,
+            suppressWarnings: suppressWarnings,
+            enableAsserts: enableAsserts,
+            experimentalFlags: experimentalFlags,
+            bytecode: bytecode,
+            packageConfig: dotPackagesFile);
+        isolateCompilers[isolateId] = compiler;
+        await compiler.compile(
+            component.mainMethod?.enclosingLibrary?.importUri ??
+                component.libraries.last.importUri);
+      } catch (e) {
+        port.send(new CompilationResult.errors([
+          "Error when trying to create a compiler for expression compilation: "
+              "'$e'."
+        ], null)
+            .toResponse());
+        return;
+      }
     }
   }
 
@@ -1179,9 +1220,9 @@ void _debugDumpKernel(Uint8List bytes) {
 }
 
 class _ExpressionCompilationFromDillSettings {
-  int hotReloadCount;
+  int blobLoadCount;
   int prevDillCount;
 
   _ExpressionCompilationFromDillSettings(
-      this.hotReloadCount, this.prevDillCount);
+      this.blobLoadCount, this.prevDillCount);
 }

runtime/vm/compiler_test.cc

@@ -225,6 +225,7 @@ TEST_CASE(EvalExpression) {
 
     Dart_KernelCompilationResult compilation_result =
         KernelIsolate::CompileExpressionToKernel(
+            /*platform_kernel=*/nullptr, /*platform_kernel_size=*/0,
             expr_text.ToCString(), Array::empty_array(), Array::empty_array(),
             String::Handle(lib_handle.url()).ToCString(), "A",
             /* is_static= */ false);

runtime/vm/dart_api_impl.cc

@@ -5774,6 +5774,9 @@ DART_EXPORT Dart_Handle Dart_LoadLibraryFromKernel(const uint8_t* buffer,
       kernel::KernelLoader::LoadEntireProgram(program.get(), false);
   program.reset();
 
+  IsolateGroupSource* source = Isolate::Current()->source();
+  source->add_loaded_blob(Z, td);
+
   return Api::NewHandle(T, result.raw());
 #endif  // defined(DART_PRECOMPILED_RUNTIME)
 }

runtime/vm/debugger_api_impl_test.cc

@@ -180,6 +180,7 @@ DART_EXPORT Dart_Handle Dart_EvaluateStaticExpr(Dart_Handle lib_handle,
   } else {
     Dart_KernelCompilationResult compilation_result =
         KernelIsolate::CompileExpressionToKernel(
+            /* platform_kernel= */ nullptr, /* platform_kernel_size= */ 0,
             expr.ToCString(),
             /* definitions= */ Array::empty_array(),
             /* type_defintions= */ Array::empty_array(),

runtime/vm/isolate.cc

@@ -153,6 +153,63 @@ static InstancePtr DeserializeMessage(Thread* thread, Message* message) {
   }
 }
 
+void IsolateGroupSource::add_loaded_blob(
+    Zone* zone,
+    const ExternalTypedData& external_typed_data) {
+  Array& loaded_blobs = Array::Handle();
+  bool saved_external_typed_data = false;
+  if (loaded_blobs_ != nullptr) {
+    loaded_blobs = loaded_blobs_;
+
+    // Walk the array, and (if stuff was removed) compact and reuse the space.
+    // Note that the space has to be compacted as the ordering is important.
+    WeakProperty& weak_property = WeakProperty::Handle();
+    WeakProperty& weak_property_tmp = WeakProperty::Handle();
+    ExternalTypedData& existing_entry = ExternalTypedData::Handle(zone);
+    intptr_t next_entry_index = 0;
+    for (intptr_t i = 0; i < loaded_blobs.Length(); i++) {
+      weak_property ^= loaded_blobs.At(i);
+      if (weak_property.key() != ExternalTypedData::null()) {
+        if (i != next_entry_index) {
+          existing_entry = ExternalTypedData::RawCast(weak_property.key());
+          weak_property_tmp ^= loaded_blobs.At(next_entry_index);
+          weak_property_tmp.set_key(existing_entry);
+        }
+        next_entry_index++;
+      }
+    }
+    if (next_entry_index < loaded_blobs.Length()) {
+      // There's now space to re-use.
+      weak_property ^= loaded_blobs.At(next_entry_index);
+      weak_property.set_key(external_typed_data);
+      next_entry_index++;
+      saved_external_typed_data = true;
+    }
+    if (next_entry_index < loaded_blobs.Length()) {
+      ExternalTypedData& nullExternalTypedData =
+          ExternalTypedData::Handle(zone);
+      while (next_entry_index < loaded_blobs.Length()) {
+        // Null out any extra spaces.
+        weak_property ^= loaded_blobs.At(next_entry_index);
+        weak_property.set_key(nullExternalTypedData);
+        next_entry_index++;
+      }
+    }
+  }
+  if (!saved_external_typed_data) {
+    const WeakProperty& weak_property =
+        WeakProperty::Handle(WeakProperty::New(Heap::kOld));
+    weak_property.set_key(external_typed_data);
+
+    intptr_t length = loaded_blobs.IsNull() ? 0 : loaded_blobs.Length();
+    Array& new_array =
+        Array::Handle(Array::Grow(loaded_blobs, length + 1, Heap::kOld));
+    new_array.SetAt(length, weak_property);
+    loaded_blobs_ = new_array.raw();
+  }
+  num_blob_loads_++;
+}
+
 void IdleTimeHandler::InitializeWithHeap(Heap* heap) {
   MutexLocker ml(&mutex_);
   ASSERT(heap_ == nullptr && heap != nullptr);
@@ -2676,9 +2733,9 @@ void Isolate::VisitObjectPointers(ObjectPointerVisitor* visitor,
   visitor->VisitPointer(reinterpret_cast<ObjectPtr*>(&deoptimized_code_array_));
   visitor->VisitPointer(reinterpret_cast<ObjectPtr*>(&sticky_error_));
   if (isolate_group_ != nullptr) {
-    if (isolate_group_->source()->hot_reload_blobs_ != nullptr) {
+    if (isolate_group_->source()->loaded_blobs_ != nullptr) {
       visitor->VisitPointer(reinterpret_cast<ObjectPtr*>(
-          &(isolate_group_->source()->hot_reload_blobs_)));
+          &(isolate_group_->source()->loaded_blobs_)));
     }
   }
 #if !defined(PRODUCT)

runtime/vm/isolate.h

@@ -200,10 +200,13 @@ class IsolateGroupSource {
         flags(flags),
         script_kernel_buffer(nullptr),
         script_kernel_size(-1),
-        hot_reload_blobs_(nullptr),
-        num_hot_reloads_(0) {}
+        loaded_blobs_(nullptr),
+        num_blob_loads_(0) {}
   ~IsolateGroupSource() { free(name); }
 
+  void add_loaded_blob(Zone* zone_,
+                       const ExternalTypedData& external_typed_data);
+
   // The arguments used for spawning in
   // `Dart_CreateIsolateGroupFromKernel` / `Dart_CreateIsolate`.
   const char* script_uri;
@@ -223,9 +226,9 @@ class IsolateGroupSource {
   // Any newly spawned isolates need to use this permutation map.
   std::unique_ptr<intptr_t[]> cid_permutation_map;
 
-  // List of weak pointers to external typed data for hot reload blobs.
-  ArrayPtr hot_reload_blobs_;
-  intptr_t num_hot_reloads_;
+  // List of weak pointers to external typed data for loaded blobs.
+  ArrayPtr loaded_blobs_;
+  intptr_t num_blob_loads_;
 };
 
 // Tracks idle time and notifies heap when idle time expired.

runtime/vm/isolate_reload.cc

@@ -619,65 +619,13 @@ bool IsolateGroupReloadContext::Reload(bool force_reload,
       const auto& typed_data = ExternalTypedData::Handle(
           Z, ExternalTypedData::NewFinalizeWithFree(
                  const_cast<uint8_t*>(kernel_buffer), kernel_buffer_size));
-
       kernel_program = kernel::Program::ReadFromTypedData(typed_data);
     }
 
     ExternalTypedData& external_typed_data =
         ExternalTypedData::Handle(Z, kernel_program.get()->typed_data()->raw());
     IsolateGroupSource* source = Isolate::Current()->source();
-    Array& hot_reload_blobs = Array::Handle();
-    bool saved_external_typed_data = false;
-    if (source->hot_reload_blobs_ != nullptr) {
-      hot_reload_blobs = source->hot_reload_blobs_;
-
-      // Walk the array, and (if stuff was removed) compact and reuse the space.
-      // Note that the space has to be compacted as the ordering is important.
-      WeakProperty& weak_property = WeakProperty::Handle();
-      WeakProperty& weak_property_tmp = WeakProperty::Handle();
-      ExternalTypedData& existing_entry = ExternalTypedData::Handle(Z);
-      intptr_t next_entry_index = 0;
-      for (intptr_t i = 0; i < hot_reload_blobs.Length(); i++) {
-        weak_property ^= hot_reload_blobs.At(i);
-        if (weak_property.key() != ExternalTypedData::null()) {
-          if (i != next_entry_index) {
-            existing_entry = ExternalTypedData::RawCast(weak_property.key());
-            weak_property_tmp ^= hot_reload_blobs.At(next_entry_index);
-            weak_property_tmp.set_key(existing_entry);
-          }
-          next_entry_index++;
-        }
-      }
-      if (next_entry_index < hot_reload_blobs.Length()) {
-        // There's now space to re-use.
-        weak_property ^= hot_reload_blobs.At(next_entry_index);
-        weak_property.set_key(external_typed_data);
-        next_entry_index++;
-        saved_external_typed_data = true;
-      }
-      if (next_entry_index < hot_reload_blobs.Length()) {
-        ExternalTypedData& nullExternalTypedData = ExternalTypedData::Handle(Z);
-        while (next_entry_index < hot_reload_blobs.Length()) {
-          // Null out any extra spaces.
-          weak_property ^= hot_reload_blobs.At(next_entry_index);
-          weak_property.set_key(nullExternalTypedData);
-          next_entry_index++;
-        }
-      }
-    }
-    if (!saved_external_typed_data) {
-      const WeakProperty& weak_property =
-          WeakProperty::Handle(WeakProperty::New(Heap::kOld));
-      weak_property.set_key(external_typed_data);
-
-      intptr_t length =
-          hot_reload_blobs.IsNull() ? 0 : hot_reload_blobs.Length();
-      Array& new_array =
-          Array::Handle(Array::Grow(hot_reload_blobs, length + 1, Heap::kOld));
-      new_array.SetAt(length, weak_property);
-      source->hot_reload_blobs_ = new_array.raw();
-    }
-    source->num_hot_reloads_++;
+    source->add_loaded_blob(Z, external_typed_data);
 
     modified_libs_ = new (Z) BitVector(Z, num_old_libs_);
     kernel::KernelLoader::FindModifiedLibraries(