Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Avoid reloading the kernel snapshot when spawning an isolate in the same group #48478

Merged
auto-submit merged 9 commits into from
Dec 1, 2023
Merged

Avoid reloading the kernel snapshot when spawning an isolate in the same group #48478

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2ac3196
Avoid reloading the kernel snapshot when spawning an isolate in the s…
dnfield Nov 29, 2023
7496e88
Aaron review
dnfield Nov 29, 2023
8c6ab06
missing file
dnfield Nov 29, 2023
24811e3
fix condition
dnfield Nov 29, 2023
c9ac2c5
avoid leaking isolate group data
dnfield Nov 30, 2023
f202786
lint fix
dnfield Nov 30, 2023
1b95db1
Update runtime/dart_isolate.h
dnfield Nov 30, 2023
5c912bf
unique_ptr copiable lambda
dnfield Nov 30, 2023
5b20ee0
less crashy
dnfield Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions runtime/dart_isolate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,12 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
)));

auto isolate_data = std::make_unique<std::shared_ptr<DartIsolate>>(
std::shared_ptr<DartIsolate>(new DartIsolate(settings, // settings
true, // is_root_isolate
context // context
)));
std::shared_ptr<DartIsolate>(new DartIsolate(
settings, // settings
true, // is_root_isolate
context, // context
!!spawning_isolate // spawn_in_group
Comment thread
gaaclarke marked this conversation as resolved.
Outdated
)));

DartErrorString error;
Dart_Isolate vm_isolate = nullptr;
Expand Down Expand Up @@ -276,7 +278,8 @@ std::weak_ptr<DartIsolate> 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,
Expand All @@ -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;
}

Expand Down Expand Up @@ -548,13 +552,13 @@ bool DartIsolate::LoadKernel(const std::shared_ptr<const fml::Mapping>& 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());

Expand Down Expand Up @@ -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;
Expand Down
6 changes: 5 additions & 1 deletion runtime/dart_isolate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -28,6 +29,7 @@ namespace flutter {
class DartVM;
class DartIsolateGroupData;
class IsolateConfiguration;
enum class IsolateLaunchType;
Comment thread
gaaclarke marked this conversation as resolved.
Outdated

//------------------------------------------------------------------------------
/// @brief Represents an instance of a live isolate. An isolate is a
Expand Down Expand Up @@ -412,6 +414,7 @@ class DartIsolate : public UIDartState {
fml::RefPtr<fml::TaskRunner> 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<DartIsolate> CreateRootIsolate(
const Settings& settings,
Expand All @@ -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);
Comment thread
gaaclarke marked this conversation as resolved.
Outdated

//----------------------------------------------------------------------------
/// @brief Initializes the given (current) isolate.
Expand Down
95 changes: 95 additions & 0 deletions runtime/dart_isolate_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

@gaaclarke gaaclarke Nov 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please capture a unique_ptr instead of a raw pointer.

@dnfield dnfield Nov 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't, because mapping is not copiable.

@gaaclarke gaaclarke Nov 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can manually copy them (as noted above).

@dnfield dnfield Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want to do this in this patch. The fixtures are set up to do file mappings, and if I do something like that here it will be one lone place where the fixture loading has to change.

The whole point of this test is that loading them twice shouldn't happen. I've updated the test so that it will abort instead of segfaulting in this case.

@gaaclarke gaaclarke Nov 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that's fine. You can still make this conform to the google style guide by capturing the unique_ptr and making your closure mutable.

    settings.application_kernels =
        [&get_kernel_count,
         mapping = std::move(mappings.front())]() mutable -> Mappings {
      ASSERT_TRUE(mapping);  // <-- This assurt will fail if it's called twice.
      get_kernel_count++;
      if (get_kernel_count > 1) {
        FML_LOG(ERROR)
            << "Unexpectedly got more than one call for the kernel mapping.";
        abort();
      }
      std::vector<std::unique_ptr<const fml::Mapping>> kernel_mappings;
      std::unique_ptr<const fml::Mapping> emplace_mapping;
      mapping.swap(emplace_mapping);
      kernel_mappings.emplace_back(std::move(emplace_mapping));
      return kernel_mappings;
    };

You might have to make the closure copiable. I can't remember those rules off the top of my head.

edit: updated to use a swap to be safe.

@gaaclarke gaaclarke Nov 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that's not quite safe. You have to use a std::swap or set it to null after the move, sorry.

get_kernel_count++;
std::vector<std::unique_ptr<const fml::Mapping>> kernel_mappings;
kernel_mappings.emplace_back(std::unique_ptr<const fml::Mapping>(mapping));
Comment thread
gaaclarke marked this conversation as resolved.
Outdated
return kernel_mappings;
};

std::shared_ptr<DartIsolate> 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
Comment thread
gaaclarke marked this conversation as resolved.
Outdated
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

Expand Down
8 changes: 7 additions & 1 deletion runtime/isolate_configuration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<const fml::Mapping> kernel)
: kernel_(std::move(kernel)) {}
Expand Down Expand Up @@ -202,12 +203,17 @@ PrepareKernelMappings(const std::vector<std::string>& kernel_pieces_paths,
std::unique_ptr<IsolateConfiguration> IsolateConfiguration::InferFromSettings(
const Settings& settings,
const std::shared_ptr<AssetManager>& asset_manager,
const fml::RefPtr<fml::TaskRunner>& io_worker) {
const fml::RefPtr<fml::TaskRunner>& io_worker,
IsolateLaunchType launch_type) {
Comment thread
gaaclarke marked this conversation as resolved.
// 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());
}
Expand Down
18 changes: 17 additions & 1 deletion runtime/isolate_configuration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -57,14 +68,19 @@ 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`.
///
[[nodiscard]] static std::unique_ptr<IsolateConfiguration> InferFromSettings(
const Settings& settings,
const std::shared_ptr<AssetManager>& asset_manager = nullptr,
const fml::RefPtr<fml::TaskRunner>& io_worker = nullptr);
const fml::RefPtr<fml::TaskRunner>& io_worker = nullptr,
IsolateLaunchType launch_type = IsolateLaunchType::kNewGroup);

//----------------------------------------------------------------------------
/// @brief Creates an AOT isolate configuration using snapshot symbols
Expand Down