From 6818b23a150fc641d90cfb2ed19397e46aff8c60 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Mon, 8 Jan 2024 10:31:02 +0100 Subject: [PATCH] Enforce consistent stack size for Flutter threads (flutter/engine#49111) Fixes https://github.com/flutter/flutter/issues/72156 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I signed the [CLA]. - [X] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- engine/src/flutter/fml/thread.cc | 73 ++++++++++++++++++- engine/src/flutter/fml/thread.h | 7 +- engine/src/flutter/fml/thread_unittests.cc | 60 ++++++++++++--- .../profiling/sampling_profiler_unittest.cc | 4 +- 4 files changed, 129 insertions(+), 15 deletions(-) diff --git a/engine/src/flutter/fml/thread.cc b/engine/src/flutter/fml/thread.cc index a905fd071e..cff08b4f8f 100644 --- a/engine/src/flutter/fml/thread.cc +++ b/engine/src/flutter/fml/thread.cc @@ -24,6 +24,71 @@ namespace fml { +typedef std::function ThreadFunction; + +class ThreadHandle { + public: + explicit ThreadHandle(ThreadFunction&& function); + ~ThreadHandle(); + + void Join(); + + private: +#if defined(FML_OS_WIN) + HANDLE thread_; +#else + pthread_t thread_; +#endif +}; + +#if defined(FML_OS_WIN) +ThreadHandle::ThreadHandle(ThreadFunction&& function) { + thread_ = (HANDLE*)_beginthreadex( + nullptr, Thread::GetDefaultStackSize(), + [](void* arg) -> unsigned { + std::unique_ptr function( + reinterpret_cast(arg)); + (*function)(); + return 0; + }, + new ThreadFunction(std::move(function)), 0, nullptr); + FML_CHECK(thread_ != nullptr); +} + +void ThreadHandle::Join() { + WaitForSingleObjectEx(thread_, INFINITE, FALSE); +} + +ThreadHandle::~ThreadHandle() { + CloseHandle(thread_); +} +#else +ThreadHandle::ThreadHandle(ThreadFunction&& function) { + pthread_attr_t attr; + pthread_attr_init(&attr); + int result = pthread_attr_setstacksize(&attr, Thread::GetDefaultStackSize()); + FML_CHECK(result == 0); + result = pthread_create( + &thread_, &attr, + [](void* arg) -> void* { + std::unique_ptr function( + reinterpret_cast(arg)); + (*function)(); + return nullptr; + }, + new ThreadFunction(std::move(function))); + FML_CHECK(result == 0); + result = pthread_attr_destroy(&attr); + FML_CHECK(result == 0); +} + +void ThreadHandle::Join() { + pthread_join(thread_, nullptr); +} + +ThreadHandle::~ThreadHandle() {} +#endif + #if defined(FML_OS_WIN) // The information on how to set the thread name comes from // a MSDN article: http://msdn2.microsoft.com/en-us/library/xcb2z8hs.aspx @@ -75,7 +140,7 @@ Thread::Thread(const ThreadConfigSetter& setter, const ThreadConfig& config) fml::AutoResetWaitableEvent latch; fml::RefPtr runner; - thread_ = std::make_unique( + thread_ = std::make_unique( [&latch, &runner, setter, config]() -> void { setter(config); fml::MessageLoop::EnsureInitializedForCurrentThread(); @@ -102,7 +167,11 @@ void Thread::Join() { } joined_ = true; task_runner_->PostTask([]() { MessageLoop::GetCurrent().Terminate(); }); - thread_->join(); + thread_->Join(); +} + +size_t Thread::GetDefaultStackSize() { + return 1024 * 1024 * 2; } } // namespace fml diff --git a/engine/src/flutter/fml/thread.h b/engine/src/flutter/fml/thread.h index 02a52e4a78..b7d09d182b 100644 --- a/engine/src/flutter/fml/thread.h +++ b/engine/src/flutter/fml/thread.h @@ -9,13 +9,14 @@ #include #include #include -#include #include "flutter/fml/macros.h" #include "flutter/fml/task_runner.h" namespace fml { +class ThreadHandle; + class Thread { public: /// Valid values for priority of Thread. @@ -59,8 +60,10 @@ class Thread { static void SetCurrentThreadName(const ThreadConfig& config); + static size_t GetDefaultStackSize(); + private: - std::unique_ptr thread_; + std::unique_ptr thread_; fml::RefPtr task_runner_; diff --git a/engine/src/flutter/fml/thread_unittests.cc b/engine/src/flutter/fml/thread_unittests.cc index 7b0f1f0d6e..0f668aed3e 100644 --- a/engine/src/flutter/fml/thread_unittests.cc +++ b/engine/src/flutter/fml/thread_unittests.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "flutter/fml/build_config.h" #include "flutter/fml/thread.h" #if defined(FML_OS_MACOSX) || defined(FML_OS_LINUX) || defined(FML_OS_ANDROID) @@ -15,6 +16,11 @@ #else #endif +#if defined(FML_OS_WIN) +#include +#endif + +#include #include #include "gtest/gtest.h" @@ -37,6 +43,35 @@ TEST(Thread, HasARunningMessageLoop) { ASSERT_TRUE(done); } +TEST(Thread, HasExpectedStackSize) { + size_t stack_size = 0; + fml::Thread thread; + + thread.GetTaskRunner()->PostTask([&stack_size]() { +#if defined(FML_OS_WIN) + ULONG_PTR low_limit; + ULONG_PTR high_limit; + GetCurrentThreadStackLimits(&low_limit, &high_limit); + stack_size = high_limit - low_limit; +#elif defined(FML_OS_MACOSX) + stack_size = pthread_get_stacksize_np(pthread_self()); +#else + pthread_attr_t attr; + pthread_getattr_np(pthread_self(), &attr); + pthread_attr_getstacksize(&attr, &stack_size); + pthread_attr_destroy(&attr); +#endif + }); + thread.Join(); + + // Actual stack size will be aligned to page size, this assumes no supported + // platform has a page size larger than 16k. On Linux reducing the default + // stack size (8MB) does not seem to have any effect. + const size_t kPageSize = 16384; + ASSERT_TRUE(stack_size / kPageSize >= + fml::Thread::GetDefaultStackSize() / kPageSize); +} + #if FLUTTER_PTHREAD_SUPPORTED TEST(Thread, ThreadNameCreatedWithConfig) { const std::string name = "Thread1"; @@ -45,15 +80,20 @@ TEST(Thread, ThreadNameCreatedWithConfig) { bool done = false; thread.GetTaskRunner()->PostTask([&done, &name]() { done = true; - char thread_name[8]; + char thread_name[16]; pthread_t current_thread = pthread_self(); - pthread_getname_np(current_thread, thread_name, 8); + pthread_getname_np(current_thread, thread_name, 16); ASSERT_EQ(thread_name, name); }); thread.Join(); ASSERT_TRUE(done); } +static int clamp_priority(int priority, int policy) { + return std::clamp(priority, sched_get_priority_min(policy), + sched_get_priority_max(policy)); +} + static void MockThreadConfigSetter(const fml::Thread::ThreadConfig& config) { // set thread name fml::Thread::SetCurrentThreadName(config); @@ -63,10 +103,10 @@ static void MockThreadConfigSetter(const fml::Thread::ThreadConfig& config) { int policy = SCHED_OTHER; switch (config.priority) { case fml::Thread::ThreadPriority::kDisplay: - param.sched_priority = 10; + param.sched_priority = clamp_priority(10, policy); break; default: - param.sched_priority = 1; + param.sched_priority = clamp_priority(1, policy); } pthread_setschedparam(tid, policy, ¶m); } @@ -84,13 +124,13 @@ TEST(Thread, ThreadPriorityCreatedWithConfig) { int policy; thread.GetTaskRunner()->PostTask([&]() { done = true; - char thread_name[8]; + char thread_name[16]; pthread_t current_thread = pthread_self(); - pthread_getname_np(current_thread, thread_name, 8); + pthread_getname_np(current_thread, thread_name, 16); pthread_getschedparam(current_thread, &policy, ¶m); ASSERT_EQ(thread_name, thread1_name); ASSERT_EQ(policy, SCHED_OTHER); - ASSERT_EQ(param.sched_priority, 1); + ASSERT_EQ(param.sched_priority, clamp_priority(1, policy)); }); fml::Thread thread2(MockThreadConfigSetter, @@ -98,13 +138,13 @@ TEST(Thread, ThreadPriorityCreatedWithConfig) { thread2_name, fml::Thread::ThreadPriority::kDisplay)); thread2.GetTaskRunner()->PostTask([&]() { done = true; - char thread_name[8]; + char thread_name[16]; pthread_t current_thread = pthread_self(); - pthread_getname_np(current_thread, thread_name, 8); + pthread_getname_np(current_thread, thread_name, 16); pthread_getschedparam(current_thread, &policy, ¶m); ASSERT_EQ(thread_name, thread2_name); ASSERT_EQ(policy, SCHED_OTHER); - ASSERT_EQ(param.sched_priority, 10); + ASSERT_EQ(param.sched_priority, clamp_priority(10, policy)); }); thread.Join(); ASSERT_TRUE(done); diff --git a/engine/src/flutter/shell/profiling/sampling_profiler_unittest.cc b/engine/src/flutter/shell/profiling/sampling_profiler_unittest.cc index 95ed49c38c..6195bb6bd4 100644 --- a/engine/src/flutter/shell/profiling/sampling_profiler_unittest.cc +++ b/engine/src/flutter/shell/profiling/sampling_profiler_unittest.cc @@ -2,9 +2,11 @@ // 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 + #include "flutter/fml/message_loop_impl.h" #include "flutter/fml/thread.h" +#include "flutter/shell/profiling/sampling_profiler.h" #include "flutter/testing/testing.h" #include "gmock/gmock.h"