From bc694af2cf5cf83b4c2befaded1b28fca400f34c Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 6 Dec 2019 16:51:34 -0800 Subject: [PATCH 1/4] Cleanup the IO thread GrContext Fixes https://github.com/flutter/flutter/issues/19558 This is tested by the devicelab test fast_scroll_large_images__memory --- flow/skia_gpu_object.cc | 12 ++++++++++-- flow/skia_gpu_object.h | 8 +++++++- shell/common/shell_io_manager.cc | 3 ++- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/flow/skia_gpu_object.cc b/flow/skia_gpu_object.cc index dd6c6b8436dff..48b919b213260 100644 --- a/flow/skia_gpu_object.cc +++ b/flow/skia_gpu_object.cc @@ -5,14 +5,17 @@ #include "flutter/flow/skia_gpu_object.h" #include "flutter/fml/message_loop.h" +#include "flutter/fml/trace_event.h" namespace flutter { SkiaUnrefQueue::SkiaUnrefQueue(fml::RefPtr task_runner, - fml::TimeDelta delay) + fml::TimeDelta delay, + GrContext* context) : task_runner_(std::move(task_runner)), drain_delay_(delay), - drain_pending_(false) {} + drain_pending_(false), + context_(context) {} SkiaUnrefQueue::~SkiaUnrefQueue() { FML_DCHECK(objects_.empty()); @@ -29,6 +32,7 @@ void SkiaUnrefQueue::Unref(SkRefCnt* object) { } void SkiaUnrefQueue::Drain() { + TRACE_EVENT0("flutter", "SkiaUnrefQueue::Drain"); std::deque skia_objects; { std::scoped_lock lock(mutex_); @@ -39,6 +43,10 @@ void SkiaUnrefQueue::Drain() { for (SkRefCnt* skia_object : skia_objects) { skia_object->unref(); } + + if (context_ != nullptr) { + context_->performDeferredCleanup(std::chrono::milliseconds(0)); + } } } // namespace flutter diff --git a/flow/skia_gpu_object.h b/flow/skia_gpu_object.h index 2a09f982a4386..bb6958c03f59d 100644 --- a/flow/skia_gpu_object.h +++ b/flow/skia_gpu_object.h @@ -12,6 +12,7 @@ #include "flutter/fml/memory/weak_ptr.h" #include "flutter/fml/task_runner.h" #include "third_party/skia/include/core/SkRefCnt.h" +#include "third_party/skia/include/gpu/GrContext.h" namespace flutter { @@ -34,9 +35,14 @@ class SkiaUnrefQueue : public fml::RefCountedThreadSafe { std::mutex mutex_; std::deque objects_; bool drain_pending_; + GrContext* context_; + // The `GrContext* context` is only used for signaling Skia to + // performDeferredCleanup. It can be nullptr when such signaling is not needed + // (e.g., in unit tests). SkiaUnrefQueue(fml::RefPtr task_runner, - fml::TimeDelta delay); + fml::TimeDelta delay, + GrContext* context = nullptr); ~SkiaUnrefQueue(); diff --git a/shell/common/shell_io_manager.cc b/shell/common/shell_io_manager.cc index 6c23167bf85d2..660fe4961170b 100644 --- a/shell/common/shell_io_manager.cc +++ b/shell/common/shell_io_manager.cc @@ -62,7 +62,8 @@ ShellIOManager::ShellIOManager( : nullptr), unref_queue_(fml::MakeRefCounted( std::move(unref_queue_task_runner), - fml::TimeDelta::FromMilliseconds(8))), + fml::TimeDelta::FromMilliseconds(8), + resource_context_.get())), weak_factory_(this), is_gpu_disabled_sync_switch_(is_gpu_disabled_sync_switch) { if (!resource_context_) { From 5927426927889c9cfcc9cce13046085592db4430 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Mon, 9 Dec 2019 15:36:44 -0800 Subject: [PATCH 2/4] Use a WeakPtr instead of a raw pointer Otherwise, the IOManager may be destructed before the Drain is called and that could cause a crash. --- flow/skia_gpu_object.cc | 4 ++-- flow/skia_gpu_object.h | 4 ++-- shell/common/shell_io_manager.cc | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/flow/skia_gpu_object.cc b/flow/skia_gpu_object.cc index 48b919b213260..8f43626281aac 100644 --- a/flow/skia_gpu_object.cc +++ b/flow/skia_gpu_object.cc @@ -11,7 +11,7 @@ namespace flutter { SkiaUnrefQueue::SkiaUnrefQueue(fml::RefPtr task_runner, fml::TimeDelta delay, - GrContext* context) + fml::WeakPtr context) : task_runner_(std::move(task_runner)), drain_delay_(delay), drain_pending_(false), @@ -44,7 +44,7 @@ void SkiaUnrefQueue::Drain() { skia_object->unref(); } - if (context_ != nullptr) { + if (context_) { context_->performDeferredCleanup(std::chrono::milliseconds(0)); } } diff --git a/flow/skia_gpu_object.h b/flow/skia_gpu_object.h index bb6958c03f59d..37850ce0b6cc2 100644 --- a/flow/skia_gpu_object.h +++ b/flow/skia_gpu_object.h @@ -35,14 +35,14 @@ class SkiaUnrefQueue : public fml::RefCountedThreadSafe { std::mutex mutex_; std::deque objects_; bool drain_pending_; - GrContext* context_; + fml::WeakPtr context_; // The `GrContext* context` is only used for signaling Skia to // performDeferredCleanup. It can be nullptr when such signaling is not needed // (e.g., in unit tests). SkiaUnrefQueue(fml::RefPtr task_runner, fml::TimeDelta delay, - GrContext* context = nullptr); + fml::WeakPtr context = {}); ~SkiaUnrefQueue(); diff --git a/shell/common/shell_io_manager.cc b/shell/common/shell_io_manager.cc index 660fe4961170b..97badd40bb25b 100644 --- a/shell/common/shell_io_manager.cc +++ b/shell/common/shell_io_manager.cc @@ -63,7 +63,7 @@ ShellIOManager::ShellIOManager( unref_queue_(fml::MakeRefCounted( std::move(unref_queue_task_runner), fml::TimeDelta::FromMilliseconds(8), - resource_context_.get())), + GetResourceContext())), weak_factory_(this), is_gpu_disabled_sync_switch_(is_gpu_disabled_sync_switch) { if (!resource_context_) { From c7e6e9013ec321c3939d5ad35de9d8160867e073 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Mon, 9 Dec 2019 15:39:11 -0800 Subject: [PATCH 3/4] Nits --- flow/skia_gpu_object.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flow/skia_gpu_object.cc b/flow/skia_gpu_object.cc index 8f43626281aac..aadb4e3b72f71 100644 --- a/flow/skia_gpu_object.cc +++ b/flow/skia_gpu_object.cc @@ -44,7 +44,7 @@ void SkiaUnrefQueue::Drain() { skia_object->unref(); } - if (context_) { + if (context_ && skia_objects.size() > 0) { context_->performDeferredCleanup(std::chrono::milliseconds(0)); } } From d536937dc0c816bb696a32d48c20c8f79b39fda0 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Mon, 9 Dec 2019 16:51:33 -0800 Subject: [PATCH 4/4] Fix the thread issue of the unit test --- flow/skia_gpu_object_unittests.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/flow/skia_gpu_object_unittests.cc b/flow/skia_gpu_object_unittests.cc index 9df82ad11c312..35737708ac5ca 100644 --- a/flow/skia_gpu_object_unittests.cc +++ b/flow/skia_gpu_object_unittests.cc @@ -11,6 +11,8 @@ #include "gtest/gtest.h" #include "third_party/skia/include/core/SkRefCnt.h" +#include + namespace flutter { namespace testing { @@ -42,6 +44,18 @@ class SkiaGpuObjectTest : public ThreadTest { delayed_unref_queue_(fml::MakeRefCounted( unref_task_runner(), fml::TimeDelta::FromSeconds(3))) { + // The unref queues must be created in the same thread of the + // unref_task_runner so the queue can access the same-thread-only WeakPtr of + // the GrContext constructed during the creation. + std::promise queuesCreated; + unref_task_runner_->PostTask([this, &queuesCreated]() { + unref_queue_ = fml::MakeRefCounted( + unref_task_runner(), fml::TimeDelta::FromSeconds(0)); + delayed_unref_queue_ = fml::MakeRefCounted( + unref_task_runner(), fml::TimeDelta::FromSeconds(3)); + queuesCreated.set_value(true); + }); + queuesCreated.get_future().wait(); ::testing::FLAGS_gtest_death_test_style = "threadsafe"; }