[Lldb-commits] [PATCH] D13652: Change ConstString to support massive multi-threaded access

[Tamas Berghammer via lldb-commits]

tberghammer added inline comments.

================
Comment at: source/Core/ConstString.cpp:147-152
@@ -165,7 +146,8 @@
 protected:
-    //------------------------------------------------------------------
-    // Typedefs
-    //------------------------------------------------------------------
-    typedef StringPool::iterator iterator;
-    typedef StringPool::const_iterator const_iterator;
+    uint8_t
+    hash(const llvm::StringRef &s)
+    {
+        uint32_t h = llvm::HashString(s);
+        return ((h >> 24) ^ (h >> 16) ^ (h >> 8) ^ h) & 0xff;
+    }
 
----------------
clayborg wrote:
> Is there a way we can use the hash that llvm uses for its string pool here? We are calculating two hashes: one for this to see which pool it will go into, and then another when the string is hashed into the string pool object. It would be nice if we can calculate the hash once and maybe do/add a string pool insertion that the pool with use to verify the string is in the pool or insert it using the supplied hash?
I don't see any reasonable way to avoid using 2 hash function without re-implementing llvm::StringMap with multi-threaded support in mind with per bucket mutextes. One of the issue is that llvm::StringMap don't have any interface where we can specify a hash value for an insert to avoid the calculation of the hash. The other problem is that we want to use a different hash function for selecting the pool and then selecting the bucket to achieve a uniform distribution between the buckets inside the StringMap. I am already a little bit concerned because we use 2 very similar hash function (StringMap use the LSB of llvm::HashString) what can cause some performance degradation.

I think a nice solution would be to use a hash map with built in multi-threaded support, or even better with a lock-free implementation (lock/unlock takes a lot of time) but I don't think implementing it would worth the effort.

================
Comment at: source/Core/ConstString.cpp:175
@@ -174,3 @@
-    //------------------------------------------------------------------
-    mutable Mutex m_mutex;
-    StringPool m_string_map;
----------------
zturner wrote:
> Did you consider changing this to an `llvm::RWMutex`?
I haven't tried it, but I don't see any easy way to use it because we use a single StringMap::insert call to read and possibly write to the map. If we want to get the advantage out from RWMutex then we should split it into a StringMap::find and then a StringMap::insert call what is doing 2 lookup.


http://reviews.llvm.org/D13652