Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] started cleaning up pools when devices are deleted #41857

Merged
auto-submit merged 5 commits into from
May 9, 2023
Merged

[Impeller] started cleaning up pools when devices are deleted #41857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d13af8
started cleaning up pools when devices are deleted
gaaclarke May 5, 2023
d0f1428
added test
gaaclarke May 9, 2023
9009fa6
update license file
gaaclarke May 9, 2023
32cf60d
removed validation `abort()` and strong hold on the context
gaaclarke May 9, 2023
9f5c04c
format
gaaclarke May 9, 2023
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 ci/licenses_golden/excluded_files
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@
../../../flutter/impeller/image/README.md
../../../flutter/impeller/playground
../../../flutter/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc
../../../flutter/impeller/renderer/backend/vulkan/context_vk_unittests.cc
../../../flutter/impeller/renderer/backend/vulkan/test
../../../flutter/impeller/renderer/capabilities_unittests.cc
../../../flutter/impeller/renderer/compute_subgroup_unittests.cc
Expand Down
1 change: 1 addition & 0 deletions impeller/renderer/backend/vulkan/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ impeller_component("vulkan_unittests") {
testonly = true
sources = [
"blit_command_vk_unittests.cc",
"context_vk_unittests.cc",
"test/mock_vulkan.cc",
"test/mock_vulkan.h",
]
Expand Down
5 changes: 2 additions & 3 deletions impeller/renderer/backend/vulkan/command_encoder_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ class TrackedObjectsVK {
}
auto pool = pool_.lock();
if (!pool) {
VALIDATION_LOG
<< "Command pool died before a command buffer could be recycled.";
// The buffer can not be freed if its command pool has been destroyed.
buffer_.release();
return;
Expand Down Expand Up @@ -159,7 +157,8 @@ bool CommandEncoderVK::Submit() {

return fence_waiter_->AddFence(
std::move(fence), [tracked_objects = std::move(tracked_objects_)] {
// Nothing to do, we just drop the tracked objects on the floor.
// Nothing to do, we just drop the tracked
// objects on the floor.
});
}

Expand Down
7 changes: 3 additions & 4 deletions impeller/renderer/backend/vulkan/command_pool_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@
namespace impeller {

using CommandPoolMap = std::map<uint64_t, std::shared_ptr<CommandPoolVK>>;

// TODO(https://github.com/flutter/flutter/issues/125571): This is storing tons
// of CommandPoolVK's in the test runner since many contexts are created on the
// same thread. We need to come up with a different way to clean these up.
FML_THREAD_LOCAL fml::ThreadLocalUniquePtr<CommandPoolMap> tls_command_pool;

static Mutex g_all_pools_mutex;
Expand Down Expand Up @@ -52,6 +48,9 @@ std::shared_ptr<CommandPoolVK> CommandPoolVK::GetThreadLocal(
}

void CommandPoolVK::ClearAllPools(const ContextVK* context) {
if (tls_command_pool.get()) {
tls_command_pool.get()->erase(context->GetHash());
Copy link
Member

@jason-simmons jason-simmons May 9, 2023

Choose a reason for hiding this comment

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

Move this to the end of the function so it runs after the block that calls Reset on every CommandPoolVK associated with this context.

In theory this should be unnecessary because nobody else should be holding on to a strong reference to this thread's CommandPoolVK, and the erase should destruct it and do all the necessary cleanup. But for safety I think if would be prudent to ensure that the current thread's CommandPoolVK for this context is explicitly reset at the same time as the ones for other threads.

Copy link
Member

@jason-simmons jason-simmons May 9, 2023

Choose a reason for hiding this comment

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

Actually I just realized this is unnecessary. If this thread's CommandPoolVK is still alive after the tls_command_pool.erase, then the corresponding weak_ptr in g_all_pools will still be promotable to a shared_ptr and will get the Reset call.

So the current implementation in the PR will work as intended.

}
Lock pool_lock(g_all_pools_mutex);
if (auto found = g_all_pools.find(context); found != g_all_pools.end()) {
for (auto& weak_pool : found->second) {
Expand Down
30 changes: 30 additions & 0 deletions impeller/renderer/backend/vulkan/context_vk_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// 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/testing/testing.h"
#include "impeller/renderer/backend/vulkan/command_pool_vk.h"
#include "impeller/renderer/backend/vulkan/context_vk.h"
#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h"

namespace impeller {
namespace testing {

TEST(ContextVKTest, DeletesCommandPools) {
std::weak_ptr<ContextVK> weak_context;
std::weak_ptr<CommandPoolVK> weak_pool;
{
std::shared_ptr<ContextVK> context = CreateMockVulkanContext();
std::shared_ptr<CommandPoolVK> pool =
CommandPoolVK::GetThreadLocal(context.get());
weak_pool = pool;
weak_context = context;
ASSERT_TRUE(weak_pool.lock());
ASSERT_TRUE(weak_context.lock());
}
ASSERT_FALSE(weak_pool.lock());
ASSERT_FALSE(weak_context.lock());
}

} // namespace testing
} // namespace impeller