From 2ac3196a9ea94680d084ebe4ce0bba50ea03ec7a Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 28 Nov 2023 16:25:02 -0800 Subject: [PATCH 1/9] Avoid reloading the kernel snapshot when spawning an isolate in the same group --- runtime/dart_isolate.cc | 28 +++++---- runtime/dart_isolate.h | 6 +- runtime/dart_isolate_unittests.cc | 95 +++++++++++++++++++++++++++++++ runtime/isolate_configuration.cc | 8 ++- runtime/isolate_configuration.h | 18 +++++- 5 files changed, 140 insertions(+), 15 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 910b0d4580ed8..7953c5b99b3a1 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -211,10 +211,12 @@ std::weak_ptr DartIsolate::CreateRootIsolate( ))); auto isolate_data = std::make_unique>( - std::shared_ptr(new DartIsolate(settings, // settings - true, // is_root_isolate - context // context - ))); + std::shared_ptr(new DartIsolate( + settings, // settings + true, // is_root_isolate + context, // context + !!spawning_isolate // spawn_in_group + ))); DartErrorString error; Dart_Isolate vm_isolate = nullptr; @@ -276,7 +278,8 @@ std::weak_ptr DartIsolate::CreateRootIsolate( DartIsolate::DartIsolate(const Settings& settings, bool is_root_isolate, - const UIDartState::Context& context) + const UIDartState::Context& context, + bool spawn_in_group) : UIDartState(settings.task_observer_add, settings.task_observer_remove, settings.log_tag, @@ -287,7 +290,8 @@ DartIsolate::DartIsolate(const Settings& settings, context), may_insecurely_connect_to_all_domains_( settings.may_insecurely_connect_to_all_domains), - domain_network_policy_(settings.domain_network_policy) { + domain_network_policy_(settings.domain_network_policy), + spawn_in_group_(spawn_in_group) { phase_ = Phase::Uninitialized; } @@ -548,13 +552,13 @@ bool DartIsolate::LoadKernel(const std::shared_ptr& mapping, return false; } - if (!mapping || mapping->GetSize() == 0) { - return false; - } - tonic::DartState::Scope scope(this); - if (!child_isolate) { + if (!child_isolate && !spawn_in_group_) { + if (!mapping || mapping->GetSize() == 0) { + return false; + } + // Use root library provided by kernel in favor of one provided by snapshot. Dart_SetRootLibrary(Dart_Null()); @@ -923,7 +927,7 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback( }); if (*error) { - FML_LOG(ERROR) << "CreateDartIsolateGroup failed: " << error; + FML_LOG(ERROR) << "CreateDartIsolateGroup failed: " << *error; } return vm_isolate; diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index 04e413a68bb29..cbd7b8a5a55c8 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -20,6 +20,7 @@ #include "flutter/lib/ui/ui_dart_state.h" #include "flutter/lib/ui/window/platform_configuration.h" #include "flutter/runtime/dart_snapshot.h" +#include "runtime/isolate_configuration.h" #include "third_party/dart/runtime/include/dart_api.h" #include "third_party/tonic/dart_state.h" @@ -28,6 +29,7 @@ namespace flutter { class DartVM; class DartIsolateGroupData; class IsolateConfiguration; +enum class IsolateLaunchType; //------------------------------------------------------------------------------ /// @brief Represents an instance of a live isolate. An isolate is a @@ -412,6 +414,7 @@ class DartIsolate : public UIDartState { fml::RefPtr message_handling_task_runner_; const bool may_insecurely_connect_to_all_domains_; std::string domain_network_policy_; + bool spawn_in_group_; static std::weak_ptr CreateRootIsolate( const Settings& settings, @@ -425,7 +428,8 @@ class DartIsolate : public UIDartState { DartIsolate(const Settings& settings, bool is_root_isolate, - const UIDartState::Context& context); + const UIDartState::Context& context, + bool spawn_in_group = false); //---------------------------------------------------------------------------- /// @brief Initializes the given (current) isolate. diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 2a299f3150c2f..aa117bfd1af18 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -592,6 +592,101 @@ TEST_F(DartIsolateTest, DartPluginRegistrantIsCalled) { ASSERT_EQ(messages[0], "_PluginRegistrant.register() was called"); } +TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + auto settings = CreateSettingsForFixture(); + auto vm_ref = DartVMRef::Create(settings); + ASSERT_TRUE(vm_ref); + auto vm_data = vm_ref.GetVMData(); + ASSERT_TRUE(vm_data); + TaskRunners task_runners(GetCurrentTestName(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner() // + ); + + ASSERT_TRUE(settings.application_kernels); + auto mappings = settings.application_kernels(); + ASSERT_EQ(mappings.size(), 1u); + + size_t get_kernel_count = 0u; + // This feels a little brittle, but the alternative seems to be making + // DartIsolate have virtual methods so it can be mocked or exposing weird + // test-only API on IsolateConfiguration. + settings.application_kernels = + [&get_kernel_count, mapping = mappings.front().release()]() -> Mappings { + get_kernel_count++; + std::vector> kernel_mappings; + kernel_mappings.emplace_back(std::unique_ptr(mapping)); + return kernel_mappings; + }; + + std::shared_ptr root_isolate; + { + auto isolate_configuration = + IsolateConfiguration::InferFromSettings(settings); + + UIDartState::Context context(task_runners); + context.advisory_script_uri = "main.dart"; + context.advisory_script_entrypoint = "main"; + auto weak_isolate = DartIsolate::CreateRunningRootIsolate( + vm_data->GetSettings(), // settings + vm_data->GetIsolateSnapshot(), // isolate snapshot + nullptr, // platform configuration + DartIsolate::Flags{}, // flags + nullptr, // root_isolate_create_callback + settings.isolate_create_callback, // isolate create callback + settings.isolate_shutdown_callback, // isolate shutdown callback + "main", // dart entrypoint + std::nullopt, // dart entrypoint library + {}, // dart entrypoint arguments + std::move(isolate_configuration), // isolate configuration + context // engine context + ); + root_isolate = weak_isolate.lock(); + } + ASSERT_TRUE(root_isolate); + ASSERT_EQ(root_isolate->GetPhase(), DartIsolate::Phase::Running); + ASSERT_EQ(get_kernel_count, 1u); + + { + auto isolate_configuration = IsolateConfiguration::InferFromSettings( + settings, // + /*asset_manager=*/nullptr, // + /*io_worker=*/nullptr, // + /*launch_type=*/IsolateLaunchType::kExistingGroup // + ); + + UIDartState::Context context(task_runners); + context.advisory_script_uri = "main.dart"; + context.advisory_script_entrypoint = "main"; + auto weak_isolate = DartIsolate::CreateRunningRootIsolate( + vm_data->GetSettings(), // settings + vm_data->GetIsolateSnapshot(), // isolate snapshot + nullptr, // platform configuration + DartIsolate::Flags{}, // flags + nullptr, // root_isolate_create_callback + settings.isolate_create_callback, // isolate create callback + settings.isolate_shutdown_callback, // isolate shutdown callback + "main", // dart entrypoint + std::nullopt, // dart entrypoint library + {}, // dart entrypoint arguments + std::move(isolate_configuration), // isolate configuration + context, // engine context + root_isolate.get() // spawning_isolate + ); + auto spawned_isolate = weak_isolate.lock(); + ASSERT_TRUE(spawned_isolate); + ASSERT_EQ(spawned_isolate->GetPhase(), DartIsolate::Phase::Running); + ASSERT_EQ(get_kernel_count, 1u); + + ASSERT_TRUE(spawned_isolate->Shutdown()); + } + + ASSERT_TRUE(root_isolate->Shutdown()); +} + } // namespace testing } // namespace flutter diff --git a/runtime/isolate_configuration.cc b/runtime/isolate_configuration.cc index 54eb6c35ee3c4..ffbc28e035496 100644 --- a/runtime/isolate_configuration.cc +++ b/runtime/isolate_configuration.cc @@ -43,6 +43,7 @@ class AppSnapshotIsolateConfiguration final : public IsolateConfiguration { class KernelIsolateConfiguration : public IsolateConfiguration { public: + // The kernel mapping may be nullptr if reusing the group's loaded kernel. explicit KernelIsolateConfiguration( std::unique_ptr kernel) : kernel_(std::move(kernel)) {} @@ -202,12 +203,17 @@ PrepareKernelMappings(const std::vector& kernel_pieces_paths, std::unique_ptr IsolateConfiguration::InferFromSettings( const Settings& settings, const std::shared_ptr& asset_manager, - const fml::RefPtr& io_worker) { + const fml::RefPtr& io_worker, + IsolateLaunchType launch_type) { // Running in AOT mode. if (DartVM::IsRunningPrecompiledCode()) { return CreateForAppSnapshot(); } + if (launch_type == IsolateLaunchType::kExistingGroup) { + return CreateForKernel(nullptr); + } + if (settings.application_kernels) { return CreateForKernelList(settings.application_kernels()); } diff --git a/runtime/isolate_configuration.h b/runtime/isolate_configuration.h index 46f9c9368c2d3..a4c809ee9fbea 100644 --- a/runtime/isolate_configuration.h +++ b/runtime/isolate_configuration.h @@ -19,6 +19,17 @@ namespace flutter { +/// Describes whether the isolate is part of a group or not. +/// +/// If the isolate is part of a group, it avoids reloading the kernel snapshot. +enum class IsolateLaunchType { + /// The isolate is launched as a solo isolate or to start a new group. + kNewGroup, + /// The isolate is launched as part of a group, and avoids reloading the + /// kernel snapshot. + kExistingGroup, +}; + //------------------------------------------------------------------------------ /// @brief An isolate configuration is a collection of snapshots and asset /// managers that the engine will use to configure the isolate @@ -57,6 +68,10 @@ class IsolateConfiguration { /// @param[in] io_worker An optional IO worker. Specify `nullptr` if a /// worker should not be used or one is not /// available. + /// @param[in] launch_type Whether the isolate is launching to form a new + /// group or as part of an existing group. If it is + /// part of an existing group, the isolate will + /// reuse resources if it can. /// /// @return An isolate configuration if one can be inferred from the /// settings. If not, returns `nullptr`. @@ -64,7 +79,8 @@ class IsolateConfiguration { [[nodiscard]] static std::unique_ptr InferFromSettings( const Settings& settings, const std::shared_ptr& asset_manager = nullptr, - const fml::RefPtr& io_worker = nullptr); + const fml::RefPtr& io_worker = nullptr, + IsolateLaunchType launch_type = IsolateLaunchType::kNewGroup); //---------------------------------------------------------------------------- /// @brief Creates an AOT isolate configuration using snapshot symbols From 7496e88e437c9002841b523c761e65bc1bca9317 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 29 Nov 2023 07:40:25 -0800 Subject: [PATCH 2/9] Aaron review --- runtime/dart_isolate.cc | 15 ++--- runtime/dart_isolate.h | 1 - runtime/dart_isolate_unittests.cc | 107 ++++++++++++++++-------------- 3 files changed, 65 insertions(+), 58 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 7953c5b99b3a1..37d9b3cbdb9a7 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -212,11 +212,10 @@ std::weak_ptr DartIsolate::CreateRootIsolate( auto isolate_data = std::make_unique>( std::shared_ptr(new DartIsolate( - settings, // settings - true, // is_root_isolate - context, // context - !!spawning_isolate // spawn_in_group - ))); + /*settings=*/settings, + /*is_root_isolate=*/true, + /*context=*/context, + /*is_spawning_in_group=*/!!spawning_isolate))); DartErrorString error; Dart_Isolate vm_isolate = nullptr; @@ -279,7 +278,7 @@ std::weak_ptr DartIsolate::CreateRootIsolate( DartIsolate::DartIsolate(const Settings& settings, bool is_root_isolate, const UIDartState::Context& context, - bool spawn_in_group) + bool is_spawning_in_group) : UIDartState(settings.task_observer_add, settings.task_observer_remove, settings.log_tag, @@ -291,7 +290,7 @@ DartIsolate::DartIsolate(const Settings& settings, may_insecurely_connect_to_all_domains_( settings.may_insecurely_connect_to_all_domains), domain_network_policy_(settings.domain_network_policy), - spawn_in_group_(spawn_in_group) { + is_spawning_in_group_(is_spawning_in_group) { phase_ = Phase::Uninitialized; } @@ -554,7 +553,7 @@ bool DartIsolate::LoadKernel(const std::shared_ptr& mapping, tonic::DartState::Scope scope(this); - if (!child_isolate && !spawn_in_group_) { + if (!child_isolate && !is_spawning_in_group_) { if (!mapping || mapping->GetSize() == 0) { return false; } diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index cbd7b8a5a55c8..b187052d2d6a7 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -29,7 +29,6 @@ namespace flutter { class DartVM; class DartIsolateGroupData; class IsolateConfiguration; -enum class IsolateLaunchType; //------------------------------------------------------------------------------ /// @brief Represents an instance of a live isolate. An isolate is a diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index aa117bfd1af18..0bf9b848cd79a 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -606,21 +606,30 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { GetCurrentTaskRunner() // ); - ASSERT_TRUE(settings.application_kernels); - auto mappings = settings.application_kernels(); - ASSERT_EQ(mappings.size(), 1u); - size_t get_kernel_count = 0u; - // This feels a little brittle, but the alternative seems to be making - // DartIsolate have virtual methods so it can be mocked or exposing weird - // test-only API on IsolateConfiguration. - settings.application_kernels = - [&get_kernel_count, mapping = mappings.front().release()]() -> Mappings { - get_kernel_count++; - std::vector> kernel_mappings; - kernel_mappings.emplace_back(std::unique_ptr(mapping)); - return kernel_mappings; - }; + if (DartVM::IsRunningPrecompiledCode()) { + ASSERT_TRUE(settings.application_kernels); + auto mappings = settings.application_kernels(); + ASSERT_EQ(mappings.size(), 1u); + + // This feels a little brittle, but the alternative seems to be making + // DartIsolate have virtual methods so it can be mocked or exposing weird + // test-only API on IsolateConfiguration. + settings.application_kernels = + [&get_kernel_count, + mapping = mappings.front().release()]() -> Mappings { + get_kernel_count++; + if (get_kernel_count > 1) { + FML_LOG(ERROR) + << "Unexpectedly got more than one call for the kernel mapping."; + abort(); + } + std::vector> kernel_mappings; + kernel_mappings.emplace_back( + std::unique_ptr(mapping)); + return kernel_mappings; + }; + } std::shared_ptr root_isolate; { @@ -631,56 +640,56 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { context.advisory_script_uri = "main.dart"; context.advisory_script_entrypoint = "main"; auto weak_isolate = DartIsolate::CreateRunningRootIsolate( - vm_data->GetSettings(), // settings - vm_data->GetIsolateSnapshot(), // isolate snapshot - nullptr, // platform configuration - DartIsolate::Flags{}, // flags - nullptr, // root_isolate_create_callback - settings.isolate_create_callback, // isolate create callback - settings.isolate_shutdown_callback, // isolate shutdown callback - "main", // dart entrypoint - std::nullopt, // dart entrypoint library - {}, // dart entrypoint arguments - std::move(isolate_configuration), // isolate configuration - context // engine context - ); + /*settings=*/vm_data->GetSettings(), + /*isolate_snapshot=*/vm_data->GetIsolateSnapshot(), + /*platform_configuration=*/nullptr, + /*flags=*/DartIsolate::Flags{}, + /*root_isolate_create_callback=*/nullptr, + /*isolate_create_callback=*/settings.isolate_create_callback, + /*isolate_shutdown_callback=*/settings.isolate_shutdown_callback, + /*dart_entrypoint=*/"main", + /*dart_entrypoint_library=*/std::nullopt, + /*dart_entrypoint_arguments=*/{}, + /*isolate_configuration=*/std::move(isolate_configuration), + /*context=*/context); root_isolate = weak_isolate.lock(); } ASSERT_TRUE(root_isolate); ASSERT_EQ(root_isolate->GetPhase(), DartIsolate::Phase::Running); - ASSERT_EQ(get_kernel_count, 1u); + if (DartVM::IsRunningPrecompiledCode()) { + ASSERT_EQ(get_kernel_count, 1u); + } { auto isolate_configuration = IsolateConfiguration::InferFromSettings( - settings, // - /*asset_manager=*/nullptr, // - /*io_worker=*/nullptr, // - /*launch_type=*/IsolateLaunchType::kExistingGroup // - ); + /*settings=*/settings, + /*asset_manager=*/nullptr, + /*io_worker=*/nullptr, + /*launch_type=*/IsolateLaunchType::kExistingGroup); UIDartState::Context context(task_runners); context.advisory_script_uri = "main.dart"; context.advisory_script_entrypoint = "main"; auto weak_isolate = DartIsolate::CreateRunningRootIsolate( - vm_data->GetSettings(), // settings - vm_data->GetIsolateSnapshot(), // isolate snapshot - nullptr, // platform configuration - DartIsolate::Flags{}, // flags - nullptr, // root_isolate_create_callback - settings.isolate_create_callback, // isolate create callback - settings.isolate_shutdown_callback, // isolate shutdown callback - "main", // dart entrypoint - std::nullopt, // dart entrypoint library - {}, // dart entrypoint arguments - std::move(isolate_configuration), // isolate configuration - context, // engine context - root_isolate.get() // spawning_isolate - ); + /*settings=*/vm_data->GetSettings(), + /*isolate_snapshot=*/vm_data->GetIsolateSnapshot(), + /*platform_configuration=*/nullptr, + /*flags=*/DartIsolate::Flags{}, + /*root_isolate_create_callback=*/nullptr, + /*isolate_create_callback=*/settings.isolate_create_callback, + /*isolate_shutdown_callback=*/settings.isolate_shutdown_callback, + /*dart_entrypoint=*/"main", + /*dart_entrypoint_library=*/std::nullopt, + /*dart_entrypoint_arguments=*/{}, + /*isolate_configuration=*/std::move(isolate_configuration), + /*context=*/context, + /*spawning_isolate=*/root_isolate.get()); auto spawned_isolate = weak_isolate.lock(); ASSERT_TRUE(spawned_isolate); ASSERT_EQ(spawned_isolate->GetPhase(), DartIsolate::Phase::Running); - ASSERT_EQ(get_kernel_count, 1u); - + if (DartVM::IsRunningPrecompiledCode()) { + ASSERT_EQ(get_kernel_count, 1u); + } ASSERT_TRUE(spawned_isolate->Shutdown()); } From 8c6ab06b3efc25175bddcb917d25fa73963d2aa6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 29 Nov 2023 07:54:48 -0800 Subject: [PATCH 3/9] missing file --- runtime/dart_isolate.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index b187052d2d6a7..f53eb717f9b01 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -413,7 +413,7 @@ class DartIsolate : public UIDartState { fml::RefPtr message_handling_task_runner_; const bool may_insecurely_connect_to_all_domains_; std::string domain_network_policy_; - bool spawn_in_group_; + bool is_spawning_in_group_; static std::weak_ptr CreateRootIsolate( const Settings& settings, @@ -428,7 +428,7 @@ class DartIsolate : public UIDartState { DartIsolate(const Settings& settings, bool is_root_isolate, const UIDartState::Context& context, - bool spawn_in_group = false); + bool is_spawning_in_group = false); //---------------------------------------------------------------------------- /// @brief Initializes the given (current) isolate. From 24811e335e5bba72b75f8ad8a6edb126fc91f110 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 29 Nov 2023 08:12:15 -0800 Subject: [PATCH 4/9] fix condition --- runtime/dart_isolate_unittests.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 0bf9b848cd79a..a0bff0c3a8c10 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -607,7 +607,7 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { ); size_t get_kernel_count = 0u; - if (DartVM::IsRunningPrecompiledCode()) { + if (!DartVM::IsRunningPrecompiledCode()) { ASSERT_TRUE(settings.application_kernels); auto mappings = settings.application_kernels(); ASSERT_EQ(mappings.size(), 1u); @@ -656,7 +656,7 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { } ASSERT_TRUE(root_isolate); ASSERT_EQ(root_isolate->GetPhase(), DartIsolate::Phase::Running); - if (DartVM::IsRunningPrecompiledCode()) { + if (!DartVM::IsRunningPrecompiledCode()) { ASSERT_EQ(get_kernel_count, 1u); } @@ -687,7 +687,7 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { auto spawned_isolate = weak_isolate.lock(); ASSERT_TRUE(spawned_isolate); ASSERT_EQ(spawned_isolate->GetPhase(), DartIsolate::Phase::Running); - if (DartVM::IsRunningPrecompiledCode()) { + if (!DartVM::IsRunningPrecompiledCode()) { ASSERT_EQ(get_kernel_count, 1u); } ASSERT_TRUE(spawned_isolate->Shutdown()); From c9ac2c5d6a70803b78563e15093da2cde8e1bbd0 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 30 Nov 2023 11:03:06 -0800 Subject: [PATCH 5/9] avoid leaking isolate group data --- runtime/dart_isolate.cc | 65 ++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 37d9b3cbdb9a7..6bde83da9790c 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -196,19 +196,8 @@ std::weak_ptr DartIsolate::CreateRootIsolate( const DartIsolate* spawning_isolate) { TRACE_EVENT0("flutter", "DartIsolate::CreateRootIsolate"); - // The child isolate preparer is null but will be set when the isolate is - // being prepared to run. - auto isolate_group_data = - std::make_unique>( - std::shared_ptr(new DartIsolateGroupData( - settings, // settings - std::move(isolate_snapshot), // isolate snapshot - context.advisory_script_uri, // advisory URI - context.advisory_script_entrypoint, // advisory entrypoint - nullptr, // child isolate preparer - isolate_create_callback, // isolate create callback - isolate_shutdown_callback // isolate shutdown callback - ))); + // Only needed if this is the main isolate for the group. + std::unique_ptr> isolate_group_data; auto isolate_data = std::make_unique>( std::shared_ptr(new DartIsolate( @@ -223,24 +212,40 @@ std::weak_ptr DartIsolate::CreateRootIsolate( IsolateMaker isolate_maker; if (spawning_isolate) { - isolate_maker = [spawning_isolate]( - std::shared_ptr* - isolate_group_data, - std::shared_ptr* isolate_data, - Dart_IsolateFlags* flags, char** error) { - return Dart_CreateIsolateInGroup( - /*group_member=*/spawning_isolate->isolate(), - /*name=*/(*isolate_group_data)->GetAdvisoryScriptEntrypoint().c_str(), - /*shutdown_callback=*/ - reinterpret_cast( - DartIsolate::SpawnIsolateShutdownCallback), - /*cleanup_callback=*/ - reinterpret_cast( - DartIsolateCleanupCallback), - /*child_isolate_data=*/isolate_data, - /*error=*/error); - }; + isolate_maker = + [spawning_isolate]( + std::shared_ptr* isolate_group_data, + std::shared_ptr* isolate_data, + Dart_IsolateFlags* flags, char** error) { + return Dart_CreateIsolateInGroup( + /*group_member=*/spawning_isolate->isolate(), + /*name=*/ + spawning_isolate->GetIsolateGroupData() + .GetAdvisoryScriptEntrypoint() + .c_str(), + /*shutdown_callback=*/ + reinterpret_cast( + DartIsolate::SpawnIsolateShutdownCallback), + /*cleanup_callback=*/ + reinterpret_cast( + DartIsolateCleanupCallback), + /*child_isolate_data=*/isolate_data, + /*error=*/error); + }; } else { + // The child isolate preparer is null but will be set when the isolate is + // being prepared to run. + isolate_group_data = + std::make_unique>( + std::shared_ptr(new DartIsolateGroupData( + settings, // settings + std::move(isolate_snapshot), // isolate snapshot + context.advisory_script_uri, // advisory URI + context.advisory_script_entrypoint, // advisory entrypoint + nullptr, // child isolate preparer + isolate_create_callback, // isolate create callback + isolate_shutdown_callback // isolate shutdown callback + ))); isolate_maker = [](std::shared_ptr* isolate_group_data, std::shared_ptr* isolate_data, From f2027861ff5ca4055d17f79b1ebe73935d295317 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 30 Nov 2023 11:21:55 -0800 Subject: [PATCH 6/9] lint fix --- runtime/dart_isolate_unittests.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index a0bff0c3a8c10..f999a1a16b204 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -649,7 +649,7 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { /*isolate_shutdown_callback=*/settings.isolate_shutdown_callback, /*dart_entrypoint=*/"main", /*dart_entrypoint_library=*/std::nullopt, - /*dart_entrypoint_arguments=*/{}, + /*dart_entrypoint_args=*/{}, /*isolate_configuration=*/std::move(isolate_configuration), /*context=*/context); root_isolate = weak_isolate.lock(); @@ -680,7 +680,7 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { /*isolate_shutdown_callback=*/settings.isolate_shutdown_callback, /*dart_entrypoint=*/"main", /*dart_entrypoint_library=*/std::nullopt, - /*dart_entrypoint_arguments=*/{}, + /*dart_entrypoint_args=*/{}, /*isolate_configuration=*/std::move(isolate_configuration), /*context=*/context, /*spawning_isolate=*/root_isolate.get()); From 1b95db14d627e42f3fed6d8b7a574b8eabeb8567 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 30 Nov 2023 13:23:21 -0800 Subject: [PATCH 7/9] Update runtime/dart_isolate.h Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com> --- runtime/dart_isolate.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index f53eb717f9b01..0cc5e90345b5d 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -413,7 +413,7 @@ class DartIsolate : public UIDartState { fml::RefPtr message_handling_task_runner_; const bool may_insecurely_connect_to_all_domains_; std::string domain_network_policy_; - bool is_spawning_in_group_; + const bool is_spawning_in_group_; static std::weak_ptr CreateRootIsolate( const Settings& settings, From 5c912bf91a4b33784047547402aa196baca39284 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 30 Nov 2023 13:49:43 -0800 Subject: [PATCH 8/9] unique_ptr copiable lambda --- runtime/dart_isolate_unittests.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index f999a1a16b204..c063f095f4e90 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -615,9 +615,12 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { // This feels a little brittle, but the alternative seems to be making // DartIsolate have virtual methods so it can be mocked or exposing weird // test-only API on IsolateConfiguration. - settings.application_kernels = - [&get_kernel_count, - mapping = mappings.front().release()]() -> Mappings { + settings + .application_kernels = fml::MakeCopyable([&get_kernel_count, + mapping = std::move( + mappings + .front())]() mutable + -> Mappings { get_kernel_count++; if (get_kernel_count > 1) { FML_LOG(ERROR) @@ -625,10 +628,9 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { abort(); } std::vector> kernel_mappings; - kernel_mappings.emplace_back( - std::unique_ptr(mapping)); + kernel_mappings.emplace_back(std::move(mapping)); return kernel_mappings; - }; + }); } std::shared_ptr root_isolate; From 5b20ee01c63bcc1fcb4123b1ebb523607cc29e87 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 30 Nov 2023 14:04:16 -0800 Subject: [PATCH 9/9] less crashy --- runtime/dart_isolate_unittests.cc | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index c063f095f4e90..b8a42860ae7e0 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -615,22 +615,19 @@ TEST_F(DartIsolateTest, SpawningAnIsolateDoesNotReloadKernel) { // This feels a little brittle, but the alternative seems to be making // DartIsolate have virtual methods so it can be mocked or exposing weird // test-only API on IsolateConfiguration. - settings - .application_kernels = fml::MakeCopyable([&get_kernel_count, - mapping = std::move( - mappings - .front())]() mutable - -> Mappings { - get_kernel_count++; - if (get_kernel_count > 1) { - FML_LOG(ERROR) - << "Unexpectedly got more than one call for the kernel mapping."; - abort(); - } - std::vector> kernel_mappings; - kernel_mappings.emplace_back(std::move(mapping)); - return kernel_mappings; - }); + settings.application_kernels = fml::MakeCopyable( + [&get_kernel_count, + mapping = std::move(mappings.front())]() mutable -> Mappings { + get_kernel_count++; + EXPECT_EQ(get_kernel_count, 1u) + << "Unexpectedly got more than one call for the kernel mapping."; + EXPECT_TRUE(mapping); + std::vector> kernel_mappings; + if (mapping) { + kernel_mappings.emplace_back(std::move(mapping)); + } + return kernel_mappings; + }); } std::shared_ptr root_isolate;