Skip to content

Commit 31c4875

Browse files
flutteractionsbotflutteractionsbot
authored
[CP-stable][Impeller] Maintain a global map of each context's currently active thread-local command pools (#170013)
This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. ### Issue Link: What is the link to the issue this cherry-pick is addressing? #169208 ### Changelog Description: Explain this cherry pick in one line that is accessible to most Flutter developers. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples Fixes a memory leak in the Impeller Vulkan back end. ### Impact Description: What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch) The memory usage of apps using Impeller/Vulkan will increase as frames are rendered. Memory consumption will grow until the Android activity enters the stopped state. ### Workaround: Is there a workaround for this issue? Disabling Impeller ### Risk: What is the risk level of this cherry-pick? - [ x ] Medium ### Test Coverage: Are you confident that your fix is well-tested by automated tests? - [ x ] Yes ### Validation Steps: What are the steps to validate that this fix works? Start an app using Impeller/Vulkan that renders frames nonstop (for example, video playback). Leave it running for several minutes. Check memory metrics with a tool like `adb shell dumpsys meminfo` and verify that memory usage is stable.
1 parent 46a4c05 commit 31c4875

File tree

5 files changed

+70
-27
lines changed

5 files changed

+70
-27
lines changed

‎engine/src/flutter/impeller/golden_tests/golden_playground_test_mac.cc‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,11 @@ void GoldenPlaygroundTest::SetTypographerContext(
132132

133133
void GoldenPlaygroundTest::TearDown() {
134134
ASSERT_FALSE(dlopen("/usr/local/lib/libMoltenVK.dylib", RTLD_NOLOAD));
135+
136+
auto context = GetContext();
137+
if (context) {
138+
context->DisposeThreadLocalCachedResources();
139+
}
135140
}
136141

137142
namespace {
@@ -274,6 +279,9 @@ RuntimeStage::Map GoldenPlaygroundTest::OpenAssetAsRuntimeStage(
274279
}
275280

276281
std::shared_ptr<Context> GoldenPlaygroundTest::GetContext() const {
282+
if (!pimpl_->screenshotter) {
283+
return nullptr;
284+
}
277285
return pimpl_->screenshotter->GetPlayground().GetContext();
278286
}
279287

‎engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.cc‎

Lines changed: 31 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -171,10 +171,23 @@ static thread_local std::unique_ptr<CommandPoolMap> tls_command_pool_map;
171171
// with that context.
172172
static Mutex g_all_pools_map_mutex;
173173
static std::unordered_map<
174-
const ContextVK*,
175-
std::vector<std::weak_ptr<CommandPoolVK>>> g_all_pools_map
174+
uint64_t,
175+
std::unordered_map<std::thread::id,
176+
std::weak_ptr<CommandPoolVK>>> g_all_pools_map
176177
IPLR_GUARDED_BY(g_all_pools_map_mutex);
177178

179+
CommandPoolRecyclerVK::CommandPoolRecyclerVK(
180+
const std::shared_ptr<ContextVK>& context)
181+
: context_(context), context_hash_(context->GetHash()) {}
182+
183+
// Visible for testing.
184+
// Returns the number of pools in g_all_pools_map for the given context.
185+
int CommandPoolRecyclerVK::GetGlobalPoolCount(const ContextVK& context) {
186+
Lock all_pools_lock(g_all_pools_map_mutex);
187+
auto it = g_all_pools_map.find(context.GetHash());
188+
return it != g_all_pools_map.end() ? it->second.size() : 0;
189+
}
190+
178191
// TODO(matanlurey): Return a status_or<> instead of nullptr when we have one.
179192
std::shared_ptr<CommandPoolVK> CommandPoolRecyclerVK::Get() {
180193
auto const strong_context = context_.lock();
@@ -187,8 +200,7 @@ std::shared_ptr<CommandPoolVK> CommandPoolRecyclerVK::Get() {
187200
tls_command_pool_map.reset(new CommandPoolMap());
188201
}
189202
CommandPoolMap& pool_map = *tls_command_pool_map.get();
190-
auto const hash = strong_context->GetHash();
191-
auto const it = pool_map.find(hash);
203+
auto const it = pool_map.find(context_hash_);
192204
if (it != pool_map.end()) {
193205
return it->second;
194206
}
@@ -201,11 +213,11 @@ std::shared_ptr<CommandPoolVK> CommandPoolRecyclerVK::Get() {
201213

202214
auto const resource = std::make_shared<CommandPoolVK>(
203215
std::move(data->pool), std::move(data->buffers), context_);
204-
pool_map.emplace(hash, resource);
216+
pool_map.emplace(context_hash_, resource);
205217

206218
{
207219
Lock all_pools_lock(g_all_pools_map_mutex);
208-
g_all_pools_map[strong_context.get()].push_back(resource);
220+
g_all_pools_map[context_hash_][std::this_thread::get_id()] = resource;
209221
}
210222

211223
return resource;
@@ -275,30 +287,33 @@ void CommandPoolRecyclerVK::Reclaim(
275287
RecycledData{.pool = std::move(pool), .buffers = std::move(buffers)});
276288
}
277289

278-
CommandPoolRecyclerVK::~CommandPoolRecyclerVK() {
279-
// Ensure all recycled pools are reclaimed before this is destroyed.
280-
Dispose();
281-
}
282-
283290
void CommandPoolRecyclerVK::Dispose() {
284291
CommandPoolMap* pool_map = tls_command_pool_map.get();
285292
if (pool_map) {
286-
pool_map->clear();
293+
pool_map->erase(context_hash_);
294+
}
295+
296+
{
297+
Lock all_pools_lock(g_all_pools_map_mutex);
298+
auto found = g_all_pools_map.find(context_hash_);
299+
if (found != g_all_pools_map.end()) {
300+
found->second.erase(std::this_thread::get_id());
301+
}
287302
}
288303
}
289304

290-
void CommandPoolRecyclerVK::DestroyThreadLocalPools(const ContextVK* context) {
305+
void CommandPoolRecyclerVK::DestroyThreadLocalPools() {
291306
// Delete the context's entry in this thread's command pool map.
292307
if (tls_command_pool_map.get()) {
293-
tls_command_pool_map.get()->erase(context->GetHash());
308+
tls_command_pool_map.get()->erase(context_hash_);
294309
}
295310

296311
// Destroy all other thread-local CommandPoolVK instances associated with
297312
// this context.
298313
Lock all_pools_lock(g_all_pools_map_mutex);
299-
auto found = g_all_pools_map.find(context);
314+
auto found = g_all_pools_map.find(context_hash_);
300315
if (found != g_all_pools_map.end()) {
301-
for (auto& weak_pool : found->second) {
316+
for (auto& [thread_id, weak_pool] : found->second) {
302317
auto pool = weak_pool.lock();
303318
if (!pool) {
304319
continue;

‎engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.h‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -103,25 +103,20 @@ class CommandPoolVK final {
103103
class CommandPoolRecyclerVK final
104104
: public std::enable_shared_from_this<CommandPoolRecyclerVK> {
105105
public:
106-
~CommandPoolRecyclerVK();
107-
108106
/// A unique command pool and zero or more recycled command buffers.
109107
struct RecycledData {
110108
vk::UniqueCommandPool pool;
111109
std::vector<vk::UniqueCommandBuffer> buffers;
112110
};
113111

114112
/// @brief Clean up resources held by all per-thread command pools
115-
/// associated with the given context.
116-
///
117-
/// @param[in] context The context.
118-
static void DestroyThreadLocalPools(const ContextVK* context);
113+
/// associated with the context.
114+
void DestroyThreadLocalPools();
119115

120116
/// @brief Creates a recycler for the given |ContextVK|.
121117
///
122118
/// @param[in] context The context to create the recycler for.
123-
explicit CommandPoolRecyclerVK(std::weak_ptr<ContextVK> context)
124-
: context_(std::move(context)) {}
119+
explicit CommandPoolRecyclerVK(const std::shared_ptr<ContextVK>& context);
125120

126121
/// @brief Gets a command pool for the current thread.
127122
///
@@ -137,11 +132,15 @@ class CommandPoolRecyclerVK final
137132
std::vector<vk::UniqueCommandBuffer>&& buffers,
138133
bool should_trim = false);
139134

140-
/// @brief Clears all recycled command pools to let them be reclaimed.
135+
/// @brief Clears this context's thread-local command pool.
141136
void Dispose();
142137

138+
// Visible for testing.
139+
static int GetGlobalPoolCount(const ContextVK& context);
140+
143141
private:
144142
std::weak_ptr<ContextVK> context_;
143+
uint64_t context_hash_;
145144

146145
Mutex recycled_mutex_;
147146
std::vector<RecycledData> recycled_ IPLR_GUARDED_BY(recycled_mutex_);

‎engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,5 +228,24 @@ TEST(CommandPoolRecyclerVKTest, ExtraCommandBufferAllocationsTriggerTrim) {
228228
context->Shutdown();
229229
}
230230

231+
TEST(CommandPoolRecyclerVKTest, RecyclerGlobalPoolMapSize) {
232+
auto context = MockVulkanContextBuilder().Build();
233+
auto const recycler = context->GetCommandPoolRecycler();
234+
235+
// The global pool list for this context should initially be empty.
236+
EXPECT_EQ(CommandPoolRecyclerVK::GetGlobalPoolCount(*context), 0);
237+
238+
// Creating a pool for this thread should insert the pool into the global map.
239+
auto pool = recycler->Get();
240+
EXPECT_EQ(CommandPoolRecyclerVK::GetGlobalPoolCount(*context), 1);
241+
242+
// Disposing this thread's pool should remove it from the global map.
243+
pool.reset();
244+
recycler->Dispose();
245+
EXPECT_EQ(CommandPoolRecyclerVK::GetGlobalPoolCount(*context), 0);
246+
247+
context->Shutdown();
248+
}
249+
231250
} // namespace testing
232251
} // namespace impeller

‎engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,9 @@ ContextVK::~ContextVK() {
134134
if (device_holder_ && device_holder_->device) {
135135
[[maybe_unused]] auto result = device_holder_->device->waitIdle();
136136
}
137-
CommandPoolRecyclerVK::DestroyThreadLocalPools(this);
137+
if (command_pool_recycler_) {
138+
command_pool_recycler_->DestroyThreadLocalPools();
139+
}
138140
}
139141

140142
Context::BackendType ContextVK::GetBackendType() const {
@@ -421,7 +423,7 @@ void ContextVK::Setup(Settings settings) {
421423
}
422424

423425
auto command_pool_recycler =
424-
std::make_shared<CommandPoolRecyclerVK>(weak_from_this());
426+
std::make_shared<CommandPoolRecyclerVK>(shared_from_this());
425427
if (!command_pool_recycler) {
426428
VALIDATION_LOG << "Could not create command pool recycler.";
427429
return;

0 commit comments

Comments
 (0)