Remove lock from ConstString::GetLength

[scott.smith]

Summary

ConstStrings are immutable, so there is no need to grab even a reader lock in order to read the length field.

[labath]

Looks good, thank you.

Out of curiosity, have you observed any performance improvements resulting from this?

[scott.smith]

In D32306#733316, @labath wrote:

Looks good, thank you.

Out of curiosity, have you observed any performance improvements resulting from this?

10.2% reduction in # of instructions executed, 9.1% reduction in # of cycles, as measured by 'perf stat' in single threaded mode (I disabled TaskPool in order to get more repeatable results).

Can you commit this for me? I don't have commit access.

Thank you!

[zturner]

source/Utility/ConstString.cpp

49

Why do we even have this function which digs into the StringMap internals rather than just calling existing StringMap member functions? Can Can we just delete GetStringMapEntryFromKeyData entirely and use StringMap::find?

[labath]

source/Utility/ConstString.cpp

49

Can we just delete GetStringMapEntryFromKeyData entirely and use StringMap::find?

Unfortunately, I don't think that's possible. StringMap::find expects a StringRef. In order to construct that, we need to know the length of the string, and we're back where we started :(

In reality, this is doing a very different operation than find (which takes a random string and checks whether it's in the map) -- this takes a string which we know to be in the map and get its size.

It will take some rearchitecting of the ConstString class to get rid of this hack. Probably it could be fixed by ConstString storing a StringMap::iterator instead of the raw pointer. In any case, that seems out of scope of this change.

[labath]

In D32306#736115, @scott.smith wrote:

10.2% reduction in # of instructions executed, 9.1% reduction in # of cycles, as measured by 'perf stat' in single threaded mode (I disabled TaskPool in order to get more repeatable results).

That is awesome, thanks.

Can you commit this for me? I don't have commit access.

I will, let's just wait what Zachary says.

[scott.smith]

source/Utility/ConstString.cpp

49

Probably performance. If we have to call Find, then we have to call hash, fault in the appropriate bucket, and then finally return the entry that we already have in hand. Plus we'd need the lock.

[scott.smith]

Use StringMapEntry::GetStringMapEntryFromKeyData instead of ConstString's version.

[labath]

source/Utility/ConstString.cpp

49

Cool, I didn't notice that one when looking for it. I guess at this point we can just delete our copy of GetStringMapEntryFromKeyData completely and call the StringPool's version instead.

[scott.smith]

source/Utility/ConstString.cpp

49

It will push a lot of lines past the 80 char limit. Do you want me to make that change? If not, can you submit this one since I do not have commit access? Thanks!

[labath]

source/Utility/ConstString.cpp

49

Ok, nevermind, let's leave that as is for now. thanks for the patch.

source/Utility/ConstString.cpp

 public:
   typedef const char *StringPoolValueType;
   typedef llvm::StringMap<StringPoolValueType, llvm::BumpPtrAllocator>
       StringPool;
   typedef llvm::StringMapEntry<StringPoolValueType> StringPoolEntryType;
 
   static StringPoolEntryType &
   GetStringMapEntryFromKeyData(const char *keyData) {
-    char *ptr = const_cast<char *>(keyData) - sizeof(StringPoolEntryType);
-    return *reinterpret_cast<StringPoolEntryType *>(ptr);
+    return StringPoolEntryType::GetStringMapEntryFromKeyData(keyData);
   }
 
-  size_t GetConstCStringLength(const char *ccstr) const {
+  static size_t GetConstCStringLength(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);
+      // Since the entry is read only, and we derive the entry entirely from the
+      // pointer, we don't need the lock.
       const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
       return entry.getKey().size();
     }
     return 0;
   }
 
   StringPoolValueType GetMangledCounterpart(const char *ccstr) const {
     if (ccstr != nullptr) {
       const uint8_t h = hash(llvm::StringRef(ccstr));
       llvm::sys::SmartScopedReader<false> rlock(m_string_pools[h].m_mutex);

 
 ConstString::ConstString(const llvm::StringRef &s)
     : m_string(StringPool().GetConstCStringWithLength(s.data(), s.size())) {}
 
 bool ConstString::operator<(const ConstString &rhs) const {
   if (m_string == rhs.m_string)
     return false;
 
-  llvm::StringRef lhs_string_ref(m_string,
-                                 StringPool().GetConstCStringLength(m_string));
-  llvm::StringRef rhs_string_ref(
-      rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+  llvm::StringRef lhs_string_ref(GetStringRef());
+  llvm::StringRef rhs_string_ref(rhs.GetStringRef());
 
   // If both have valid C strings, then return the comparison
   if (lhs_string_ref.data() && rhs_string_ref.data())
     return lhs_string_ref < rhs_string_ref;
 
   // Else one of them was nullptr, so if LHS is nullptr then it is less than
   return lhs_string_ref.data() == nullptr;
 }
 
 Stream &lldb_private::operator<<(Stream &s, const ConstString &str) {
   const char *cstr = str.GetCString();
   if (cstr != nullptr)
     s << cstr;
 
   return s;
 }
 
 size_t ConstString::GetLength() const {
-  return StringPool().GetConstCStringLength(m_string);
+  return Pool::GetConstCStringLength(m_string);
 }
 
 bool ConstString::Equals(const ConstString &lhs, const ConstString &rhs,
                          const bool case_sensitive) {
   if (lhs.m_string == rhs.m_string)
     return true;
 
   // Since the pointers weren't equal, and identical ConstStrings always have
   // identical pointers,
   // the result must be false for case sensitive equality test.
   if (case_sensitive)
     return false;
 
   // perform case insensitive equality test
-  llvm::StringRef lhs_string_ref(
-      lhs.m_string, StringPool().GetConstCStringLength(lhs.m_string));
-  llvm::StringRef rhs_string_ref(
-      rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+  llvm::StringRef lhs_string_ref(lhs.GetStringRef());
+  llvm::StringRef rhs_string_ref(rhs.GetStringRef());
   return lhs_string_ref.equals_lower(rhs_string_ref);
 }
 
 int ConstString::Compare(const ConstString &lhs, const ConstString &rhs,
                          const bool case_sensitive) {
   // If the iterators are the same, this is the same string
   const char *lhs_cstr = lhs.m_string;
   const char *rhs_cstr = rhs.m_string;
   if (lhs_cstr == rhs_cstr)
     return 0;
   if (lhs_cstr && rhs_cstr) {
-    llvm::StringRef lhs_string_ref(
-        lhs_cstr, StringPool().GetConstCStringLength(lhs_cstr));
-    llvm::StringRef rhs_string_ref(
-        rhs_cstr, StringPool().GetConstCStringLength(rhs_cstr));
+    llvm::StringRef lhs_string_ref(lhs.GetStringRef());
+    llvm::StringRef rhs_string_ref(rhs.GetStringRef());
 
     if (case_sensitive) {
       return lhs_string_ref.compare(rhs_string_ref);
     } else {
       return lhs_string_ref.compare_lower(rhs_string_ref);
     }
   }