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

Create root isolate asynchronously #20142

Merged
merged 1 commit into from
Sep 1, 2020
Merged

Create root isolate asynchronously #20142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
142 changes: 93 additions & 49 deletions runtime/runtime_controller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ namespace flutter {

RuntimeController::RuntimeController(RuntimeDelegate& client,
TaskRunners p_task_runners)
: client_(client), vm_(nullptr), task_runners_(p_task_runners) {}
: client_(client),
vm_(nullptr),
task_runners_(p_task_runners),
weak_factory_(this) {}

RuntimeController::RuntimeController(
RuntimeDelegate& p_client,
Expand Down Expand Up @@ -50,48 +53,80 @@ 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)) {
// Create the root isolate as soon as the runtime controller is initialized.
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.
// It will be run at a later point when the engine provides a run
// configuration and then runs the isolate.
auto strong_root_isolate =
DartIsolate::CreateRootIsolate(
vm_->GetVMData()->GetSettings(), //
isolate_snapshot_, //
task_runners_, //
std::make_unique<PlatformConfiguration>(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);
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<PlatformConfiguration>(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;
Copy link
Member

Choose a reason for hiding this comment

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

The API as originally designed expects the root isolate to be ready when the runtime controller has been created (this assumption is prevalent throughout the engine). Now, a race has been introduced with no way of telling from the caller about when the root isolate is ready (or the other ivars).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it breaks the assumption but also makes sure the root isolate must be ready before running.


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();
Copy link
Contributor

@liyuqian liyuqian Aug 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: add a comment here that 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.

This comment might also be help for the GetRootIsolate declaration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

});
}

RuntimeController::~RuntimeController() {
Expand All @@ -107,8 +142,8 @@ RuntimeController::~RuntimeController() {
}
}

bool RuntimeController::IsRootIsolateRunning() const {
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
bool RuntimeController::IsRootIsolateRunning() {
std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

Relying on GetRootIsolate().lock() seems to be fragile as one can easily write a root_isolate_.lock() in the future and didn't realize it can cause some issue.

Besides, even if GetRootIsolate().lock() is enforced everywhere, the async speedup may easily be broken if someone calls GetRootIsolate() or some methods that calls it in the wrong place. I believe that's why this PR moves GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription()); to another place.

(#18225 doesn't seem to have this problem.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Access to root_isolate_ directly may escape from creating the isolate, this fragility is a price of this pr. Since it is a private variable, the affection scope is inside RuntimeController and it might be acceptable in my personal opinion. I'll add some comments for root_isolate_ to point it out, is there any better workaround?

If someone calls GetRootIsolate(), it means the root isolate is required right now, then it is necessary to create it in a synchronous way if the isolate is not created yet.
Another reason of moving GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription()); is that RuntimeController should be accessed only on the UI thread, while originally it was running inside Shell::Setup() on the platform thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another reason of moving GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription()); is that RuntimeController should be accessed only on the UI thread, while originally it was running inside Shell::Setup() on the platform thread.

Good catch, I guess we'll want to fix this regardless if we're going to adopt the bigger solution. We'll have more discussions about whether to accept the price of this pr.

if (root_isolate) {
return root_isolate->GetPhase() == DartIsolate::Phase::Running;
}
Expand Down Expand Up @@ -234,7 +269,7 @@ bool RuntimeController::ReportTimings(std::vector<int64_t> timings) {
}

bool RuntimeController::NotifyIdle(int64_t deadline) {
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
if (!root_isolate) {
return false;
}
Expand Down Expand Up @@ -291,7 +326,7 @@ bool RuntimeController::DispatchSemanticsAction(int32_t id,

PlatformConfiguration*
RuntimeController::GetPlatformConfigurationIfAvailable() {
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
return root_isolate ? root_isolate->platform_configuration() : nullptr;
}

Expand Down Expand Up @@ -353,17 +388,17 @@ RuntimeController::ComputePlatformResolvedLocale(
}

Dart_Port RuntimeController::GetMainPort() {
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
return root_isolate ? root_isolate->main_port() : ILLEGAL_PORT;
}

std::string RuntimeController::GetIsolateName() {
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
return root_isolate ? root_isolate->debug_name() : "";
}

bool RuntimeController::HasLivePorts() {
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
if (!root_isolate) {
return false;
}
Expand All @@ -372,11 +407,20 @@ bool RuntimeController::HasLivePorts() {
}

tonic::DartErrorHandleType RuntimeController::GetLastError() {
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
std::shared_ptr<DartIsolate> root_isolate = GetRootIsolate().lock();
return root_isolate ? root_isolate->GetLastError() : tonic::kNoError;
}

std::weak_ptr<DartIsolate> RuntimeController::GetRootIsolate() {
std::shared_ptr<DartIsolate> root_isolate = root_isolate_.lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously root_isolate_ was private. Now GetRootIsolate is public. I wonder if there's any specific reason to expose it publicly? Its usage seems to be still completely internal. If possible, we'd like to minimize our public API surface.

Copy link
Member Author

Choose a reason for hiding this comment

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

GetRootIsolate() is originally public. Engine calls this API to prepare, launch isolate, and do some callbacks. But seems these processes can be withdraw back into RuntimeController and then this API can be marked private.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed it as I originally that GetRootIsolate wasn't there. Then it's fine to keep using the public GetRootIsolatehere.

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_;
}

Expand Down
13 changes: 11 additions & 2 deletions runtime/runtime_controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef FLUTTER_RUNTIME_RUNTIME_CONTROLLER_H_
#define FLUTTER_RUNTIME_RUNTIME_CONTROLLER_H_

#include <future>
#include <memory>
#include <vector>

Expand Down Expand Up @@ -340,7 +341,7 @@ class RuntimeController : public PlatformConfigurationClient {
///
/// @return True if root isolate running, False otherwise.
///
virtual bool IsRootIsolateRunning() const;
virtual bool IsRootIsolateRunning();

//----------------------------------------------------------------------------
/// @brief Dispatch the specified platform message to running root
Expand Down Expand Up @@ -422,7 +423,10 @@ 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.
/// 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.
///
/// @return The root isolate reference.
///
Expand Down Expand Up @@ -471,11 +475,16 @@ class RuntimeController : public PlatformConfigurationClient {
std::string advisory_script_entrypoint_;
std::function<void(int64_t)> idle_notification_callback_;
PlatformData platform_data_;
std::future<void> 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<DartIsolate> root_isolate_;
std::pair<bool, uint32_t> root_isolate_return_code_ = {false, 0};
const fml::closure isolate_create_callback_;
const fml::closure isolate_shutdown_callback_;
std::shared_ptr<const fml::Mapping> persistent_isolate_data_;
fml::WeakPtrFactory<RuntimeController> weak_factory_;

PlatformConfiguration* GetPlatformConfigurationIfAvailable();

Expand Down
2 changes: 2 additions & 0 deletions runtime/runtime_delegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class RuntimeDelegate {

virtual FontCollection& GetFontCollection() = 0;

virtual void OnRootIsolateCreated() = 0;

virtual void UpdateIsolateDescription(const std::string isolate_name,
int64_t isolate_port) = 0;

Expand Down
4 changes: 4 additions & 0 deletions shell/common/engine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,10 @@ void Engine::HandlePlatformMessage(fml::RefPtr<PlatformMessage> message) {
}
}

void Engine::OnRootIsolateCreated() {
delegate_.OnRootIsolateCreated();
}

void Engine::UpdateIsolateDescription(const std::string isolate_name,
int64_t isolate_port) {
delegate_.UpdateIsolateDescription(isolate_name, isolate_port);
Expand Down
12 changes: 12 additions & 0 deletions shell/common/engine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,15 @@ 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
Expand Down Expand Up @@ -812,6 +821,9 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate {
// |RuntimeDelegate|
void HandlePlatformMessage(fml::RefPtr<PlatformMessage> message) override;

// |RuntimeDelegate|
void OnRootIsolateCreated() override;

// |RuntimeDelegate|
void UpdateIsolateDescription(const std::string isolate_name,
int64_t isolate_port) override;
Expand Down
4 changes: 3 additions & 1 deletion shell/common/engine_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class MockDelegate : public Engine::Delegate {
MOCK_METHOD1(OnEngineHandlePlatformMessage,
void(fml::RefPtr<PlatformMessage>));
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,
Expand All @@ -46,6 +47,7 @@ class MockRuntimeDelegate : public RuntimeDelegate {
void(SemanticsNodeUpdates, CustomAccessibilityActionUpdates));
MOCK_METHOD1(HandlePlatformMessage, void(fml::RefPtr<PlatformMessage>));
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,
Expand All @@ -57,7 +59,7 @@ class MockRuntimeController : public RuntimeController {
public:
MockRuntimeController(RuntimeDelegate& client, TaskRunners p_task_runners)
: RuntimeController(client, p_task_runners) {}
MOCK_CONST_METHOD0(IsRootIsolateRunning, bool());
MOCK_METHOD0(IsRootIsolateRunning, bool());
MOCK_METHOD1(DispatchPlatformMessage, bool(fml::RefPtr<PlatformMessage>));
};

Expand Down
25 changes: 21 additions & 4 deletions shell/common/shell.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,8 +563,6 @@ bool Shell::Setup(std::unique_ptr<PlatformView> platform_view,

is_setup_ = true;

vm_->GetServiceProtocol()->AddHandler(this, GetServiceProtocolDescription());

PersistentCache::GetCacheForProcess()->AddWorkerTaskRunner(
task_runners_.GetIOTaskRunner());

Expand Down Expand Up @@ -1134,6 +1132,19 @@ 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) {
Expand Down Expand Up @@ -1278,9 +1289,15 @@ bool Shell::HandleServiceProtocolMessage(
// |ServiceProtocol::Handler|
ServiceProtocol::Handler::Description Shell::GetServiceProtocolDescription()
const {
FML_DCHECK(task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread());

Copy link
Contributor

Choose a reason for hiding this comment

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

Per @jason-simmons 's suggestion

So it's unsafe to access Shell::engine_ directly from a UI thread task. UI thread tasks should use weak_engine_ and check if it is null.

let's just add an if (!weak_engine_) check here for extra safety although currently we shouldn't get a nullptr here. It probably looks very similar to your first version of if (!engine_) implementation. I guess Flutter engine team is just more used to if (!weak_ptr) than if (!raw_ptr) 😄

Other things look good to me and I'm ready to approve it.

@chinmaygarde : do you have any other requests to change? (You still flagged this PR as change requesteed.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

if (!weak_engine_) {
return ServiceProtocol::Handler::Description();
}

return {
engine_->GetUIIsolateMainPort(),
engine_->GetUIIsolateName(),
weak_engine_->GetUIIsolateMainPort(),
weak_engine_->GetUIIsolateName(),
};
}

Expand Down
3 changes: 3 additions & 0 deletions shell/common/shell.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,9 @@ 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;
Expand Down