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 3 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
10 changes: 6 additions & 4 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 @@ -157,10 +157,12 @@ bool CommandEncoderVK::Submit() {
return false;
}

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.
});
return fence_waiter_->AddFence(std::move(fence),
[tracked_objects = std::move(tracked_objects_),
strong_device = std::move(strong_device)] {

@jason-simmons jason-simmons May 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this capturing a strong reference to the ContextVK in the lambda?

TrackedObjectsVK holds a weak reference to the CommandPoolVK. That should allow cleanup of the tracked objects if the CommandPoolVK and the ContextVK/vk::Device are still alive (and do nothing if they are already gone)

@gaaclarke gaaclarke May 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's because that buffer.release() call that you just added wasn't in the TrackedObjects destructor. This may no longer be necessary after your change. Let me test it.

@gaaclarke gaaclarke May 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope, is still necessary. It crashes without it because VALIDATION_LOG has an abort in it. I'll remove it.

@gaaclarke gaaclarke May 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Our options are to keep a strong hold on the device in the fence, or to release the buffer without crashing. I believe you prefer removing the strong link and calling release(). I think I do too so I did that, although I'm unhappy about the C++ vulkan wrapper and how it looks like a leak.

// Nothing to do, we just drop the tracked
// objects on the floor.
});
}

vk::CommandBuffer CommandEncoderVK::GetCommandBuffer() const {
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());

@jason-simmons jason-simmons May 9, 2023

Copy link
Copy Markdown
Contributor

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.

@jason-simmons jason-simmons May 9, 2023

Copy link
Copy Markdown
Contributor

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