Revert "Reland "[vm/concurrency] Introduce concept of Isolate Groups""

This reverts commit 67ab3be.

Reason for revert: Causes non-deterministic failures on front-end bots on Windows with "Isolate creation failed" error. See https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8909292129328681248/+/steps/unit_tests/0/stdout

Original change's description:
> Reland "[vm/concurrency] Introduce concept of Isolate Groups"
> 
> An Isolate Group (IG) is a collection of isolates which were spawned from the
> same source. This allows the VM to:
> 
>   * have a guarantee that all isolates within one IG can safely exchange
>     structured objects (currently we rely on embedder for this
>     guarantee)
> 
>   * hot-reload all isolates together (currently we only reload one
>     isolate, leaving same-source isolates in inconsistent state)
> 
>   * make a shared heap for all isolates from the same IG, which paves
>     the way for faster communication and sharing of immutable objects.
> 
> All isolates within one IG will share the same IsolateGroupSource.
> 
> **Embedder changes**
> 
> This change makes breaking embedder API changes to support this new
> concept of Isolate Groups: The existing isolate lifecycle callbacks
> given to Dart_Initialize will become Isolate Group lifecycle callbacks.
> A new callback `initialize_isolate` callback will be added which can
> initialize a new isolate within an existing IG.
> 
> Existing embedders can be updated by performing the following renames
> 
>   Dart_CreateIsolate -> Dart_CreateIsolateGroup
>   Dart_IsolateCreateCallback -> Dart_IsolateGroupCreateCallback
>   Dart_IsolateCleanupCallback -> Dart_IsolateGroupShutdownCallback
>   Dart_CreateIsolateFromKernel -> Dart_CreateIsolateGroupFromKernel
>   Dart_CurrentIsolateData -> Dart_CurrentIsolateGroupData
>   Dart_IsolateData -> Dart_IsolateGroupData
>   Dart_GetNativeIsolateData -> Dart_GetNativeIsolateGroupData
>   Dart_InitializeParams.create -> Dart_InitializeParams.create_group
>   Dart_InitializeParams.cleanup -> Dart_InitializeParams.shutdown_group
>   Dart_InitializeParams.shutdown -> Dart_InitializeParams.shutdown_isolate
> 
> By default `Isolate.spawn` will cause the creation of a new IG.
> 
> Though an embedder can opt-into supporting multiple isolates within one IG by
> providing a callback to the newly added `Dart_InitializeParams.initialize_isolate`.
> The responsibility of this new callback is to initialize an existing
> isolate (which was setup by re-using source code from the spawning
> isolate - i.e. the one which used `Isolate.spawn`) by setting native
> resolvers, initializing global state, etc.
> 
> Issue #36648
> Issue #36097
> 
> Original review: https://dart-review.googlesource.com/c/sdk/+/105241
> 
> Difference to original review:
> 
>   * Give each isolate it's own [Loader] (for now)
>   * Sort classes during initialization for spawned isolates if app-jit is used (to match main isolate)
>   * Fix IsolateData memory leak if isolate startup fails
> 
> Change-Id: I98277d3d10fe275aa9b8a16b6bdd446bbea0b100
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/107506
> Commit-Queue: Martin Kustermann <[email protected]>
> Reviewed-by: Ryan Macnak <[email protected]>

[email protected],[email protected],[email protected]

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: Ia4e0f4f9fc317499d3570a371c5bdf9aed799e77
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108101
Reviewed-by: Vyacheslav Egorov <[email protected]>
Commit-Queue: Vyacheslav Egorov <[email protected]>

Author: mraleph

runtime/bin/dart_embedder_api_impl.cc

@@ -45,9 +45,9 @@ Dart_Isolate CreateKernelServiceIsolate(const IsolateCreationData& data,
                                         const uint8_t* buffer,
                                         intptr_t buffer_size,
                                         char** error) {
-  Dart_Isolate kernel_isolate = Dart_CreateIsolateGroupFromKernel(
+  Dart_Isolate kernel_isolate = Dart_CreateIsolateFromKernel(
       data.script_uri, data.main, buffer, buffer_size, data.flags,
-      data.isolate_group_data, data.isolate_data, error);
+      data.callback_data, error);
   if (kernel_isolate == nullptr) {
     return nullptr;
   }
@@ -78,9 +78,9 @@ Dart_Isolate CreateVmServiceIsolate(const IsolateCreationData& data,
   }
   data.flags->load_vmservice_library = true;
 
-  Dart_Isolate service_isolate = Dart_CreateIsolateGroupFromKernel(
+  Dart_Isolate service_isolate = Dart_CreateIsolateFromKernel(
       data.script_uri, data.main, kernel_buffer, kernel_buffer_size, data.flags,
-      data.isolate_group_data, data.isolate_data, error);
+      data.callback_data, error);
   if (service_isolate == nullptr) {
     return nullptr;
   }

runtime/bin/gen_snapshot.cc

@@ -745,25 +745,23 @@ static int CreateIsolateAndSnapshot(const CommandLineOptions& inputs) {
     isolate_flags.entry_points = no_entry_points;
   }
 
-  auto isolate_group_data =
-      new IsolateGroupData(nullptr, nullptr, nullptr, nullptr, false);
+  IsolateData* isolate_data = new IsolateData(NULL, NULL, NULL, NULL);
   Dart_Isolate isolate;
   char* error = NULL;
   if (isolate_snapshot_data == NULL) {
     // We need to capture the vmservice library in the core snapshot, so load it
     // in the main isolate as well.
     isolate_flags.load_vmservice_library = true;
-    isolate = Dart_CreateIsolateGroupFromKernel(
-        NULL, NULL, kernel_buffer, kernel_buffer_size, &isolate_flags,
-        isolate_group_data, /*isolate_data=*/nullptr, &error);
+    isolate = Dart_CreateIsolateFromKernel(NULL, NULL, kernel_buffer,
+                                           kernel_buffer_size, &isolate_flags,
+                                           isolate_data, &error);
   } else {
-    isolate = Dart_CreateIsolateGroup(NULL, NULL, isolate_snapshot_data,
-                                      isolate_snapshot_instructions, NULL, NULL,
-                                      &isolate_flags, isolate_group_data,
-                                      /*isolate_data=*/nullptr, &error);
+    isolate = Dart_CreateIsolate(NULL, NULL, isolate_snapshot_data,
+                                 isolate_snapshot_instructions, NULL, NULL,
+                                 &isolate_flags, isolate_data, &error);
   }
   if (isolate == NULL) {
-    delete isolate_group_data;
+    delete isolate_data;
     Syslog::PrintErr("%s\n", error);
     free(error);
     return kErrorExitCode;
@@ -785,9 +783,9 @@ static int CreateIsolateAndSnapshot(const CommandLineOptions& inputs) {
   // If the input dill file does not have a root library, then
   // Dart_LoadScript will error.
   //
-  // TODO(kernel): Dart_CreateIsolateGroupFromKernel should respect the root
-  // library in the kernel file, though this requires auditing the other
-  // loading paths in the embedders that had to work around this.
+  // TODO(kernel): Dart_CreateIsolateFromKernel should respect the root library
+  // in the kernel file, though this requires auditing the other loading paths
+  // in the embedders that had to work around this.
   result = Dart_SetRootLibrary(
       Dart_LoadLibraryFromKernel(kernel_buffer, kernel_buffer_size));
   CHECK_RESULT(result);

runtime/bin/isolate_data.cc

@@ -9,54 +9,45 @@
 namespace dart {
 namespace bin {
 
-IsolateGroupData::IsolateGroupData(const char* url,
-                                   const char* package_root,
-                                   const char* packages_file,
-                                   AppSnapshot* app_snapshot,
-                                   bool isolate_run_app_snapshot)
+IsolateData::IsolateData(const char* url,
+                         const char* package_root,
+                         const char* packages_file,
+                         AppSnapshot* app_snapshot)
     : script_url((url != NULL) ? strdup(url) : NULL),
       package_root(NULL),
+      packages_file(NULL),
+      loader_(NULL),
       app_snapshot_(app_snapshot),
       dependencies_(NULL),
       resolved_packages_config_(NULL),
       kernel_buffer_(NULL),
-      kernel_buffer_size_(0),
-      isolate_run_app_snapshot_(isolate_run_app_snapshot) {
+      kernel_buffer_size_(0) {
   if (package_root != NULL) {
     ASSERT(packages_file == NULL);
-    package_root = strdup(package_root);
+    this->package_root = strdup(package_root);
   } else if (packages_file != NULL) {
-    packages_file_ = strdup(packages_file);
+    this->packages_file = strdup(packages_file);
   }
 }
 
-IsolateGroupData::~IsolateGroupData() {
+void IsolateData::OnIsolateShutdown() {
+}
+
+IsolateData::~IsolateData() {
   free(script_url);
   script_url = NULL;
   free(package_root);
   package_root = NULL;
-  free(packages_file_);
-  packages_file_ = NULL;
+  free(packages_file);
+  packages_file = NULL;
   free(resolved_packages_config_);
   resolved_packages_config_ = NULL;
   kernel_buffer_ = NULL;
   kernel_buffer_size_ = 0;
+  delete app_snapshot_;
+  app_snapshot_ = NULL;
   delete dependencies_;
 }
 
-IsolateData::IsolateData(IsolateGroupData* isolate_group_data)
-    : isolate_group_data_(isolate_group_data),
-      loader_(nullptr),
-      packages_file_(nullptr) {
-  if (isolate_group_data->packages_file_ != nullptr) {
-    packages_file_ = strdup(isolate_group_data->packages_file_);
-  }
-}
-
-IsolateData::~IsolateData() {
-  free(packages_file_);
-  packages_file_ = nullptr;
-}
-
 }  // namespace bin
 }  // namespace dart

runtime/bin/isolate_data.h

@@ -28,54 +28,62 @@ class AppSnapshot;
 class EventHandler;
 class Loader;
 
-// Data associated with every isolate group in the standalone VM
+// Data associated with every isolate in the standalone VM
 // embedding. This is used to free external resources for each isolate
-// group when the isolate group shuts down.
-class IsolateGroupData {
+// when the isolate shuts down.
+class IsolateData {
  public:
-  IsolateGroupData(const char* url,
-                   const char* package_root,
-                   const char* packages_file,
-                   AppSnapshot* app_snapshot,
-                   bool isolate_run_app_snapshot);
-  ~IsolateGroupData();
+  IsolateData(const char* url,
+              const char* package_root,
+              const char* packages_file,
+              AppSnapshot* app_snapshot);
+  ~IsolateData();
 
   char* script_url;
   char* package_root;
+  char* packages_file;
 
   const std::shared_ptr<uint8_t>& kernel_buffer() const {
     return kernel_buffer_;
   }
 
   intptr_t kernel_buffer_size() const { return kernel_buffer_size_; }
 
-  // Associate the given kernel buffer with this IsolateGroupData without
-  // giving it ownership of the buffer.
+  // Associate the given kernel buffer with this IsolateData without giving it
+  // ownership of the buffer.
   void SetKernelBufferUnowned(uint8_t* buffer, intptr_t size) {
     ASSERT(kernel_buffer_.get() == NULL);
     kernel_buffer_ = std::shared_ptr<uint8_t>(buffer, FreeUnownedKernelBuffer);
     kernel_buffer_size_ = size;
   }
 
-  // Associate the given kernel buffer with this IsolateGroupData and give it
-  // ownership of the buffer. This IsolateGroupData is the first one to own the
+  // Associate the given kernel buffer with this IsolateData and give it
+  // ownership of the buffer. This IsolateData is the first one to own the
   // buffer.
   void SetKernelBufferNewlyOwned(uint8_t* buffer, intptr_t size) {
     ASSERT(kernel_buffer_.get() == NULL);
     kernel_buffer_ = std::shared_ptr<uint8_t>(buffer, free);
     kernel_buffer_size_ = size;
   }
 
-  // Associate the given kernel buffer with this IsolateGroupData and give it
+  // Associate the given kernel buffer with this IsolateData and give it
   // ownership of the buffer. The buffer is already owned by another
-  // IsolateGroupData.
+  // IsolateData.
   void SetKernelBufferAlreadyOwned(std::shared_ptr<uint8_t> buffer,
                                    intptr_t size) {
     ASSERT(kernel_buffer_.get() == NULL);
     kernel_buffer_ = std::move(buffer);
     kernel_buffer_size_ = size;
   }
 
+  void UpdatePackagesFile(const char* packages_file_) {
+    if (packages_file != NULL) {
+      free(packages_file);
+      packages_file = NULL;
+    }
+    packages_file = strdup(packages_file_);
+  }
+
   const char* resolved_packages_config() const {
     return resolved_packages_config_;
   }
@@ -88,54 +96,6 @@ class IsolateGroupData {
     resolved_packages_config_ = strdup(packages_config);
   }
 
-  MallocGrowableArray<char*>* dependencies() const { return dependencies_; }
-  void set_dependencies(MallocGrowableArray<char*>* deps) {
-    dependencies_ = deps;
-  }
-
-  bool RunFromAppSnapshot() const {
-    // If the main isolate is using an app snapshot the [app_snapshot_] pointer
-    // will be still nullptr (see main.cc:CreateIsolateGroupAndSetupHelper)
-    //
-    // Because of thus we have an additional boolean signaling whether the
-    // isolate was started from an app snapshot.
-    return app_snapshot_ != nullptr || isolate_run_app_snapshot_;
-  }
-
- private:
-  friend class IsolateData;  // For packages_file_
-
-  std::unique_ptr<AppSnapshot> app_snapshot_;
-  MallocGrowableArray<char*>* dependencies_;
-  char* resolved_packages_config_;
-  std::shared_ptr<uint8_t> kernel_buffer_;
-  intptr_t kernel_buffer_size_;
-  char* packages_file_ = nullptr;
-  bool isolate_run_app_snapshot_;
-
-  static void FreeUnownedKernelBuffer(uint8_t*) {}
-
-  DISALLOW_COPY_AND_ASSIGN(IsolateGroupData);
-};
-
-// Data associated with every isolate in the standalone VM
-// embedding. This is used to free external resources for each isolate
-// when the isolate shuts down.
-class IsolateData {
- public:
-  explicit IsolateData(IsolateGroupData* isolate_group_data);
-  ~IsolateData();
-
-  IsolateGroupData* isolate_group_data() const { return isolate_group_data_; }
-
-  void UpdatePackagesFile(const char* packages_file) {
-    if (packages_file != nullptr) {
-      free(packages_file_);
-      packages_file_ = nullptr;
-    }
-    packages_file_ = strdup(packages_file);
-  }
-
   // While loading a loader is associated with the isolate.
   bool HasLoader() const { return loader_ != NULL; }
   Loader* loader() const {
@@ -146,13 +106,22 @@ class IsolateData {
     ASSERT((loader_ == NULL) || (loader == NULL));
     loader_ = loader;
   }
+  MallocGrowableArray<char*>* dependencies() const { return dependencies_; }
+  void set_dependencies(MallocGrowableArray<char*>* deps) {
+    dependencies_ = deps;
+  }
 
-  const char* packages_file() const { return packages_file_; }
+  void OnIsolateShutdown();
 
  private:
-  IsolateGroupData* isolate_group_data_;
   Loader* loader_;
-  char* packages_file_;
+  AppSnapshot* app_snapshot_;
+  MallocGrowableArray<char*>* dependencies_;
+  char* resolved_packages_config_;
+  std::shared_ptr<uint8_t> kernel_buffer_;
+  intptr_t kernel_buffer_size_;
+
+  static void FreeUnownedKernelBuffer(uint8_t*) {}
 
   DISALLOW_COPY_AND_ASSIGN(IsolateData);
 };

runtime/bin/loader.cc

@@ -263,18 +263,18 @@ static bool PathContainsSeparator(const char* path) {
 
 void Loader::AddDependencyLocked(Loader* loader, const char* resolved_uri) {
   MallocGrowableArray<char*>* dependencies =
-      loader->isolate_group_data()->dependencies();
+      loader->isolate_data_->dependencies();
   if (dependencies == NULL) {
     return;
   }
   dependencies->Add(strdup(resolved_uri));
 }
 
 void Loader::ResolveDependenciesAsFilePaths() {
-  IsolateGroupData* isolate_group_data =
-      reinterpret_cast<IsolateGroupData*>(Dart_CurrentIsolateGroupData());
-  ASSERT(isolate_group_data != NULL);
-  MallocGrowableArray<char*>* dependencies = isolate_group_data->dependencies();
+  IsolateData* isolate_data =
+      reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
+  ASSERT(isolate_data != NULL);
+  MallocGrowableArray<char*>* dependencies = isolate_data->dependencies();
   if (dependencies == NULL) {
     return;
   }
@@ -454,15 +454,15 @@ bool Loader::ProcessQueueLocked(ProcessResult process_result) {
   return !hit_error;
 }
 
-void Loader::InitForSnapshot(const char* snapshot_uri,
-                             IsolateData* isolate_data) {
+void Loader::InitForSnapshot(const char* snapshot_uri) {
+  IsolateData* isolate_data =
+      reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
   ASSERT(isolate_data != NULL);
   ASSERT(!isolate_data->HasLoader());
   // Setup a loader. The constructor does a bunch of leg work.
   Loader* loader = new Loader(isolate_data);
   // Send the init message.
-  loader->Init(isolate_data->isolate_group_data()->package_root,
-               isolate_data->packages_file(),
+  loader->Init(isolate_data->package_root, isolate_data->packages_file,
                DartUtils::original_working_directory, snapshot_uri);
   // Destroy the loader. The destructor does a bunch of leg work.
   delete loader;
@@ -516,15 +516,15 @@ Dart_Handle Loader::SendAndProcessReply(intptr_t tag,
                                         Dart_Handle url,
                                         uint8_t** payload,
                                         intptr_t* payload_length) {
-  auto isolate_data = reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
+  IsolateData* isolate_data =
+      reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
   ASSERT(isolate_data != NULL);
   ASSERT(!isolate_data->HasLoader());
   Loader* loader = NULL;
 
   // Setup the loader. The constructor does a bunch of leg work.
   loader = new Loader(isolate_data);
-  loader->Init(isolate_data->isolate_group_data()->package_root,
-               isolate_data->packages_file(),
+  loader->Init(isolate_data->package_root, isolate_data->packages_file,
                DartUtils::original_working_directory, NULL);
   ASSERT(loader != NULL);
   ASSERT(isolate_data->HasLoader());
@@ -565,10 +565,6 @@ Dart_Handle Loader::ResolveAsFilePath(Dart_Handle url,
                              payload_length);
 }
 
-IsolateGroupData* Loader::isolate_group_data() {
-  return isolate_data_->isolate_group_data();
-}
-
 #if defined(DART_PRECOMPILED_RUNTIME)
 Dart_Handle Loader::LibraryTagHandler(Dart_LibraryTag tag,
                                       Dart_Handle library,
@@ -683,7 +679,8 @@ Dart_Handle Loader::LibraryTagHandler(Dart_LibraryTag tag,
     }
   }
 
-  auto isolate_data = reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
+  IsolateData* isolate_data =
+      reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
   ASSERT(isolate_data != NULL);
   if ((tag == Dart_kScriptTag) && Dart_IsString(library)) {
     // Update packages file for isolate.
@@ -710,8 +707,7 @@ Dart_Handle Loader::LibraryTagHandler(Dart_LibraryTag tag,
 
     // Setup the loader. The constructor does a bunch of leg work.
     loader = new Loader(isolate_data);
-    loader->Init(isolate_data->isolate_group_data()->package_root,
-                 isolate_data->packages_file(),
+    loader->Init(isolate_data->package_root, isolate_data->packages_file,
                  DartUtils::original_working_directory,
                  (tag == Dart_kScriptTag) ? url_string : NULL);
   } else {