From 5585ed99039efb3705025e1c58170cdb86af111b Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Tue, 1 Sep 2020 22:05:53 -0700 Subject: [PATCH] Revert "Create root isolate asynchronously (#20142)" (#20937) This reverts commit 95f2b72728ee7e51800f1784e458e45dac675b3a. --- runtime/runtime_controller.cc | 142 +++++++++++-------------------- runtime/runtime_controller.h | 13 +-- runtime/runtime_delegate.h | 2 - shell/common/engine.cc | 4 - shell/common/engine.h | 12 --- shell/common/engine_unittests.cc | 4 +- shell/common/shell.cc | 25 +----- shell/common/shell.h | 3 - 8 files changed, 56 insertions(+), 149 deletions(-) diff --git a/runtime/runtime_controller.cc b/runtime/runtime_controller.cc index 036553f4c0492..f66cfed778660 100644 --- a/runtime/runtime_controller.cc +++ b/runtime/runtime_controller.cc @@ -18,10 +18,7 @@ namespace flutter { RuntimeController::RuntimeController(RuntimeDelegate& client, TaskRunners p_task_runners) - : client_(client), - vm_(nullptr), - task_runners_(p_task_runners), - weak_factory_(this) {} + : client_(client), vm_(nullptr), task_runners_(p_task_runners) {} RuntimeController::RuntimeController( RuntimeDelegate& p_client, @@ -53,80 +50,48 @@ RuntimeController::RuntimeController( platform_data_(std::move(p_platform_data)), isolate_create_callback_(p_isolate_create_callback), isolate_shutdown_callback_(p_isolate_shutdown_callback), - persistent_isolate_data_(std::move(p_persistent_isolate_data)), - weak_factory_(this) { - // Create the root isolate as soon as the runtime controller is initialized, - // but not using a synchronous way to avoid blocking the platform thread a - // long time as it is waiting while creating `Shell` on that platform thread. + persistent_isolate_data_(std::move(p_persistent_isolate_data)) { + // Create the root isolate as soon as the runtime controller is initialized. // It will be run at a later point when the engine provides a run // configuration and then runs the isolate. - create_and_config_root_isolate_ = - std::async(std::launch::deferred, [self = weak_factory_.GetWeakPtr()]() { - if (!self) { - return; - } - - auto strong_root_isolate = - DartIsolate::CreateRootIsolate( - self->vm_->GetVMData()->GetSettings(), // - self->isolate_snapshot_, // - self->task_runners_, // - std::make_unique(self.get()), // - self->snapshot_delegate_, // - self->io_manager_, // - self->unref_queue_, // - self->image_decoder_, // - self->advisory_script_uri_, // - self->advisory_script_entrypoint_, // - nullptr, // - self->isolate_create_callback_, // - self->isolate_shutdown_callback_ // - ) - .lock(); - - FML_CHECK(strong_root_isolate) << "Could not create root isolate."; - - // The root isolate ivar is weak. - self->root_isolate_ = strong_root_isolate; - - strong_root_isolate->SetReturnCodeCallback([self](uint32_t code) { - if (!self) { - return; - } - - self->root_isolate_return_code_ = {true, code}; - }); - - if (auto* platform_configuration = - self->GetPlatformConfigurationIfAvailable()) { - tonic::DartState::Scope scope(strong_root_isolate); - platform_configuration->DidCreateIsolate(); - if (!self->FlushRuntimeStateToIsolate()) { - FML_DLOG(ERROR) << "Could not setup initial isolate state."; - } - } else { - FML_DCHECK(false) - << "RuntimeController created without window binding."; - } - - FML_DCHECK(Dart_CurrentIsolate() == nullptr); - - self->client_.OnRootIsolateCreated(); - return; - }); - - // We're still trying to create the root isolate as soon as possible here on - // the UI thread although it's deferred a little bit by - // std::async(std::launch::deferred, ...). So the callers of `GetRootIsolate` - // should get a quick return after this UI thread task. - task_runners_.GetUITaskRunner()->PostTask( - [self = weak_factory_.GetWeakPtr()]() { - if (!self) { - return; - } - - self->GetRootIsolate(); - }); + auto strong_root_isolate = + DartIsolate::CreateRootIsolate( + vm_->GetVMData()->GetSettings(), // + isolate_snapshot_, // + task_runners_, // + std::make_unique(this), // + snapshot_delegate_, // + io_manager_, // + unref_queue_, // + image_decoder_, // + p_advisory_script_uri, // + p_advisory_script_entrypoint, // + nullptr, // + isolate_create_callback_, // + isolate_shutdown_callback_ // + ) + .lock(); + + FML_CHECK(strong_root_isolate) << "Could not create root isolate."; + + // The root isolate ivar is weak. + root_isolate_ = strong_root_isolate; + + strong_root_isolate->SetReturnCodeCallback([this](uint32_t code) { + root_isolate_return_code_ = {true, code}; + }); + + if (auto* platform_configuration = GetPlatformConfigurationIfAvailable()) { + tonic::DartState::Scope scope(strong_root_isolate); + platform_configuration->DidCreateIsolate(); + if (!FlushRuntimeStateToIsolate()) { + FML_DLOG(ERROR) << "Could not setup initial isolate state."; + } + } else { + FML_DCHECK(false) << "RuntimeController created without window binding."; + } + + FML_DCHECK(Dart_CurrentIsolate() == nullptr); } RuntimeController::~RuntimeController() { @@ -142,8 +107,8 @@ RuntimeController::~RuntimeController() { } } -bool RuntimeController::IsRootIsolateRunning() { - std::shared_ptr root_isolate = GetRootIsolate().lock(); +bool RuntimeController::IsRootIsolateRunning() const { + std::shared_ptr root_isolate = root_isolate_.lock(); if (root_isolate) { return root_isolate->GetPhase() == DartIsolate::Phase::Running; } @@ -269,7 +234,7 @@ bool RuntimeController::ReportTimings(std::vector timings) { } bool RuntimeController::NotifyIdle(int64_t deadline) { - std::shared_ptr root_isolate = GetRootIsolate().lock(); + std::shared_ptr root_isolate = root_isolate_.lock(); if (!root_isolate) { return false; } @@ -326,7 +291,7 @@ bool RuntimeController::DispatchSemanticsAction(int32_t id, PlatformConfiguration* RuntimeController::GetPlatformConfigurationIfAvailable() { - std::shared_ptr root_isolate = GetRootIsolate().lock(); + std::shared_ptr root_isolate = root_isolate_.lock(); return root_isolate ? root_isolate->platform_configuration() : nullptr; } @@ -388,17 +353,17 @@ RuntimeController::ComputePlatformResolvedLocale( } Dart_Port RuntimeController::GetMainPort() { - std::shared_ptr root_isolate = GetRootIsolate().lock(); + std::shared_ptr root_isolate = root_isolate_.lock(); return root_isolate ? root_isolate->main_port() : ILLEGAL_PORT; } std::string RuntimeController::GetIsolateName() { - std::shared_ptr root_isolate = GetRootIsolate().lock(); + std::shared_ptr root_isolate = root_isolate_.lock(); return root_isolate ? root_isolate->debug_name() : ""; } bool RuntimeController::HasLivePorts() { - std::shared_ptr root_isolate = GetRootIsolate().lock(); + std::shared_ptr root_isolate = root_isolate_.lock(); if (!root_isolate) { return false; } @@ -407,20 +372,11 @@ bool RuntimeController::HasLivePorts() { } tonic::DartErrorHandleType RuntimeController::GetLastError() { - std::shared_ptr root_isolate = GetRootIsolate().lock(); + std::shared_ptr root_isolate = root_isolate_.lock(); return root_isolate ? root_isolate->GetLastError() : tonic::kNoError; } std::weak_ptr RuntimeController::GetRootIsolate() { - std::shared_ptr root_isolate = root_isolate_.lock(); - if (root_isolate) { - return root_isolate_; - } - - // Root isolate is not yet created, get it and do some configuration. - FML_DCHECK(create_and_config_root_isolate_.valid()); - create_and_config_root_isolate_.get(); - return root_isolate_; } diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h index 31c7238d4371e..18adbc2c1d720 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -5,7 +5,6 @@ #ifndef FLUTTER_RUNTIME_RUNTIME_CONTROLLER_H_ #define FLUTTER_RUNTIME_RUNTIME_CONTROLLER_H_ -#include #include #include @@ -341,7 +340,7 @@ class RuntimeController : public PlatformConfigurationClient { /// /// @return True if root isolate running, False otherwise. /// - virtual bool IsRootIsolateRunning(); + virtual bool IsRootIsolateRunning() const; //---------------------------------------------------------------------------- /// @brief Dispatch the specified platform message to running root @@ -423,10 +422,7 @@ class RuntimeController : public PlatformConfigurationClient { /// @brief Get a weak pointer to the root Dart isolate. This isolate may /// only be locked on the UI task runner. Callers use this /// accessor to transition to the root isolate to the running - /// phase. Note that it might take times if the isolate is not yet - /// created, which should be done in a subsequence task after - /// constructing `RuntimeController`, or it should get a quick - /// return otherwise. + /// phase. /// /// @return The root isolate reference. /// @@ -475,16 +471,11 @@ class RuntimeController : public PlatformConfigurationClient { std::string advisory_script_entrypoint_; std::function idle_notification_callback_; PlatformData platform_data_; - std::future create_and_config_root_isolate_; - // Note that `root_isolate_` is created asynchronously from the constructor of - // `RuntimeController`, be careful to use it directly while it might have not - // been created yet. Call `GetRootIsolate()` instead which guarantees that. std::weak_ptr root_isolate_; std::pair root_isolate_return_code_ = {false, 0}; const fml::closure isolate_create_callback_; const fml::closure isolate_shutdown_callback_; std::shared_ptr persistent_isolate_data_; - fml::WeakPtrFactory weak_factory_; PlatformConfiguration* GetPlatformConfigurationIfAvailable(); diff --git a/runtime/runtime_delegate.h b/runtime/runtime_delegate.h index 55978b4dbc39f..20059827b8150 100644 --- a/runtime/runtime_delegate.h +++ b/runtime/runtime_delegate.h @@ -32,8 +32,6 @@ class RuntimeDelegate { virtual FontCollection& GetFontCollection() = 0; - virtual void OnRootIsolateCreated() = 0; - virtual void UpdateIsolateDescription(const std::string isolate_name, int64_t isolate_port) = 0; diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 2560b3ba79ff5..315780f23346b 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -495,10 +495,6 @@ void Engine::HandlePlatformMessage(fml::RefPtr message) { } } -void Engine::OnRootIsolateCreated() { - delegate_.OnRootIsolateCreated(); -} - void Engine::UpdateIsolateDescription(const std::string isolate_name, int64_t isolate_port) { delegate_.UpdateIsolateDescription(isolate_name, isolate_port); diff --git a/shell/common/engine.h b/shell/common/engine.h index d083ece9c393a..d16ae0ffed2c7 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -185,15 +185,6 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { /// virtual void OnPreEngineRestart() = 0; - //-------------------------------------------------------------------------- - /// @brief Notifies the shell that the root isolate is created. - /// Currently, this information is to add to the service - /// protocol list of available root isolates running in the VM - /// and their names so that the appropriate isolate can be - /// selected in the tools for debugging and instrumentation. - /// - virtual void OnRootIsolateCreated() = 0; - //-------------------------------------------------------------------------- /// @brief Notifies the shell of the name of the root isolate and its /// port when that isolate is launched, restarted (in the @@ -821,9 +812,6 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { // |RuntimeDelegate| void HandlePlatformMessage(fml::RefPtr message) override; - // |RuntimeDelegate| - void OnRootIsolateCreated() override; - // |RuntimeDelegate| void UpdateIsolateDescription(const std::string isolate_name, int64_t isolate_port) override; diff --git a/shell/common/engine_unittests.cc b/shell/common/engine_unittests.cc index 6efb57b959b39..54f6f00d7a8f6 100644 --- a/shell/common/engine_unittests.cc +++ b/shell/common/engine_unittests.cc @@ -24,7 +24,6 @@ class MockDelegate : public Engine::Delegate { MOCK_METHOD1(OnEngineHandlePlatformMessage, void(fml::RefPtr)); MOCK_METHOD0(OnPreEngineRestart, void()); - MOCK_METHOD0(OnRootIsolateCreated, void()); MOCK_METHOD2(UpdateIsolateDescription, void(const std::string, int64_t)); MOCK_METHOD1(SetNeedsReportTimings, void(bool)); MOCK_METHOD1(ComputePlatformResolvedLocale, @@ -47,7 +46,6 @@ class MockRuntimeDelegate : public RuntimeDelegate { void(SemanticsNodeUpdates, CustomAccessibilityActionUpdates)); MOCK_METHOD1(HandlePlatformMessage, void(fml::RefPtr)); MOCK_METHOD0(GetFontCollection, FontCollection&()); - MOCK_METHOD0(OnRootIsolateCreated, void()); MOCK_METHOD2(UpdateIsolateDescription, void(const std::string, int64_t)); MOCK_METHOD1(SetNeedsReportTimings, void(bool)); MOCK_METHOD1(ComputePlatformResolvedLocale, @@ -59,7 +57,7 @@ class MockRuntimeController : public RuntimeController { public: MockRuntimeController(RuntimeDelegate& client, TaskRunners p_task_runners) : RuntimeController(client, p_task_runners) {} - MOCK_METHOD0(IsRootIsolateRunning, bool()); + MOCK_CONST_METHOD0(IsRootIsolateRunning, bool()); MOCK_METHOD1(DispatchPlatformMessage, bool(fml::RefPtr)); }; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index c0af1055bece1..61e190ba3f39e 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -562,6 +562,8 @@ bool Shell::Setup(std::unique_ptr platform_view, is_setup_ = true; + vm_->GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription()); + PersistentCache::GetCacheForProcess()->AddWorkerTaskRunner( task_runners_.GetIOTaskRunner()); @@ -1131,19 +1133,6 @@ void Shell::OnPreEngineRestart() { latch.Wait(); } -// |Engine::Delegate| -void Shell::OnRootIsolateCreated() { - auto description = GetServiceProtocolDescription(); - fml::TaskRunner::RunNowOrPostTask( - task_runners_.GetPlatformTaskRunner(), - [self = weak_factory_.GetWeakPtr(), - description = std::move(description)]() { - if (self) { - self->vm_->GetServiceProtocol()->AddHandler(self.get(), description); - } - }); -} - // |Engine::Delegate| void Shell::UpdateIsolateDescription(const std::string isolate_name, int64_t isolate_port) { @@ -1288,15 +1277,9 @@ bool Shell::HandleServiceProtocolMessage( // |ServiceProtocol::Handler| ServiceProtocol::Handler::Description Shell::GetServiceProtocolDescription() const { - FML_DCHECK(task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); - - if (!weak_engine_) { - return ServiceProtocol::Handler::Description(); - } - return { - weak_engine_->GetUIIsolateMainPort(), - weak_engine_->GetUIIsolateName(), + engine_->GetUIIsolateMainPort(), + engine_->GetUIIsolateName(), }; } diff --git a/shell/common/shell.h b/shell/common/shell.h index 95d6a05687a8b..dd09fad2ae7a6 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -513,9 +513,6 @@ class Shell final : public PlatformView::Delegate, // |Engine::Delegate| void OnPreEngineRestart() override; - // |Engine::Delegate| - void OnRootIsolateCreated() override; - // |Engine::Delegate| void UpdateIsolateDescription(const std::string isolate_name, int64_t isolate_port) override;