From 9643c9d1108a4ad2915614330da0f8ad78258b69 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Mon, 14 Dec 2020 17:59:13 -0800 Subject: [PATCH] Started shutting down the sampler when it gets deleted (#23012) --- ci/licenses_golden/licenses_flutter | 1 + shell/common/BUILD.gn | 1 + shell/profiling/BUILD.gn | 9 +++ shell/profiling/sampling_profiler.cc | 27 +++++++- shell/profiling/sampling_profiler.h | 9 ++- shell/profiling/sampling_profiler_unittest.cc | 64 +++++++++++++++++++ 6 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 shell/profiling/sampling_profiler_unittest.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index e65962eeb..e4624d767 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -1461,6 +1461,7 @@ FILE: ../../../flutter/shell/platform/windows/window_binding_handler_delegate.h FILE: ../../../flutter/shell/platform/windows/window_state.h FILE: ../../../flutter/shell/profiling/sampling_profiler.cc FILE: ../../../flutter/shell/profiling/sampling_profiler.h +FILE: ../../../flutter/shell/profiling/sampling_profiler_unittest.cc FILE: ../../../flutter/shell/version/version.cc FILE: ../../../flutter/shell/version/version.h FILE: ../../../flutter/sky/packages/flutter_services/lib/empty.dart diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index b08fa8f6a..c5b89c720 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -254,6 +254,7 @@ if (enable_unittests) { ":shell_unittests_fixtures", "//flutter/assets", "//flutter/common/graphics", + "//flutter/shell/profiling:profiling_unittests", "//flutter/shell/version", "//third_party/googletest:gmock", ] diff --git a/shell/profiling/BUILD.gn b/shell/profiling/BUILD.gn index 0ddef5cde..457ef6d29 100644 --- a/shell/profiling/BUILD.gn +++ b/shell/profiling/BUILD.gn @@ -17,3 +17,12 @@ source_set("profiling") { deps = _profiler_deps } + +source_set("profiling_unittests") { + testonly = true + sources = [ "sampling_profiler_unittest.cc" ] + deps = [ + ":profiling", + "//flutter/testing", + ] +} diff --git a/shell/profiling/sampling_profiler.cc b/shell/profiling/sampling_profiler.cc index 790d70f91..e1f1a0c29 100644 --- a/shell/profiling/sampling_profiler.cc +++ b/shell/profiling/sampling_profiler.cc @@ -16,7 +16,13 @@ SamplingProfiler::SamplingProfiler( sampler_(std::move(sampler)), num_samples_per_sec_(num_samples_per_sec) {} -void SamplingProfiler::Start() const { +SamplingProfiler::~SamplingProfiler() { + if (is_running_) { + Stop(); + } +} + +void SamplingProfiler::Start() { if (!profiler_task_runner_) { return; } @@ -26,12 +32,23 @@ void SamplingProfiler::Start() const { double delay_between_samples = 1.0 / num_samples_per_sec_; auto task_delay = fml::TimeDelta::FromSecondsF(delay_between_samples); UpdateObservatoryThreadName(); + is_running_ = true; SampleRepeatedly(task_delay); } +void SamplingProfiler::Stop() { + FML_DCHECK(is_running_); + auto latch = std::make_unique(); + shutdown_latch_.store(latch.get()); + latch->Wait(); + shutdown_latch_.store(nullptr); + is_running_ = false; +} + void SamplingProfiler::SampleRepeatedly(fml::TimeDelta task_delay) const { profiler_task_runner_->PostDelayedTask( - [profiler = this, task_delay = task_delay, sampler = sampler_]() { + [profiler = this, task_delay = task_delay, sampler = sampler_, + &shutdown_latch = shutdown_latch_]() { // TODO(kaushikiska): consider buffering these every n seconds to // avoid spamming the trace buffer. const ProfileSample usage = sampler(); @@ -60,7 +77,11 @@ void SamplingProfiler::SampleRepeatedly(fml::TimeDelta task_delay) const { TRACE_EVENT_INSTANT1("flutter::profiling", "GpuUsage", "gpu_usage", gpu_usage.c_str()); } - profiler->SampleRepeatedly(task_delay); + if (shutdown_latch.load()) { + shutdown_latch.load()->Signal(); + } else { + profiler->SampleRepeatedly(task_delay); + } }, task_delay); } diff --git a/shell/profiling/sampling_profiler.h b/shell/profiling/sampling_profiler.h index 146454d3f..8b4b06c00 100644 --- a/shell/profiling/sampling_profiler.h +++ b/shell/profiling/sampling_profiler.h @@ -10,6 +10,7 @@ #include #include +#include "flutter/fml/synchronization/count_down_latch.h" #include "flutter/fml/task_runner.h" #include "flutter/fml/trace_event.h" @@ -97,17 +98,23 @@ class SamplingProfiler { Sampler sampler, int num_samples_per_sec); + ~SamplingProfiler(); + /** * @brief Starts the SamplingProfiler by triggering `SampleRepeatedly`. * */ - void Start() const; + void Start(); + + void Stop(); private: const std::string thread_label_; const fml::RefPtr profiler_task_runner_; const Sampler sampler_; const uint32_t num_samples_per_sec_; + bool is_running_ = false; + std::atomic shutdown_latch_ = nullptr; void SampleRepeatedly(fml::TimeDelta task_delay) const; diff --git a/shell/profiling/sampling_profiler_unittest.cc b/shell/profiling/sampling_profiler_unittest.cc new file mode 100644 index 000000000..784b73206 --- /dev/null +++ b/shell/profiling/sampling_profiler_unittest.cc @@ -0,0 +1,64 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/shell/profiling/sampling_profiler.h" +#include "flutter/fml/message_loop_impl.h" +#include "flutter/fml/thread.h" +#include "flutter/testing/testing.h" +#include "gmock/gmock.h" + +using testing::_; +using testing::Invoke; + +namespace fml { +namespace { +class MockTaskRunner : public fml::TaskRunner { + public: + inline static RefPtr Create() { + return AdoptRef(new MockTaskRunner()); + } + MOCK_METHOD1(PostTask, void(const fml::closure& task)); + MOCK_METHOD2(PostTaskForTime, + void(const fml::closure& task, fml::TimePoint target_time)); + MOCK_METHOD2(PostDelayedTask, + void(const fml::closure& task, fml::TimeDelta delay)); + MOCK_METHOD0(RunsTasksOnCurrentThread, bool()); + MOCK_METHOD0(GetTaskQueueId, TaskQueueId()); + + private: + MockTaskRunner() : TaskRunner(fml::RefPtr()) {} +}; +} // namespace +} // namespace fml + +namespace flutter { + +TEST(SamplingProfilerTest, DeleteAfterStart) { + auto thread = + std::make_unique(flutter::testing::GetCurrentTestName()); + auto task_runner = fml::MockTaskRunner::Create(); + std::atomic invoke_count = 0; + + // Ignore calls to PostTask since that would require mocking out calls to + // Dart. + EXPECT_CALL(*task_runner, PostDelayedTask(_, _)) + .WillRepeatedly( + Invoke([&](const fml::closure& task, fml::TimeDelta delay) { + invoke_count.fetch_add(1); + thread->GetTaskRunner()->PostTask(task); + })); + + { + auto profiler = SamplingProfiler( + "profiler", + /*profiler_task_runner=*/task_runner, [] { return ProfileSample(); }, + /*num_samples_per_sec=*/1000); + profiler.Start(); + } + int invoke_count_at_delete = invoke_count.load(); + std::this_thread::sleep_for(std::chrono::milliseconds(2)); // nyquist + ASSERT_EQ(invoke_count_at_delete, invoke_count.load()); +} + +} // namespace flutter -- GitLab