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

[Impeller] removed collisions of dead command pools between tests. #41490

Merged
gaaclarke merged 1 commit into from
Apr 26, 2023
Merged

[Impeller] removed collisions of dead command pools between tests. #41490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 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 @@ -14,9 +14,11 @@

namespace impeller {

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

// TODO(tbd): 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.
Copy link
Contributor

@dnfield dnfield Apr 26, 2023

Choose a reason for hiding this comment

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

One option that I'm not 100% sure would work - make the key to this a std::weak_ptr<ContextVk>, and whenever the call is made to GetThreadLocal iterate through the map and delete dead weak_ptr keys.

Copy link
Contributor

@dnfield dnfield Apr 26, 2023

Choose a reason for hiding this comment

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

It should work if we change ContextVK to std::enable_shared_from_this and update ContextVK::Create to return shared_from_this() instead of creating a shared pointer, and then call GetThreadLocal(std::weak_from_this()).

Copy link
Member Author

@gaaclarke gaaclarke Apr 26, 2023

Choose a reason for hiding this comment

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

As noted above, this is how it was designed to work (the global variable keeps the pool alive long enough for the worker thread to use it, then destroys it when the thread is killed). I think we'll probably need to fix this design at some point and is a good reminder why the C++ style guide disallows nontrivially deleted global variables.

The alternative design would be something like make this variable a raw pointer and have the context flush and synchronize the worker thread before deleting the pool.

I'm just removing the crash and keeping the same design for now.

FML_THREAD_LOCAL fml::ThreadLocalUniquePtr<CommandPoolMap> tls_command_pool;

static Mutex g_all_pools_mutex;
Expand All @@ -33,15 +35,15 @@ std::shared_ptr<CommandPoolVK> CommandPoolVK::GetThreadLocal(
tls_command_pool.reset(new CommandPoolMap());
}
CommandPoolMap& pool_map = *tls_command_pool.get();
auto found = pool_map.find(context);
auto found = pool_map.find(context->GetHash());
Copy link
Contributor

@dnfield dnfield Apr 26, 2023

Choose a reason for hiding this comment

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

so here, just static_cast<uintptr_t>(context)

Copy link
Member Author

@gaaclarke gaaclarke Apr 26, 2023

Choose a reason for hiding this comment

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

ack

if (found != pool_map.end() && found->second->IsValid()) {
return found->second;
}
auto pool = std::shared_ptr<CommandPoolVK>(new CommandPoolVK(context));
if (!pool->IsValid()) {
return nullptr;
}
pool_map[context] = pool;
pool_map[context->GetHash()] = pool;
{
Lock pool_lock(g_all_pools_mutex);
g_all_pools[context].push_back(pool);
Expand Down
11 changes: 10 additions & 1 deletion impeller/renderer/backend/vulkan/context_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,16 @@ std::shared_ptr<ContextVK> ContextVK::Create(Settings settings) {
return context;
}

ContextVK::ContextVK() = default;
namespace {
thread_local uint64_t tls_context_count = 0;
uint64_t CalculateHash(void* ptr) {
// You could make a context once per nanosecond for 584 years on one thread
// before this overflows.
return ++tls_context_count;
}
} // namespace

ContextVK::ContextVK() : hash_(CalculateHash(this)) {}
Comment on lines +99 to +107
Copy link
Contributor

@dnfield dnfield Apr 26, 2023

Choose a reason for hiding this comment

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

Would it be simpler to just use the address of the object instead?

Copy link
Contributor

@dnfield dnfield Apr 26, 2023

Choose a reason for hiding this comment

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

(really I think we want a weak_ptr instead)

Copy link
Member Author

@gaaclarke gaaclarke Apr 26, 2023

Choose a reason for hiding this comment

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

Would it be simpler to just use the address of the object instead?

That's what it was before and that doesn't work because eventually you'll get a duplicate address and everything will come crashing down (details in the linked issue).

(really I think we want a weak_ptr instead)

It can't be a weak_ptr because the way the vulkan backend was designed is that this global variable is supposed to keep the pool alive until the thread is destroyed. One of the first things I did was try to make it be a weak_ptr but that would lead to a crash on the worker thread. I'm still seeing racy crashes so this may have to be changed eventually. I'm just trying to make the original design not crash.


ContextVK::~ContextVK() {
if (device_) {
Expand Down
3 changes: 3 additions & 0 deletions impeller/renderer/backend/vulkan/context_vk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ class ContextVK final : public Context, public BackendCast<ContextVK, Context> {

static std::shared_ptr<ContextVK> Create(Settings settings);

uint64_t GetHash() const { return hash_; }

// |Context|
~ContextVK() override;

Expand Down Expand Up @@ -134,6 +136,7 @@ class ContextVK final : public Context, public BackendCast<ContextVK, Context> {
std::shared_ptr<const Capabilities> device_capabilities_;
std::shared_ptr<FenceWaiterVK> fence_waiter_;
std::string device_name_;
const uint64_t hash_;

bool is_valid_ = false;

Expand Down