[llvm] f6b3875 - Reapply "lldb: Cache string hash during ConstString pool queries/insertions"

[David Blaikie via llvm-commits]

Author: David Blaikie
Date: 2024-02-02T20:01:51Z
New Revision: f6b387589d648945372528a4ac77c58f310e5165

URL: https://github.com/llvm/llvm-project/commit/f6b387589d648945372528a4ac77c58f310e5165
DIFF: https://github.com/llvm/llvm-project/commit/f6b387589d648945372528a4ac77c58f310e5165.diff

LOG: Reapply "lldb: Cache string hash during ConstString pool queries/insertions"

Reverted due to an internally discovered lld crash due to the underlying
StringMap changes, which turned out to be an existing lld bug that got
tickled by the StringMap changes. That's addressed in
dee8786f70a3d62b639113343fa36ef55bdbad63 so let's have another go with
this change.

Original commit message:
    lldb was rehashing the string 3 times (once to determine which StringMap
    to use, once to query the StringMap, once to insert) on insertion (twice
    on successful lookup).

    This patch allows the lldb to benefit from hash improvements in LLVM
    (from djbHash to xxh3).

    Though further changes would be needed to cache this value to disk - we
    shouldn't rely on the StringMap::hash remaining the same in the
    future/this value should not be serialized to disk. If we want cache
    this value StringMap should take a hashing template parameter to allow
    for a fixed hash to be requested.

This reverts commit 5bc1adff69315dcef670e9fcbe04067b5d5963fb.
Effectively reapplying the original 2e197602305be18b963928e6ae024a004a95af6d.

Added: 
    

Modified: 
    lldb/source/Utility/ConstString.cpp
    llvm/lib/Support/StringMap.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Utility/ConstString.cpp b/lldb/source/Utility/ConstString.cpp
index 4535771adfb73..ea897dc611cc9 100644
--- a/lldb/source/Utility/ConstString.cpp
+++ b/lldb/source/Utility/ConstString.cpp
@@ -77,10 +77,10 @@ class Pool {
     return 0;
   }
 
-  StringPoolValueType GetMangledCounterpart(const char *ccstr) const {
+  StringPoolValueType GetMangledCounterpart(const char *ccstr) {
     if (ccstr != nullptr) {
-      const uint8_t h = hash(llvm::StringRef(ccstr));
-      llvm::sys::SmartScopedReader<false> rlock(m_string_pools[h].m_mutex);
+      const PoolEntry &pool = selectPool(llvm::StringRef(ccstr));
+      llvm::sys::SmartScopedReader<false> rlock(pool.m_mutex);
       return GetStringMapEntryFromKeyData(ccstr).getValue();
     }
     return nullptr;
@@ -100,19 +100,20 @@ class Pool {
 
   const char *GetConstCStringWithStringRef(llvm::StringRef string_ref) {
     if (string_ref.data()) {
-      const uint8_t h = hash(string_ref);
+      const uint32_t string_hash = StringPool::hash(string_ref);
+      PoolEntry &pool = selectPool(string_hash);
 
       {
-        llvm::sys::SmartScopedReader<false> rlock(m_string_pools[h].m_mutex);
-        auto it = m_string_pools[h].m_string_map.find(string_ref);
-        if (it != m_string_pools[h].m_string_map.end())
+        llvm::sys::SmartScopedReader<false> rlock(pool.m_mutex);
+        auto it = pool.m_string_map.find(string_ref, string_hash);
+        if (it != pool.m_string_map.end())
           return it->getKeyData();
       }
 
-      llvm::sys::SmartScopedWriter<false> wlock(m_string_pools[h].m_mutex);
+      llvm::sys::SmartScopedWriter<false> wlock(pool.m_mutex);
       StringPoolEntryType &entry =
-          *m_string_pools[h]
-               .m_string_map.insert(std::make_pair(string_ref, nullptr))
+          *pool.m_string_map
+               .insert(std::make_pair(string_ref, nullptr), string_hash)
                .first;
       return entry.getKeyData();
     }
@@ -125,12 +126,14 @@ class Pool {
     const char *demangled_ccstr = nullptr;
 
     {
-      const uint8_t h = hash(demangled);
-      llvm::sys::SmartScopedWriter<false> wlock(m_string_pools[h].m_mutex);
+      const uint32_t demangled_hash = StringPool::hash(demangled);
+      PoolEntry &pool = selectPool(demangled_hash);
+      llvm::sys::SmartScopedWriter<false> wlock(pool.m_mutex);
 
       // Make or update string pool entry with the mangled counterpart
-      StringPool &map = m_string_pools[h].m_string_map;
-      StringPoolEntryType &entry = *map.try_emplace(demangled).first;
+      StringPool &map = pool.m_string_map;
+      StringPoolEntryType &entry =
+          *map.try_emplace_with_hash(demangled, demangled_hash).first;
 
       entry.second = mangled_ccstr;
 
@@ -141,8 +144,8 @@ class Pool {
     {
       // Now assign the demangled const string as the counterpart of the
       // mangled const string...
-      const uint8_t h = hash(llvm::StringRef(mangled_ccstr));
-      llvm::sys::SmartScopedWriter<false> wlock(m_string_pools[h].m_mutex);
+      PoolEntry &pool = selectPool(llvm::StringRef(mangled_ccstr));
+      llvm::sys::SmartScopedWriter<false> wlock(pool.m_mutex);
       GetStringMapEntryFromKeyData(mangled_ccstr).setValue(demangled_ccstr);
     }
 
@@ -171,17 +174,20 @@ class Pool {
   }
 
 protected:
-  uint8_t hash(llvm::StringRef s) const {
-    uint32_t h = llvm::djbHash(s);
-    return ((h >> 24) ^ (h >> 16) ^ (h >> 8) ^ h) & 0xff;
-  }
-
   struct PoolEntry {
     mutable llvm::sys::SmartRWMutex<false> m_mutex;
     StringPool m_string_map;
   };
 
   std::array<PoolEntry, 256> m_string_pools;
+
+  PoolEntry &selectPool(const llvm::StringRef &s) {
+    return selectPool(StringPool::hash(s));
+  }
+
+  PoolEntry &selectPool(uint32_t h) {
+    return m_string_pools[((h >> 24) ^ (h >> 16) ^ (h >> 8) ^ h) & 0xff];
+  }
 };
 
 // Frameworks and dylibs aren't supposed to have global C++ initializers so we
@@ -197,7 +203,7 @@ static Pool &StringPool() {
   static Pool *g_string_pool = nullptr;
 
   llvm::call_once(g_pool_initialization_flag,
-                 []() { g_string_pool = new Pool(); });
+                  []() { g_string_pool = new Pool(); });
 
   return *g_string_pool;
 }

diff  --git a/llvm/lib/Support/StringMap.cpp b/llvm/lib/Support/StringMap.cpp
index 451108d01d38a..432e1fc343f1f 100644
--- a/llvm/lib/Support/StringMap.cpp
+++ b/llvm/lib/Support/StringMap.cpp
@@ -148,7 +148,7 @@ int StringMapImpl::FindKey(StringRef Key, uint32_t FullHashValue) const {
   if (NumBuckets == 0)
     return -1; // Really empty table?
 #ifdef EXPENSIVE_CHECKS
-  assert(FullHashValue == hash(Key);
+  assert(FullHashValue == hash(Key));
 #endif
   if (shouldReverseIterate())
     FullHashValue = ~FullHashValue;