layers: Add BASE_NODE tree locking

Maintaining the tree of which objects are in use by command buffers is
performance critical and also likely to cause interactions between
threads. Make access to the tree thread safe by guarding each
nodes parent_nodes_ set with a separate r/w lock.

layers/base_node.cpp

@@ -36,8 +36,15 @@ void BASE_NODE::Destroy() {
 }
 
 bool BASE_NODE::InUse() const {
+    // NOTE: for performance reasons, this method calls up the tree
+    // with the read lock held.
+    auto guard = ReadLockTree();
     bool result = false;
-    for (auto& node: parent_nodes_) {
+    for (auto& item : parent_nodes_) {
+        auto node = item.second.lock();
+        if (!node) {
+            continue;
+        }
         result |= node->InUse();
         if (result) {
             break;
@@ -47,35 +54,58 @@ bool BASE_NODE::InUse() const {
 }
 
 bool BASE_NODE::AddParent(BASE_NODE *parent_node) {
-    auto result = parent_nodes_.emplace(parent_node);
+    auto guard = WriteLockTree();
+    auto result = parent_nodes_.emplace(parent_node->Handle(), std::weak_ptr<BASE_NODE>(parent_node->shared_from_this()));
     return result.second;
 }
 
 void BASE_NODE::RemoveParent(BASE_NODE *parent_node) {
-    parent_nodes_.erase(parent_node);
+    assert(parent_node);
+    auto guard = WriteLockTree();
+    parent_nodes_.erase(parent_node->Handle());
 }
 
-void BASE_NODE::Invalidate(bool unlink) {
-    NodeList invalid_nodes;
-    invalid_nodes.emplace_back(this);
-    for (auto& node: parent_nodes_) {
-        node->NotifyInvalidate(invalid_nodes, unlink);
-    }
+// copy the current set of parents so that we don't need to hold the lock
+// while calling NotifyInvalidate on them, as that would lead to recursive locking.
+BASE_NODE::NodeMap BASE_NODE::GetParentsForInvalidate(bool unlink) {
+    NodeMap result;
     if (unlink) {
+        auto guard = WriteLockTree();
+        result = std::move(parent_nodes_);
         parent_nodes_.clear();
+    } else {
+        auto guard = ReadLockTree();
+        result = parent_nodes_;
     }
+    return result;
+}
+
+BASE_NODE::NodeMap BASE_NODE::ObjectBindings() const {
+    auto guard = ReadLockTree();
+    return parent_nodes_;
+}
+
+void BASE_NODE::Invalidate(bool unlink) {
+    NodeList empty;
+    // We do not want to call the virtual method here because any special handling
+    // in an overriden NotifyInvalidate() is for when a child node has become invalid.
+    // But calling Invalidate() indicates the current node is invalid.
+    // Calling the default implementation directly here avoids duplicating it inline.
+    BASE_NODE::NotifyInvalidate(empty, unlink);
 }
 
 void BASE_NODE::NotifyInvalidate(const NodeList& invalid_nodes, bool unlink) {
-    if (parent_nodes_.size() == 0) {
+    auto current_parents = GetParentsForInvalidate(unlink);
+    if (current_parents.size() == 0) {
         return;
     }
+
     NodeList up_nodes = invalid_nodes;
-    up_nodes.emplace_back(this);
-    for (auto& node: parent_nodes_) {
-        node->NotifyInvalidate(up_nodes, unlink);
-    }
-    if (unlink) {
-        parent_nodes_.clear();
+    up_nodes.emplace_back(shared_from_this());
+    for (auto& item : current_parents) {
+        auto node = item.second.lock();
+        if (node && !node->Destroyed()) {
+            node->NotifyInvalidate(up_nodes, unlink);
+        }
     }
 }

layers/base_node.h

@@ -31,6 +31,7 @@
 #include "vk_object_types.h"
 #include "vk_layer_data.h"
 #include "vk_layer_logging.h"
+#include "vk_layer_utils.h"
 
 #include <atomic>
 
@@ -45,16 +46,32 @@ struct hash<VulkanTypedHandle> {
 };
 }  // namespace std
 
-class BASE_NODE {
+// inheriting from enable_shared_from_this<> adds a method, shared_from_this(), which
+// returns a shared_ptr version of the current object. It requires the object to
+// be created with std::make_shared<> and it MUST NOT be used from the constructor
+class BASE_NODE : public std::enable_shared_from_this<BASE_NODE> {
   public:
-    using NodeSet = layer_data::unordered_set<BASE_NODE *>;
-    using NodeList = small_vector<BASE_NODE *, 4>;
+    // Parent nodes are stored as weak_ptrs to avoid cyclic memory dependencies.
+    // Because weak_ptrs cannot safely be used as hash keys, the parents are stored
+    // in a map keyed by VulkanTypedHandle. This also allows looking for specific
+    // parent types without locking every weak_ptr.
+    using NodeMap = layer_data::unordered_map<VulkanTypedHandle, std::weak_ptr<BASE_NODE>>;
+    using NodeList = small_vector<std::shared_ptr<BASE_NODE>, 4, uint32_t>;
 
     template <typename Handle>
     BASE_NODE(Handle h, VulkanObjectType t) : handle_(h, t), destroyed_(false) {}
 
+    // because shared_from_this() does not work from the constructor, this 2nd phase
+    // constructor is where a state object should call AddParent() on its child nodes.
+    // It is called as part of ValidationStateTracker::Add()
+    virtual void LinkChildNodes() {}
+
     virtual ~BASE_NODE();
 
+    // Because state objects are reference counted, they may outlive the vulkan objects
+    // they represent. Destroy() is called when the vulkan object is destroyed, so that
+    // it can be cleaned up before all references are gone. It also triggers notifications
+    // to parent objects.
     virtual void Destroy();
 
     bool Destroyed() const { return destroyed_; }
@@ -65,25 +82,38 @@ class BASE_NODE {
     virtual bool InUse() const;
 
     virtual bool AddParent(BASE_NODE *parent_node);
-
     virtual void RemoveParent(BASE_NODE *parent_node);
 
+    // Invalidate is called on a state object to inform its parents that it
+    // is being destroyed (unlink == true) or otherwise becoming invalid (unlink == false)
     void Invalidate(bool unlink = true);
 
+    // Helper to let objects examine their immediate parents without holding the tree lock.
+    NodeMap ObjectBindings() const;
+
   protected:
-    // NOTE: the entries in invalid_nodes will likely be destroyed & deleted
-    // after the NotifyInvalidate() calls finish.
+    // Called recursively for every parent object of something that has become invalid
     virtual void NotifyInvalidate(const NodeList &invalid_nodes, bool unlink);
 
+    // returns a copy of the current set of parents so that they can be walked
+    // without the tree lock held. If unlink == true, parent_nodes_ is also cleared.
+    NodeMap GetParentsForInvalidate(bool unlink);
+
     VulkanTypedHandle handle_;
 
     // Set to true when the API-level object is destroyed, but this object may
     // hang around until its shared_ptr refcount goes to zero.
-    bool destroyed_;
+    std::atomic<bool> destroyed_;
+
+  private:
+    ReadLockGuard ReadLockTree() const { return ReadLockGuard(tree_lock_); }
+    WriteLockGuard WriteLockTree() { return WriteLockGuard(tree_lock_); }
 
     // Set of immediate parent nodes for this object. For an in-use object, the
     // parent nodes should form a tree with the root being a command buffer.
-    NodeSet parent_nodes_;
+    NodeMap parent_nodes_;
+    // Lock guarding parent_nodes_, this lock MUST NOT be used for other purposes.
+    mutable ReadWriteLock tree_lock_;
 };
 
 class REFCOUNTED_NODE : public BASE_NODE {

layers/best_practices_utils.cpp

@@ -1,6 +1,6 @@
-/* Copyright (c) 2015-2021 The Khronos Group Inc.
- * Copyright (c) 2015-2021 Valve Corporation
- * Copyright (c) 2015-2021 LunarG, Inc.
+/* Copyright (c) 2015-2022 The Khronos Group Inc.
+ * Copyright (c) 2015-2022 Valve Corporation
+ * Copyright (c) 2015-2022 LunarG, Inc.
  * Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -705,8 +705,8 @@ bool BestPractices::PreCallValidateFreeMemory(VkDevice device, VkDeviceMemory me
 
     const auto mem_info = Get<DEVICE_MEMORY_STATE>(memory);
 
-    for (const auto& node: mem_info->ObjectBindings()) {
-        const auto& obj = node->Handle();
+    for (const auto& item : mem_info->ObjectBindings()) {
+        const auto& obj = item.first;
         LogObjectList objlist(device);
         objlist.add(obj);
         objlist.add(mem_info->mem());

layers/buffer_state.h

@@ -54,10 +54,11 @@ class BUFFER_VIEW_STATE : public BASE_NODE {
 
     BUFFER_VIEW_STATE(const std::shared_ptr<BUFFER_STATE> &bf, VkBufferView bv, const VkBufferViewCreateInfo *ci,
                       VkFormatFeatureFlags2KHR ff)
-        : BASE_NODE(bv, kVulkanObjectTypeBufferView), create_info(*ci), buffer_state(bf), format_features(ff) {
-        if (buffer_state) {
-            buffer_state->AddParent(this);
-        }
+        : BASE_NODE(bv, kVulkanObjectTypeBufferView), create_info(*ci), buffer_state(bf), format_features(ff) {}
+
+    void LinkChildNodes() override {
+        // Connect child node(s), which cannot safely be done in the constructor.
+        buffer_state->AddParent(this);
     }
     virtual ~BUFFER_VIEW_STATE() {
         if (!Destroyed()) {

layers/cmd_buffer_state.cpp

@@ -252,14 +252,14 @@ const IMAGE_VIEW_STATE *CMD_BUFFER_STATE::GetActiveAttachmentImageViewState(uint
     return active_attachments->at(index);
 }
 
-void CMD_BUFFER_STATE::AddChild(BASE_NODE *child_node) {
+void CMD_BUFFER_STATE::AddChild(std::shared_ptr<BASE_NODE> &child_node) {
     assert(child_node);
     if (child_node->AddParent(this)) {
         object_bindings.insert(child_node);
     }
 }
 
-void CMD_BUFFER_STATE::RemoveChild(BASE_NODE *child_node) {
+void CMD_BUFFER_STATE::RemoveChild(std::shared_ptr<BASE_NODE> &child_node) {
     assert(child_node);
     child_node->RemoveParent(this);
     object_bindings.erase(child_node);
@@ -357,12 +357,6 @@ void CMD_BUFFER_STATE::Reset() {
 
     transform_feedback_active = false;
 
-    // Remove object bindings
-    for (auto *base_obj : object_bindings) {
-        RemoveChild(base_obj);
-    }
-    object_bindings.clear();
-
     // Clean up the label data
     ResetCmdDebugUtilsLabel(dev_data->report_data, commandBuffer());
 
@@ -458,20 +452,20 @@ void CMD_BUFFER_STATE::NotifyInvalidate(const BASE_NODE::NodeList &invalid_nodes
     }
     assert(!invalid_nodes.empty());
     LogObjectList log_list;
-    for (auto *obj : invalid_nodes) {
+    for (auto &obj : invalid_nodes) {
         log_list.object_list.emplace_back(obj->Handle());
     }
     broken_bindings.emplace(invalid_nodes[0]->Handle(), log_list);
 
     if (unlink) {
-        for (auto *obj : invalid_nodes) {
+        for (auto &obj : invalid_nodes) {
             object_bindings.erase(obj);
             switch (obj->Type()) {
                 case kVulkanObjectTypeCommandBuffer:
-                    linkedCommandBuffers.erase(static_cast<CMD_BUFFER_STATE *>(obj));
+                    linkedCommandBuffers.erase(static_cast<CMD_BUFFER_STATE *>(obj.get()));
                     break;
                 case kVulkanObjectTypeImage:
-                    image_layout_map.erase(static_cast<IMAGE_STATE *>(obj));
+                    image_layout_map.erase(static_cast<IMAGE_STATE *>(obj.get()));
                     break;
                 default:
                     break;
@@ -626,7 +620,7 @@ void CMD_BUFFER_STATE::BeginRenderPass(CMD_TYPE cmd_type, const VkRenderPassBegi
 
     // Connect this RP to cmdBuffer
     if (!dev_data->disabled[command_buffer_state] && activeRenderPass) {
-        AddChild(activeRenderPass.get());
+        AddChild(activeRenderPass);
     }
 
     auto chained_device_group_struct = LvlFindInChain<VkDeviceGroupRenderPassBeginInfo>(pRenderPassBegin->pNext);
@@ -652,7 +646,7 @@ void CMD_BUFFER_STATE::BeginRenderPass(CMD_TYPE cmd_type, const VkRenderPassBegi
         UpdateAttachmentsView(pRenderPassBegin);
 
         // Connect this framebuffer and its children to this cmdBuffer
-        AddChild(activeFramebuffer.get());
+        AddChild(activeFramebuffer);
     }
 }
 
@@ -783,7 +777,7 @@ void CMD_BUFFER_STATE::Begin(const VkCommandBufferBeginInfo *pBeginInfo) {
 
                         // Connect this framebuffer and its children to this cmdBuffer
                         if (!dev_data->disabled[command_buffer_state]) {
-                            AddChild(activeFramebuffer.get());
+                            AddChild(activeFramebuffer);
                         }
                     }
                 }
@@ -853,7 +847,7 @@ void CMD_BUFFER_STATE::ExecuteCommands(uint32_t commandBuffersCount, const VkCom
 
         sub_cb_state->primaryCommandBuffer = commandBuffer();
         linkedCommandBuffers.insert(sub_cb_state.get());
-        AddChild(sub_cb_state.get());
+        AddChild(sub_cb_state);
         for (auto &function : sub_cb_state->queryUpdates) {
             queryUpdates.push_back(function);
         }
@@ -1178,10 +1172,10 @@ void CMD_BUFFER_STATE::RecordStateCmd(CMD_TYPE cmd_type, CBStatusFlags state_bit
 void CMD_BUFFER_STATE::RecordTransferCmd(CMD_TYPE cmd_type, std::shared_ptr<BINDABLE> &&buf1, std::shared_ptr<BINDABLE> &&buf2) {
     RecordCmd(cmd_type);
     if (buf1) {
-        AddChild(buf1.get());
+        AddChild(buf1);
     }
     if (buf2) {
-        AddChild(buf2.get());
+        AddChild(buf2);
     }
 }
 
@@ -1195,7 +1189,7 @@ void CMD_BUFFER_STATE::RecordSetEvent(CMD_TYPE cmd_type, VkEvent event, VkPipeli
     if (!dev_data->disabled[command_buffer_state]) {
         auto event_state = dev_data->Get<EVENT_STATE>(event);
         if (event_state) {
-            AddChild(event_state.get());
+            AddChild(event_state);
         }
     }
     events.push_back(event);
@@ -1213,7 +1207,7 @@ void CMD_BUFFER_STATE::RecordResetEvent(CMD_TYPE cmd_type, VkEvent event, VkPipe
     if (!dev_data->disabled[command_buffer_state]) {
         auto event_state = dev_data->Get<EVENT_STATE>(event);
         if (event_state) {
-            AddChild(event_state.get());
+            AddChild(event_state);
         }
     }
     events.push_back(event);
@@ -1232,7 +1226,7 @@ void CMD_BUFFER_STATE::RecordWaitEvents(CMD_TYPE cmd_type, uint32_t eventCount,
         if (!dev_data->disabled[command_buffer_state]) {
             auto event_state = dev_data->Get<EVENT_STATE>(pEvents[i]);
             if (event_state) {
-                AddChild(event_state.get());
+                AddChild(event_state);
             }
         }
         waitedEvents.insert(pEvents[i]);
@@ -1248,13 +1242,13 @@ void CMD_BUFFER_STATE::RecordBarriers(uint32_t memoryBarrierCount, const VkMemor
     for (uint32_t i = 0; i < bufferMemoryBarrierCount; i++) {
         auto buffer_state = dev_data->Get<BUFFER_STATE>(pBufferMemoryBarriers[i].buffer);
         if (buffer_state) {
-            AddChild(buffer_state.get());
+            AddChild(buffer_state);
         }
     }
     for (uint32_t i = 0; i < imageMemoryBarrierCount; i++) {
         auto image_state = dev_data->Get<IMAGE_STATE>(pImageMemoryBarriers[i].image);
         if (image_state) {
-            AddChild(image_state.get());
+            AddChild(image_state);
         }
     }
 }
@@ -1265,13 +1259,13 @@ void CMD_BUFFER_STATE::RecordBarriers(const VkDependencyInfoKHR &dep_info) {
     for (uint32_t i = 0; i < dep_info.bufferMemoryBarrierCount; i++) {
         auto buffer_state = dev_data->Get<BUFFER_STATE>(dep_info.pBufferMemoryBarriers[i].buffer);
         if (buffer_state) {
-            AddChild(buffer_state.get());
+            AddChild(buffer_state);
         }
     }
     for (uint32_t i = 0; i < dep_info.imageMemoryBarrierCount; i++) {
         auto image_state = dev_data->Get<IMAGE_STATE>(dep_info.pImageMemoryBarriers[i].image);
         if (image_state) {
-            AddChild(image_state.get());
+            AddChild(image_state);
         }
     }
 }
@@ -1283,7 +1277,7 @@ void CMD_BUFFER_STATE::RecordWriteTimestamp(CMD_TYPE cmd_type, VkPipelineStageFl
 
     if (!dev_data->disabled[command_buffer_state]) {
         auto pool_state = dev_data->Get<QUERY_POOL_STATE>(queryPool);
-        AddChild(pool_state.get());
+        AddChild(pool_state);
     }
     QueryObject query = {queryPool, slot};
     EndQuery(query);