This has vulkan resources that aren't being cleaned up until the accessing thread is destroyed and there is no guarantee that that is happening before the vulkan context is destroyed. DisposeThreadLocalCachedResources will clear them out, but is that getting called for all threads that are calling CreateCommandBuffer()?

I think so?

Thread local storage is really tricky to get right and this has bitten us in the past when it's storing vulkan resources. It's actually a violation of the c++ style guide that states static variables should only be trivially destructed. Can we instead pass in the pool as an argument to creating a command buffer? Then objects on each thread can own a pool with proper scoping.

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Can we instead pass in the pool as an argument to creating a command buffer?

That doesn't really solve the problem this code is trying to solve - we need to share the descriptor pool across many command buffers as they are expensive to create, but we don't really have any other place to store these besides TLS in the context.

I agree that its bad, I think a better design would be having a single thread local object that provided access to all thread specific APIs. But I'm not sure how to make that work at the abstraction layer we have, because it would be vulkan only AFAIK.

I think if you make the pool a required argument you can percolate it up the call stack until there is a location that is somewhat synonymous with being associated with a thread. There is a absl wrapper from objects that leaks them that can be used for static variables for this. I can't recall it off the top of my head. At a bare minimum we should do that to avoid weird crashes.

I can go back to the lock on the hashmap in the meantime?

I just looked over that version and seems reasonable to me.

I think if you make the pool a required argument you can percolate it up the call stack until there is a location that is somewhat synonymous with being associated with a thread. There is a absl wrapper from objects that leaks them that can be used for static variables for this. I can't recall it off the top of my head. At a bare minimum we should do that to avoid weird crashes.

The pool is a vulkan specifc class. I need to redesign parts of the hal to lets this sort of abstraction exist across platforms. Then you're right, I can just have the rasterizer own it and release it at the end of the frame instead of the context doing it magically.

The thread-local command pools handled this by keeping a global map of every command pool created on behalf of each ContextVK.

When a ContextVK is deleted, it will release the resources held by all of its per-thread pools. (See g_all_pools_map_mutex and CommandPoolRecyclerVK::DestroyThreadLocalPools)

Do we need a lock in ~ContextVK, or do we have a guarentee that no one will be calling these from a separate thread while the contextvk is being deleted?

hmm. I'm not sure.

I think no, because the the destructor can only run once there are no more references to the context. So nothing else could be calling anything that references the desc pool? (unless done through a raw or weak ptr during destruction)

If a ContextVK instance is being deleted, then the current thread is releasing the last reference to that instance and no other thread could be holding any references.

So there is no need to acquire any lock when the cached_descriptor_pool_ member is destructed.

Thank you Jason

Minimize the scope where this lock is held:

{
  Lock lock(desc_pool_mutex_);

 
 cached_descriptor_pool_.erase(std::this_thread::get_id());
}
command_pool_recycler_->Dispose();

Done