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

Started using Dart_CreateInGroup when using spawn on a release build #23782

Merged
merged 4 commits into from
Jan 22, 2021
Merged

Started using Dart_CreateInGroup when using spawn on a release build #23782

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
e3f851b
Started using Dart_CreateInGroup when using spawn on a release build.
gaaclarke Jan 20, 2021
ca7b718
put try block around building the runtime_unittests
gaaclarke Jan 22, 2021
4b6376d
reverted change to run_tests, tests are already run with the release …
gaaclarke Jan 22, 2021
f3e60f1
updated todos
gaaclarke Jan 22, 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
60 changes: 38 additions & 22 deletions runtime/dart_isolate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRunningRootIsolate(
isolate_flags, //
isolate_create_callback, //
isolate_shutdown_callback, //
std::move(volatile_path_tracker) //
std::move(volatile_path_tracker), //
spawning_isolate //
)
.lock();

Expand Down Expand Up @@ -251,23 +252,41 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
DartErrorString error;
Dart_Isolate vm_isolate = nullptr;
auto isolate_flags = flags.Get();
/// 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(),
[](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);
});

IsolateMaker isolate_maker;
// TODO(74520): Remove IsRunningPrecompiledCode conditional once isolate
// groups are supported by JIT.
if (spawning_isolate && DartVM::IsRunningPrecompiledCode()) {
isolate_maker = [spawning_isolate](
std::shared_ptr<DartIsolateGroupData>*
isolate_group_data,
std::shared_ptr<DartIsolate>* isolate_data,
Dart_IsolateFlags* flags, char** error) {
return Dart_CreateIsolateInGroup(
/*group_member=*/spawning_isolate->isolate(),
/*name=*/(*isolate_group_data)->GetAdvisoryScriptEntrypoint().c_str(),
/*shutdown_callback=*/nullptr,
/*cleanup_callback=*/nullptr,
/*child_isolate_data=*/isolate_data,
/*error=*/error);
};
} else {
isolate_maker = [](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);
Copy link
Member

@chinmaygarde chinmaygarde Jan 20, 2021

Choose a reason for hiding this comment

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

In case of error, the error message must be printed to the log (like elsewhere in this TU). Also, can you make sure we don't have to free the error message buffer? The Dart APIs are a bit inconsistent about what to do with the error buffer. Something you have to free it otherwise its a small leak.

Copy link
Member Author

@gaaclarke gaaclarke Jan 22, 2021

Choose a reason for hiding this comment

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

The tonic class DartErrorString is taking care of that here, it was a bit harder to follow before. It should be more clear now.

};
}

vm_isolate = CreateDartIsolateGroup(std::move(isolate_group_data),
std::move(isolate_data), &isolate_flags,
error.error(), isolate_maker);

if (error) {
FML_LOG(ERROR) << "CreateRootIsolate failed: " << error.str();
Expand Down Expand Up @@ -992,10 +1011,7 @@ Dart_Isolate DartIsolate::CreateDartIsolateGroup(
std::unique_ptr<std::shared_ptr<DartIsolate>> isolate_data,
Dart_IsolateFlags* flags,
char** error,
std::function<Dart_Isolate(std::shared_ptr<DartIsolateGroupData>*,
std::shared_ptr<DartIsolate>*,
Dart_IsolateFlags*,
char**)> make_isolate) {
const DartIsolate::IsolateMaker& make_isolate) {
TRACE_EVENT0("flutter", "DartIsolate::CreateDartIsolateGroup");

// Create the Dart VM isolate and give it the embedder object as the baton.
Expand Down
11 changes: 7 additions & 4 deletions runtime/dart_isolate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,15 +517,18 @@ class DartIsolate : public UIDartState {
Dart_IsolateFlags* flags,
char** error);

typedef std::function<Dart_Isolate(std::shared_ptr<DartIsolateGroupData>*,
std::shared_ptr<DartIsolate>*,
Dart_IsolateFlags*,
char**)>
IsolateMaker;

static Dart_Isolate 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,
std::function<Dart_Isolate(std::shared_ptr<DartIsolateGroupData>*,
std::shared_ptr<DartIsolate>*,
Dart_IsolateFlags*,
char**)> make_isolate);
const IsolateMaker& make_isolate);

static bool InitializeIsolate(std::shared_ptr<DartIsolate> embedder_isolate,
Dart_Isolate isolate,
Expand Down
16 changes: 16 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 @@ -136,6 +136,22 @@ TEST_F(DartIsolateTest, SpawnIsolate) {
auto spawn = weak_spawn.lock();
ASSERT_TRUE(spawn);
ASSERT_EQ(spawn->GetPhase(), DartIsolate::Phase::Running);

// TODO(74520): Remove conditional once isolate groups are supported by JIT.
if (DartVM::IsRunningPrecompiledCode()) {
Dart_IsolateGroup isolate_group;
{
auto isolate_scope = tonic::DartIsolateScope(root_isolate->isolate());
isolate_group = Dart_CurrentIsolateGroup();
}
{
auto isolate_scope = tonic::DartIsolateScope(root_isolate->isolate());
Dart_IsolateGroup spawn_isolate_group = Dart_CurrentIsolateGroup();
ASSERT_TRUE(isolate_group != nullptr);
ASSERT_EQ(isolate_group, spawn_isolate_group);
}
}

ASSERT_TRUE(spawn->Shutdown());
ASSERT_TRUE(root_isolate->Shutdown());
}
Expand Down
7 changes: 4 additions & 3 deletions runtime/dart_vm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,10 @@ static const char* kDartLanguageArgs[] = {
// clang-format on
};

static const char* kDartPrecompilationArgs[] = {
"--precompilation",
};
// TODO(74520): Remove flag once isolate group work is completed (or add it to
// JIT mode).
static const char* kDartPrecompilationArgs[] = {"--precompilation",
"--enable-isolate-groups"};
Copy link
Member Author

@gaaclarke gaaclarke Jan 22, 2021

Choose a reason for hiding this comment

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

@mkustermann Is there a risk to turning this on for users that don't ever call Dart_CreateIsolateInGroup? I worry about affecting the stability of users who don't use these experimental APIs.


FML_ALLOW_UNUSED_TYPE
static const char* kDartWriteProtectCodeArgs[] = {
Expand Down