[Lldb-commits] [PATCH] D32306: Remove lock from ConstString::GetLength

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Utility/ConstString.cpp:49
+      // pointer, we don't need the lock.
       const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
       return entry.getKey().size();
----------------
scott.smith wrote:
> labath wrote:
> > labath wrote:
> > > scott.smith wrote:
> > > > zturner wrote:
> > > > > 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`?
> > > > 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.
> > > > 
> > > > 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.
> > 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.
> 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!
Ok, nevermind, let's leave that as is for now. thanks for the patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D32306