From e3f851ba8959dd1729aada116ccb68d509cad77f Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 19 Jan 2021 17:05:54 -0800 Subject: [PATCH 1/4] Started using Dart_CreateInGroup when using spawn on a release build. --- runtime/dart_isolate.cc | 58 +++++++++++++++++++------------ runtime/dart_isolate.h | 11 +++--- runtime/dart_isolate_unittests.cc | 16 +++++++++ runtime/dart_vm.cc | 5 ++- testing/run_tests.py | 29 +++++++++++++++- 5 files changed, 89 insertions(+), 30 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 9cb2fce5e91a6..b18aa2a3ca9bf 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -143,7 +143,8 @@ std::weak_ptr DartIsolate::CreateRunningRootIsolate( isolate_flags, // isolate_create_callback, // isolate_shutdown_callback, // - std::move(volatile_path_tracker) // + std::move(volatile_path_tracker), // + spawning_isolate // ) .lock(); @@ -251,23 +252,39 @@ std::weak_ptr 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* isolate_group_data, - std::shared_ptr* 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* + 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=*/nullptr, + /*cleanup_callback=*/nullptr, + /*child_isolate_data=*/isolate_data, + /*error=*/error); + }; + } else { + isolate_maker = [](std::shared_ptr* + isolate_group_data, + std::shared_ptr* 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); + }; + } + + 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(); @@ -992,10 +1009,7 @@ Dart_Isolate DartIsolate::CreateDartIsolateGroup( std::unique_ptr> isolate_data, Dart_IsolateFlags* flags, char** error, - std::function*, - std::shared_ptr*, - 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. diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index b4705ad9c7b76..de006125f03be 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -517,15 +517,18 @@ class DartIsolate : public UIDartState { Dart_IsolateFlags* flags, char** error); + typedef std::function*, + std::shared_ptr*, + Dart_IsolateFlags*, + char**)> + IsolateMaker; + static Dart_Isolate CreateDartIsolateGroup( std::unique_ptr> isolate_group_data, std::unique_ptr> isolate_data, Dart_IsolateFlags* flags, char** error, - std::function*, - std::shared_ptr*, - Dart_IsolateFlags*, - char**)> make_isolate); + const IsolateMaker& make_isolate); static bool InitializeIsolate(std::shared_ptr embedder_isolate, Dart_Isolate isolate, diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 3b25c02dec0ae..1437419e73360 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -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()); } diff --git a/runtime/dart_vm.cc b/runtime/dart_vm.cc index b23fce230f454..bdef46fd25cec 100644 --- a/runtime/dart_vm.cc +++ b/runtime/dart_vm.cc @@ -64,9 +64,8 @@ static const char* kDartLanguageArgs[] = { // clang-format on }; -static const char* kDartPrecompilationArgs[] = { - "--precompilation", -}; +static const char* kDartPrecompilationArgs[] = {"--precompilation", + "--enable-isolate-groups"}; FML_ALLOW_UNUSED_TYPE static const char* kDartWriteProtectCodeArgs[] = { diff --git a/testing/run_tests.py b/testing/run_tests.py index 9cdd1e63331da..74492f4686fc6 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -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) + + if release_runtime_out_dir != build_dir: + RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags) RunEngineExecutable(build_dir, 'tonic_unittests', filter, shuffle_flags) @@ -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 = [ From ca7b718451a74e20822d3cce412c279a6b4afe9c Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 22 Jan 2021 11:45:38 -0800 Subject: [PATCH 2/4] put try block around building the runtime_unittests --- testing/run_tests.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 74492f4686fc6..2fdba0e82921f 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -135,7 +135,11 @@ def RunCCTests(build_dir, filter): # TODO(tbd): Remove this explicit testing of release builds after lightweight # isolates are supported in JIT mode. - release_runtime_out_dir = EnsureReleaseRuntimeTestsAreBuilt() + release_runtime_out_dir = os.path.join(out_dir, "host_release") + try: + EnsureReleaseRuntimeTestsAreBuilt(release_runtime_out_dir) + except Exception: + print("Warning: Unable to build release runtime_unittests, this may cause execution of those tests to fail if they aren't already built.") RunEngineExecutable(release_runtime_out_dir, 'runtime_unittests', filter, shuffle_flags) if release_runtime_out_dir != build_dir: @@ -310,10 +314,8 @@ def EnsureJavaTestsAreBuilt(android_out_dir): RunCmd(gn_command, cwd=buildroot_dir) RunCmd(ninja_command, cwd=buildroot_dir) -def EnsureReleaseRuntimeTestsAreBuilt(variant="host_release"): +def EnsureReleaseRuntimeTestsAreBuilt(runtime_out_dir): """Builds a release build of the runtime tests.""" - runtime_out_dir = os.path.join(out_dir, variant) - ninja_command = [ 'autoninja', '-C', From 4b6376dae2562136ccbed2185cbce5519693011e Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 22 Jan 2021 13:16:11 -0800 Subject: [PATCH 3/4] reverted change to run_tests, tests are already run with the release build --- testing/run_tests.py | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 2fdba0e82921f..9cdd1e63331da 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -133,17 +133,7 @@ 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) - # TODO(tbd): Remove this explicit testing of release builds after lightweight - # isolates are supported in JIT mode. - release_runtime_out_dir = os.path.join(out_dir, "host_release") - try: - EnsureReleaseRuntimeTestsAreBuilt(release_runtime_out_dir) - except Exception: - print("Warning: Unable to build release runtime_unittests, this may cause execution of those tests to fail if they aren't already built.") - RunEngineExecutable(release_runtime_out_dir, 'runtime_unittests', filter, shuffle_flags) - - if release_runtime_out_dir != build_dir: - RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags) + RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags) RunEngineExecutable(build_dir, 'tonic_unittests', filter, shuffle_flags) @@ -314,25 +304,6 @@ def EnsureJavaTestsAreBuilt(android_out_dir): RunCmd(gn_command, cwd=buildroot_dir) RunCmd(ninja_command, cwd=buildroot_dir) -def EnsureReleaseRuntimeTestsAreBuilt(runtime_out_dir): - """Builds a release build of the runtime tests.""" - 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 = [ From f3e60f1e6d6115bb197697c66bb8f190bc899efd Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 22 Jan 2021 13:23:51 -0800 Subject: [PATCH 4/4] updated todos --- runtime/dart_isolate.cc | 2 ++ runtime/dart_isolate_unittests.cc | 2 +- runtime/dart_vm.cc | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index b18aa2a3ca9bf..bdcd42999bb44 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -254,6 +254,8 @@ std::weak_ptr DartIsolate::CreateRootIsolate( auto isolate_flags = flags.Get(); 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* diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 1437419e73360..bb6ada9e3e98d 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -137,7 +137,7 @@ TEST_F(DartIsolateTest, SpawnIsolate) { ASSERT_TRUE(spawn); ASSERT_EQ(spawn->GetPhase(), DartIsolate::Phase::Running); - // TODO(tbd): Remove conditional once isolate groups are supported by JIT. + // TODO(74520): Remove conditional once isolate groups are supported by JIT. if (DartVM::IsRunningPrecompiledCode()) { Dart_IsolateGroup isolate_group; { diff --git a/runtime/dart_vm.cc b/runtime/dart_vm.cc index bdef46fd25cec..331dfb82e2aa6 100644 --- a/runtime/dart_vm.cc +++ b/runtime/dart_vm.cc @@ -64,6 +64,8 @@ static const char* kDartLanguageArgs[] = { // clang-format on }; +// TODO(74520): Remove flag once isolate group work is completed (or add it to +// JIT mode). static const char* kDartPrecompilationArgs[] = {"--precompilation", "--enable-isolate-groups"};