Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not serialize on UI/Raster threads for Shell::OnCreatePlatformView #29145

Merged
merged 5 commits into from
Oct 18, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions shell/common/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ if (enable_unittests) {
":shell_unittests_fixtures",
"//flutter/benchmarking",
"//flutter/flow",
"//flutter/shell/gpu:gpu_surface_software",
"//flutter/testing:dart",
"//flutter/testing:fixture_test",
"//flutter/testing:testing_lib",
Expand Down
42 changes: 16 additions & 26 deletions shell/common/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -733,16 +733,11 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> surface) {
const bool should_post_raster_task =
!task_runners_.GetRasterTaskRunner()->RunsTasksOnCurrentThread();

// Note:
// This is a synchronous operation because certain platforms depend on
// setup/suspension of all activities that may be interacting with the GPU in
// a synchronous fashion.
fml::AutoResetWaitableEvent latch;
auto raster_task =
fml::MakeCopyable([&waiting_for_first_frame = waiting_for_first_frame_,
rasterizer = rasterizer_->GetWeakPtr(), //
surface = std::move(surface), //
&latch]() mutable {
surface = std::move(surface)]() mutable {
if (rasterizer) {
// Enables the thread merger which may be used by the external view
// embedder.
Expand All @@ -751,29 +746,15 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> surface) {
}

waiting_for_first_frame.store(true);

// Step 3: All done. Signal the latch that the platform thread is
// waiting on.
latch.Signal();
});

auto ui_task = [engine = engine_->GetWeakPtr(), //
raster_task_runner = task_runners_.GetRasterTaskRunner(), //
raster_task, should_post_raster_task,
&latch //
] {
// TODO(dnfield): This probably isn't necessary. The engine should be able to
Copy link
Member

Choose a reason for hiding this comment

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

Nit: TODO(91717) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// handle things here via normal lifecycle messages.
// https://github.com/flutter/flutter/issues/91717
auto ui_task = [engine = engine_->GetWeakPtr()] {
if (engine) {
engine->OnOutputSurfaceCreated();
}
// Step 2: Next, tell the raster thread that it should create a surface for
// its rasterizer.
if (should_post_raster_task) {
fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task);
} else {
// See comment on should_post_raster_task, in this case we just unblock
// the platform thread.
latch.Signal();
}
};

// Threading: Capture platform view by raw pointer and not the weak pointer.
Expand All @@ -786,7 +767,9 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> surface) {

auto io_task = [io_manager = io_manager_->GetWeakPtr(), platform_view,
ui_task_runner = task_runners_.GetUITaskRunner(), ui_task,
shared_resource_context = shared_resource_context_] {
shared_resource_context = shared_resource_context_,
raster_task_runner = task_runners_.GetRasterTaskRunner(),
raster_task, should_post_raster_task, &latch] {
if (io_manager && !io_manager->GetResourceContext()) {
sk_sp<GrDirectContext> resource_context;
if (shared_resource_context) {
Expand All @@ -796,9 +779,16 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> surface) {
}
io_manager->NotifyResourceContextAvailable(resource_context);
}
// Step 1: Next, post a task on the UI thread to tell the engine that it has
// Step 1: Post a task on the UI thread to tell the engine that it has
// an output surface.
fml::TaskRunner::RunNowOrPostTask(ui_task_runner, ui_task);

// Step 2: Tell the raster thread that it should create a surface for
// its rasterizer.
if (should_post_raster_task) {
fml::TaskRunner::RunNowOrPostTask(raster_task_runner, raster_task);
}
latch.Signal();
};

fml::TaskRunner::RunNowOrPostTask(task_runners_.GetIOTaskRunner(), io_task);
Expand Down
158 changes: 158 additions & 0 deletions shell/common/shell_benchmarks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,98 @@

#include "flutter/shell/common/shell.h"

#include <chrono>
#include <thread>

#include "flutter/benchmarking/benchmarking.h"
#include "flutter/fml/logging.h"
#include "flutter/runtime/dart_vm.h"
#include "flutter/shell/common/thread_host.h"
#include "flutter/shell/gpu/gpu_surface_software.h"
#include "flutter/testing/elf_loader.h"
#include "flutter/testing/testing.h"

namespace flutter {
class BenchmarkExternalViewEmbedder : public ExternalViewEmbedder {
// |ExternalViewEmbedder|
SkCanvas* GetRootCanvas() override { return nullptr; }

// |ExternalViewEmbedder|
void CancelFrame() override {}

// |ExternalViewEmbedder|
void BeginFrame(
SkISize frame_size,
GrDirectContext* context,
double device_pixel_ratio,
fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) override {}

// |ExternalViewEmbedder|
void PrerollCompositeEmbeddedView(
int view_id,
std::unique_ptr<EmbeddedViewParams> params) override {}

// |ExternalViewEmbedder|
std::vector<SkCanvas*> GetCurrentCanvases() override { return {&canvas_}; }

// |ExternalViewEmbedder|
SkCanvas* CompositeEmbeddedView(int view_id) override { return &canvas_; }

private:
SkCanvas canvas_;
Copy link
Member

Choose a reason for hiding this comment

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

Here and elsewhere: Disallow copy and assign.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

};

class BenchmarkPlatformView : public PlatformView,
public GPUSurfaceSoftwareDelegate {
public:
BenchmarkPlatformView(Delegate& delegate, TaskRunners task_runners)
: PlatformView(delegate, std::move(task_runners)) {}

// |PlatformView|
std::unique_ptr<Surface> CreateRenderingSurface() override {
auto surface = std::make_unique<GPUSurfaceSoftware>(
this, true /* render to surface */);
FML_DCHECK(surface->IsValid());
return surface;
}

// |GPUSurfaceSoftwareDelegate|
sk_sp<SkSurface> AcquireBackingStore(const SkISize& size) override {
if (sk_surface_ != nullptr &&
SkISize::Make(sk_surface_->width(), sk_surface_->height()) == size) {
// The old and new surface sizes are the same. Nothing to do here.
return sk_surface_;
}

SkImageInfo info =
SkImageInfo::MakeN32(size.fWidth, size.fHeight, kPremul_SkAlphaType,
SkColorSpace::MakeSRGB());
sk_surface_ = SkSurface::MakeRaster(info, nullptr);

if (sk_surface_ == nullptr) {
FML_LOG(ERROR)
<< "Could not create backing store for software rendering.";
return nullptr;
}

return sk_surface_;
}

// |GPUSurfaceSoftwareDelegate|
bool PresentBackingStore(sk_sp<SkSurface> backing_store) override {
return true;
}

// |PlatformView|
std::shared_ptr<ExternalViewEmbedder> CreateExternalViewEmbedder() override {
return external_view_embedder_;
}

private:
sk_sp<SkSurface> sk_surface_ = nullptr;
std::shared_ptr<BenchmarkExternalViewEmbedder> external_view_embedder_ =
std::make_shared<BenchmarkExternalViewEmbedder>();
};

static void StartupAndShutdownShell(benchmark::State& state,
bool measure_startup,
Expand Down Expand Up @@ -120,4 +204,78 @@ static void BM_ShellInitializationAndShutdown(benchmark::State& state) {

BENCHMARK(BM_ShellInitializationAndShutdown);

static void BM_ShellOnPlatformViewCreated(benchmark::State& state) {
std::unique_ptr<Shell> shell;
std::unique_ptr<ThreadHost> thread_host;

auto assets_dir = fml::OpenDirectory(testing::GetFixturesPath(), false,
fml::FilePermission::kRead);
testing::ELFAOTSymbols aot_symbols;

Settings settings = {};
settings.task_observer_add = [](intptr_t, fml::closure) {};
settings.task_observer_remove = [](intptr_t) {};

if (DartVM::IsRunningPrecompiledCode()) {
aot_symbols = testing::LoadELFSymbolFromFixturesIfNeccessary(
testing::kDefaultAOTAppELFFileName);
FML_CHECK(testing::PrepareSettingsForAOTWithSymbols(settings, aot_symbols))
<< "Could not set up settings with AOT symbols.";
} else {
settings.application_kernels = [&]() {
std::vector<std::unique_ptr<const fml::Mapping>> kernel_mappings;
kernel_mappings.emplace_back(
fml::FileMapping::CreateReadOnly(assets_dir, "kernel_blob.bin"));
return kernel_mappings;
};
}

thread_host = std::make_unique<ThreadHost>(
"io.flutter.bench.", ThreadHost::Type::Platform |
ThreadHost::Type::RASTER | ThreadHost::Type::IO |
ThreadHost::Type::UI);

TaskRunners task_runners("test",
thread_host->platform_thread->GetTaskRunner(),
thread_host->raster_thread->GetTaskRunner(),
thread_host->ui_thread->GetTaskRunner(),
thread_host->io_thread->GetTaskRunner());

shell = Shell::Create(
flutter::PlatformData(), std::move(task_runners), settings,
[](Shell& shell) {
return std::make_unique<BenchmarkPlatformView>(shell,
shell.GetTaskRunners());
},
[](Shell& shell) { return std::make_unique<Rasterizer>(shell); });

FML_CHECK(shell);

while (state.KeepRunning()) {
// Artificially busy the UI thread, as if loading a larger program
// snapshot.
fml::TaskRunner::RunNowOrPostTask(
thread_host->ui_thread->GetTaskRunner(),
[]() { std::this_thread::sleep_for(std::chrono::milliseconds(5)); });
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid magic timeouts.

Copy link
Member

Choose a reason for hiding this comment

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

To elaborate on how to avoid magic timeouts, you could either setup a latch that gets signaled on the NotifyDestroyed. That can be after the timing for the run has been paused if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking a latch wouldn't be great here because the benchmark wouldn't even work without my change, but I guess it's fine and I have to re-order the threading anyway because of a misuse of GetPlatformView.

shell->GetPlatformView()->NotifyCreated();
{
benchmarking::ScopedPauseTiming pause(state);
shell->GetPlatformView()->NotifyDestroyed();
}
}

// Shutdown must occur synchronously on the platform thread.
fml::AutoResetWaitableEvent latch;
fml::TaskRunner::RunNowOrPostTask(
thread_host->platform_thread->GetTaskRunner(),
[&shell, &latch]() mutable {
shell.reset();
latch.Signal();
});
latch.Wait();
thread_host.reset();
}

BENCHMARK(BM_ShellOnPlatformViewCreated);

} // namespace flutter