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

Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup #23549

Merged
gaaclarke merged 4 commits into from
Jan 13, 2021
Merged

Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup #23549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29d2a61
Did the plumbing refactor that allows us to call Dart_CreateIsolateIn…
gaaclarke Jan 8, 2021
1e15db8
responded to xiao's feedback
gaaclarke Jan 11, 2021
dc585c6
updated docstrings
gaaclarke Jan 12, 2021
f01c330
added unit test for DartIsolate::SpawnIsolate
gaaclarke Jan 12, 2021
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
86 changes: 72 additions & 14 deletions runtime/dart_isolate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,29 @@ Dart_IsolateFlags DartIsolate::Flags::Get() const {
return flags_;
}

std::weak_ptr<DartIsolate> DartIsolate::SpawnIsolate(
const Settings& settings,
std::unique_ptr<PlatformConfiguration> platform_configuration,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
fml::WeakPtr<HintFreedDelegate> hint_freed_delegate,
std::string advisory_script_uri,
std::string advisory_script_entrypoint,
Flags flags,
const fml::closure& isolate_create_callback,
const fml::closure& isolate_shutdown_callback,
std::optional<std::string> dart_entrypoint,
std::optional<std::string> dart_entrypoint_library,
std::unique_ptr<IsolateConfiguration> isolate_configration) const {
return CreateRunningRootIsolate(
settings, GetIsolateGroupData().GetIsolateSnapshot(), GetTaskRunners(),
std::move(platform_configuration), snapshot_delegate, hint_freed_delegate,
GetIOManager(), GetSkiaUnrefQueue(), GetImageDecoder(),
advisory_script_uri, advisory_script_entrypoint, flags,
isolate_create_callback, isolate_shutdown_callback, dart_entrypoint,
dart_entrypoint_library, std::move(isolate_configration),
GetVolatilePathTracker(), this);
}

std::weak_ptr<DartIsolate> DartIsolate::CreateRunningRootIsolate(
const Settings& settings,
fml::RefPtr<const DartSnapshot> isolate_snapshot,
Expand All @@ -91,7 +114,8 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRunningRootIsolate(
std::optional<std::string> dart_entrypoint,
std::optional<std::string> dart_entrypoint_library,
std::unique_ptr<IsolateConfiguration> isolate_configration,
std::shared_ptr<VolatilePathTracker> volatile_path_tracker) {
std::shared_ptr<VolatilePathTracker> volatile_path_tracker,
const DartIsolate* spawning_isolate) {
if (!isolate_snapshot) {
FML_LOG(ERROR) << "Invalid isolate snapshot.";
return {};
Expand Down Expand Up @@ -135,7 +159,8 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRunningRootIsolate(
});

if (isolate->GetPhase() != DartIsolate::Phase::LibrariesSetup) {
FML_LOG(ERROR) << "Root isolate was created in an incorrect phase.";
FML_LOG(ERROR) << "Root isolate was created in an incorrect phase: "
<< static_cast<int>(isolate->GetPhase());
return {};
}

Expand Down Expand Up @@ -190,7 +215,8 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
Flags flags,
const fml::closure& isolate_create_callback,
const fml::closure& isolate_shutdown_callback,
std::shared_ptr<VolatilePathTracker> volatile_path_tracker) {
std::shared_ptr<VolatilePathTracker> volatile_path_tracker,
const DartIsolate* spawning_isolate) {
TRACE_EVENT0("flutter", "DartIsolate::CreateRootIsolate");

// The child isolate preparer is null but will be set when the isolate is
Expand Down Expand Up @@ -223,13 +249,28 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
)));

DartErrorString error;
Dart_Isolate vm_isolate = nullptr;
auto isolate_flags = flags.Get();
Dart_Isolate vm_isolate = CreateDartIsolateGroup(
/// TODO(b/72025) This will be where we call Dart_CreateIsolateInGroup if
/// spawning_isolate != nullptr.
vm_isolate = CreateDartIsolateGroup(
std::move(isolate_group_data), std::move(isolate_data), &isolate_flags,
error.error());
error.error(),
[](std::shared_ptr<DartIsolateGroupData>* isolate_group_data,
std::shared_ptr<DartIsolate>* isolate_data, Dart_IsolateFlags* flags,
char** error) {
return Dart_CreateIsolateGroup(
(*isolate_group_data)->GetAdvisoryScriptURI().c_str(),
(*isolate_group_data)->GetAdvisoryScriptEntrypoint().c_str(),
(*isolate_group_data)->GetIsolateSnapshot()->GetDataMapping(),
(*isolate_group_data)
->GetIsolateSnapshot()
->GetInstructionsMapping(),
flags, isolate_group_data, isolate_data, error);
});

if (error) {
FML_LOG(ERROR) << "CreateDartIsolateGroup failed: " << error.str();
FML_LOG(ERROR) << "CreateRootIsolate failed: " << error.str();
}

if (vm_isolate == nullptr) {
Expand Down Expand Up @@ -809,6 +850,11 @@ DartIsolateGroupData& DartIsolate::GetIsolateGroupData() {
return **isolate_group_data;
}

const DartIsolateGroupData& DartIsolate::GetIsolateGroupData() const {
DartIsolate* non_const_this = const_cast<DartIsolate*>(this);
return non_const_this->GetIsolateGroupData();
}

// |Dart_IsolateGroupCreateCallback|
Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback(
const char* advisory_script_uri,
Expand Down Expand Up @@ -871,7 +917,19 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback(
nullptr))); // volatile path tracker

Dart_Isolate vm_isolate = CreateDartIsolateGroup(
std::move(isolate_group_data), std::move(isolate_data), flags, error);
std::move(isolate_group_data), std::move(isolate_data), flags, error,
[](std::shared_ptr<DartIsolateGroupData>* isolate_group_data,
std::shared_ptr<DartIsolate>* isolate_data, Dart_IsolateFlags* flags,
char** error) {
return Dart_CreateIsolateGroup(
(*isolate_group_data)->GetAdvisoryScriptURI().c_str(),
(*isolate_group_data)->GetAdvisoryScriptEntrypoint().c_str(),
(*isolate_group_data)->GetIsolateSnapshot()->GetDataMapping(),
(*isolate_group_data)
->GetIsolateSnapshot()
->GetInstructionsMapping(),
flags, isolate_group_data, isolate_data, error);
});

if (*error) {
FML_LOG(ERROR) << "CreateDartIsolateGroup failed: " << error;
Expand Down Expand Up @@ -933,16 +991,16 @@ Dart_Isolate DartIsolate::CreateDartIsolateGroup(
std::unique_ptr<std::shared_ptr<DartIsolateGroupData>> isolate_group_data,
std::unique_ptr<std::shared_ptr<DartIsolate>> isolate_data,
Dart_IsolateFlags* flags,
char** error) {
char** error,
std::function<Dart_Isolate(std::shared_ptr<DartIsolateGroupData>*,
std::shared_ptr<DartIsolate>*,
Dart_IsolateFlags*,
char**)> make_isolate) {
TRACE_EVENT0("flutter", "DartIsolate::CreateDartIsolateGroup");

// Create the Dart VM isolate and give it the embedder object as the baton.
Dart_Isolate isolate = Dart_CreateIsolateGroup(
(*isolate_group_data)->GetAdvisoryScriptURI().c_str(),
(*isolate_group_data)->GetAdvisoryScriptEntrypoint().c_str(),
(*isolate_group_data)->GetIsolateSnapshot()->GetDataMapping(),
(*isolate_group_data)->GetIsolateSnapshot()->GetInstructionsMapping(),
flags, isolate_group_data.get(), isolate_data.get(), error);
Dart_Isolate isolate =
make_isolate(isolate_group_data.get(), isolate_data.get(), flags, error);

if (isolate == nullptr) {
return nullptr;
Expand Down
48 changes: 43 additions & 5 deletions runtime/dart_isolate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include "flutter/lib/ui/io_manager.h"
#include "flutter/lib/ui/snapshot_delegate.h"
#include "flutter/lib/ui/ui_dart_state.h"
#include "flutter/lib/ui/volatile_path_tracker.h"
#include "flutter/lib/ui/window/platform_configuration.h"
#include "flutter/runtime/dart_snapshot.h"
#include "third_party/dart/runtime/include/dart_api.h"
Expand Down Expand Up @@ -207,7 +206,9 @@ class DartIsolate : public UIDartState {
/// for all isolate shutdowns
/// (including the children of the
/// root isolate).
///
/// @param[in] spawning_isolate The isolate that is spawning the
/// new isolate. See also
/// DartIsolate::SpawnIsolate.
/// @return A weak pointer to the root Dart isolate. The caller must
/// ensure that the isolate is not referenced for long periods of
/// time as it prevents isolate collection when the isolate
Expand All @@ -232,7 +233,37 @@ class DartIsolate : public UIDartState {
std::optional<std::string> dart_entrypoint,
std::optional<std::string> dart_entrypoint_library,
std::unique_ptr<IsolateConfiguration> isolate_configration,
std::shared_ptr<VolatilePathTracker> volatile_path_tracker);
std::shared_ptr<VolatilePathTracker> volatile_path_tracker,
const DartIsolate* spawning_isolate = nullptr);
Copy link
Member

@xster xster Jan 12, 2021

Choose a reason for hiding this comment

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

if you change the signature, update the method doc

Copy link
Member Author

@gaaclarke gaaclarke Jan 12, 2021

Choose a reason for hiding this comment

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

Done


//----------------------------------------------------------------------------
/// @brief Creates a running DartIsolate who shares as many resources as
Copy link
Member

@xster xster Jan 12, 2021

Choose a reason for hiding this comment

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

Creates a new running DartIsolate from the current DartIsolate

Copy link
Member Author

@gaaclarke gaaclarke Jan 12, 2021

Choose a reason for hiding this comment

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

I don't think that is much of an improvement. Hopefully the other edits cover the information you are trying to convey with that.

/// possible with the caller DartIsolate. This allows them to
/// occupy less memory together and to be created faster.
/// @details Shared components will be destroyed when the last live
/// DartIsolate is destroyed. SpawnIsolate can only be used to
/// create DartIsolates whose executable code is shared with the
/// calling DartIsolate.
/// @attention Only certain setups can take advantage of the most savings
/// currently, AOT specifically.
Copy link
Member

@xster xster Jan 12, 2021

Choose a reason for hiding this comment

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

Add the same disclosure for why it's a weak pointer, who owns it, performance considerations etc.

Copy link
Member Author

@gaaclarke gaaclarke Jan 12, 2021

Choose a reason for hiding this comment

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

done

/// @return A weak pointer to a new running DartIsolate. The caller must
/// ensure that the isolate is not referenced for long periods of
/// time as it prevents isolate collection when the isolate
/// terminates itself. The caller may also only use the isolate on
/// the thread on which the isolate was created.
std::weak_ptr<DartIsolate> SpawnIsolate(
const Settings& settings,
std::unique_ptr<PlatformConfiguration> platform_configuration,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
fml::WeakPtr<HintFreedDelegate> hint_freed_delegate,
std::string advisory_script_uri,
std::string advisory_script_entrypoint,
Flags flags,
const fml::closure& isolate_create_callback,
const fml::closure& isolate_shutdown_callback,
std::optional<std::string> dart_entrypoint,
std::optional<std::string> dart_entrypoint_library,
std::unique_ptr<IsolateConfiguration> isolate_configration) const;

// |UIDartState|
~DartIsolate() override;
Expand Down Expand Up @@ -433,7 +464,8 @@ class DartIsolate : public UIDartState {
Flags flags,
const fml::closure& isolate_create_callback,
const fml::closure& isolate_shutdown_callback,
std::shared_ptr<VolatilePathTracker> volatile_path_tracker);
std::shared_ptr<VolatilePathTracker> volatile_path_tracker,
const DartIsolate* spawning_isolate = nullptr);

DartIsolate(const Settings& settings,
TaskRunners task_runners,
Expand Down Expand Up @@ -463,6 +495,8 @@ class DartIsolate : public UIDartState {

DartIsolateGroupData& GetIsolateGroupData();

const DartIsolateGroupData& GetIsolateGroupData() const;

// |Dart_IsolateGroupCreateCallback|
static Dart_Isolate DartIsolateGroupCreateCallback(
const char* advisory_script_uri,
Expand All @@ -487,7 +521,11 @@ class DartIsolate : public UIDartState {
std::unique_ptr<std::shared_ptr<DartIsolateGroupData>> isolate_group_data,
std::unique_ptr<std::shared_ptr<DartIsolate>> isolate_data,
Dart_IsolateFlags* flags,
char** error);
char** error,
std::function<Dart_Isolate(std::shared_ptr<DartIsolateGroupData>*,
std::shared_ptr<DartIsolate>*,
Dart_IsolateFlags*,
char**)> make_isolate);

static bool InitializeIsolate(std::shared_ptr<DartIsolate> embedder_isolate,
Dart_Isolate isolate,
Expand Down
63 changes: 63 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 @@ -77,6 +77,69 @@ TEST_F(DartIsolateTest, RootIsolateCreationAndShutdown) {
ASSERT_TRUE(root_isolate->Shutdown());
}

TEST_F(DartIsolateTest, SpawnIsolate) {
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() //
);

auto isolate_configuration =
IsolateConfiguration::InferFromSettings(settings);

auto weak_isolate = DartIsolate::CreateRunningRootIsolate(
vm_data->GetSettings(), // settings
vm_data->GetIsolateSnapshot(), // isolate snapshot
std::move(task_runners), // task runners
nullptr, // window
{}, // snapshot delegate
{}, // hint freed delegate
{}, // io manager
{}, // unref queue
{}, // image decoder
"main.dart", // advisory uri
"main", // advisory entrypoint,
DartIsolate::Flags{}, // flags
settings.isolate_create_callback, // isolate create callback
settings.isolate_shutdown_callback, // isolate shutdown callback
"main", // dart entrypoint
std::nullopt, // dart entrypoint library
std::move(isolate_configuration), // isolate configuration
nullptr // Volatile path tracker
);
auto root_isolate = weak_isolate.lock();
ASSERT_TRUE(root_isolate);
ASSERT_EQ(root_isolate->GetPhase(), DartIsolate::Phase::Running);

auto spawn_configuration = IsolateConfiguration::InferFromSettings(settings);

auto weak_spawn = root_isolate->SpawnIsolate(
/*settings=*/vm_data->GetSettings(),
/*platform_configuration=*/nullptr,
/*snapshot_delegate=*/{},
/*hint_freed_delegate=*/{},
/*advisory_script_uri=*/"main.dart",
/*advisory_script_entrypoint=*/"main",
/*flags=*/DartIsolate::Flags{},
/*isolate_create_callback=*/settings.isolate_create_callback,
/*isolate_shutdown_callback=*/settings.isolate_shutdown_callback,
/*dart_entrypoint=*/"main",
/*dart_entrypoint_library=*/std::nullopt,
/*isolate_configration=*/std::move(spawn_configuration));
auto spawn = weak_spawn.lock();
ASSERT_TRUE(spawn);
ASSERT_EQ(spawn->GetPhase(), DartIsolate::Phase::Running);
ASSERT_TRUE(spawn->Shutdown());
ASSERT_TRUE(root_isolate->Shutdown());
}

TEST_F(DartIsolateTest, IsolateShutdownCallbackIsInIsolateScope) {
ASSERT_FALSE(DartVMRef::IsInstanceRunning());
auto settings = CreateSettingsForFixture();
Expand Down
14 changes: 12 additions & 2 deletions runtime/dart_vm_lifecycle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ class DartVMRef {
fml::RefPtr<DartSnapshot> vm_snapshot = nullptr,
fml::RefPtr<DartSnapshot> isolate_snapshot = nullptr);

DartVMRef(const DartVMRef&) = default;

DartVMRef(DartVMRef&&);

~DartVMRef();
Expand All @@ -53,11 +55,21 @@ class DartVMRef {
return vm_.get();
}

const DartVM* get() const {
FML_DCHECK(vm_);
return vm_.get();
}

DartVM* operator->() {
FML_DCHECK(vm_);
return vm_.get();
}

const DartVM* operator->() const {
FML_DCHECK(vm_);
return vm_.get();
}

DartVM* operator&() {
FML_DCHECK(vm_);
return vm_.get();
Expand All @@ -72,8 +84,6 @@ class DartVMRef {

// Only used by Dart Isolate to register itself with the VM.
static DartVM* GetRunningVM();

FML_DISALLOW_COPY_AND_ASSIGN(DartVMRef);
};

} // namespace flutter
Expand Down
19 changes: 19 additions & 0 deletions runtime/runtime_controller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,25 @@ RuntimeController::RuntimeController(
persistent_isolate_data_(std::move(p_persistent_isolate_data)),
volatile_path_tracker_(std::move(p_volatile_path_tracker)) {}

std::unique_ptr<RuntimeController> RuntimeController::Spawn(
RuntimeDelegate& client,
std::string advisory_script_uri,
std::string advisory_script_entrypoint,
const std::function<void(int64_t)>& idle_notification_callback,
const fml::closure& isolate_create_callback,
const fml::closure& isolate_shutdown_callback,
std::shared_ptr<const fml::Mapping> persistent_isolate_data) const {
auto result = std::make_unique<RuntimeController>(
client, vm_, isolate_snapshot_, task_runners_, snapshot_delegate_,
hint_freed_delegate_, io_manager_, unref_queue_, image_decoder_,
advisory_script_uri, advisory_script_entrypoint,
idle_notification_callback, platform_data_, isolate_create_callback,
isolate_shutdown_callback, persistent_isolate_data,
volatile_path_tracker_);
result->spawning_isolate_ = root_isolate_;
return result;
}

RuntimeController::~RuntimeController() {
FML_DCHECK(Dart_CurrentIsolate() == nullptr);
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
Expand Down
Loading