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
fluttergithubbot 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 1 commit
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
58 changes: 36 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,39 @@ 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;
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 +1009,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(tbd): 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
5 changes: 2 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,8 @@ static const char* kDartLanguageArgs[] = {
// clang-format on
};

static const char* kDartPrecompilationArgs[] = {
"--precompilation",
};
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
29 changes: 28 additions & 1 deletion testing/run_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,13 @@ def RunCCTests(build_dir, filter):
# TODO(44614): Re-enable after https://github.com/flutter/flutter/issues/44614 has been addressed.
# RunEngineExecutable(build_dir, 'fml_unittests', filter, [ fml_unittests_filter ] + shuffle_flags)

RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags)
# TODO(tbd): Remove this explicit testing of release builds after lightweight
# isolates are supported in JIT mode.
release_runtime_out_dir = EnsureReleaseRuntimeTestsAreBuilt()
RunEngineExecutable(release_runtime_out_dir, 'runtime_unittests', filter, shuffle_flags)
Copy link
Member

@xster xster Jan 22, 2021

Choose a reason for hiding this comment

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

according to https://cs.opensource.google/flutter/recipes/+/master:recipes/engine.py;l=684?q=file:recipes%2Fengine.py, we might not have to do this

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.

We need run_tests.py able to run locally too. I would have expected the build to be a noop, but looks like the goma check might be happening before it releases it doesn't need to build something.

Copy link
Member

@xster xster Jan 22, 2021

Choose a reason for hiding this comment

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

in that case, wouldn't you do run_tests.py --type=engine --variant=host_release_unopt?

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.

I tried host_release_unopt I removed that after reading through the recipe, I think it's using host_release https://cs.opensource.google/flutter/recipes/+/master:recipes/engine.py;l=683?q=file:recipes%2Fengine.py

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.

I'm going to put a try block around the build so that it can continue to execution on the server, but hopefully still build locally.


if release_runtime_out_dir != build_dir:
RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags)

RunEngineExecutable(build_dir, 'tonic_unittests', filter, shuffle_flags)

Expand Down Expand Up @@ -304,6 +310,27 @@ def EnsureJavaTestsAreBuilt(android_out_dir):
RunCmd(gn_command, cwd=buildroot_dir)
RunCmd(ninja_command, cwd=buildroot_dir)

def EnsureReleaseRuntimeTestsAreBuilt(variant="host_release"):
"""Builds a release build of the runtime tests."""
runtime_out_dir = os.path.join(out_dir, variant)

ninja_command = [
'autoninja',
'-C',
runtime_out_dir,
'runtime_unittests'
]

if not os.path.exists(runtime_out_dir):
gn_command = [
os.path.join(buildroot_dir, 'flutter', 'tools', 'gn'),
'--runtime-mode=release',
'--no-lto',
]
RunCmd(gn_command, cwd=buildroot_dir)
RunCmd(ninja_command, cwd=buildroot_dir)
return runtime_out_dir

def EnsureIosTestsAreBuilt(ios_out_dir):
"""Builds the engine variant and the test dylib containing the XCTests"""
ninja_command = [
Expand Down