[Lldb-commits] [PATCH] D131996: Use a SmallPtrSet rather than a SmallVector in ClusterManager.

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, jasonmolenda, kastiglione, labath.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The m_objects store in ClusterManager holds pointers to all the objects
managed by the ClusterManager.  It is used to check whether
this ClusterManager already owns this pointer before adding it to
the ClusterManager's shared pointer or getting the shared pointer for the object.  
When the ClusterManager is deleted, we iterate over it to destroy all the managed objects.
The most common operation by far is "GetSharedPointer" on the cluster.

With a SmallVector and llvm::is_contained the performance was non-linear
in the number of elements.  For instance, printing all the elements of a 16M element std::vector
didn't complete in the time I was willing to wait for it (hours).

      

Since we are mostly doing insert & contains, some kind of set is likely to be a
better data structure.  In this patch I used SmallPtrSet.  With
that, the same array prints out in 30 seconds.  I couldn't see any noticeable difference
for ValueObjects with a small number of children.  I also tried a the same patch using a
std::unordered_set but that was slightly slower and used a fair bit
more memory than the SmallPtrSet.

      

Other than performance, this is NFC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131996

Files:
  lldb/include/lldb/Utility/SharedCluster.h


Index: lldb/include/lldb/Utility/SharedCluster.h
===================================================================
--- lldb/include/lldb/Utility/SharedCluster.h
+++ lldb/include/lldb/Utility/SharedCluster.h
@@ -11,7 +11,7 @@
 
 #include "lldb/Utility/LLDBAssert.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
 
 #include <memory>
 #include <mutex>
@@ -32,15 +32,15 @@
 
   void ManageObject(T *new_object) {
     std::lock_guard<std::mutex> guard(m_mutex);
-    assert(!llvm::is_contained(m_objects, new_object) &&
-           "ManageObject called twice for the same object?");
-    m_objects.push_back(new_object);
+    auto ret = m_objects.insert(new_object);
+    assert(ret.second && "ManageObject called twice for the same object?");
   }
 
   std::shared_ptr<T> GetSharedPointer(T *desired_object) {
     std::lock_guard<std::mutex> guard(m_mutex);
     auto this_sp = this->shared_from_this();
-    if (!llvm::is_contained(m_objects, desired_object)) {
+    size_t count =  m_objects.count(desired_object);
+    if (count == 0) {
       lldbassert(false && "object not found in shared cluster when expected");
       desired_object = nullptr;
     }
@@ -49,8 +49,7 @@
 
 private:
   ClusterManager() : m_objects() {}
-
-  llvm::SmallVector<T *, 16> m_objects;
+  llvm::SmallPtrSet<T *, 16> m_objects;
   std::mutex m_mutex;
 };
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D131996.453127.patch
Type: text/x-patch
Size: 1398 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220816/08470e73/attachment.bin>