[Lldb-commits] [PATCH] D32832: Make ConstString creation and access lockfree

[Scott Smith via Phabricator via lldb-commits]

scott.smith added a comment.

In https://reviews.llvm.org/D32832#745361, @clayborg wrote:

> Do you have performance numbers here? I am all for it if it improves performance. If this is faster than llvm::StringMap why not just fix it there? Seems like everyone could benefit if you fix it in LLVM?


A proper lockfree hashtable that allows deletions is complicated and slow for simple cases, IME.

ConstString is a special case, because

1. It's big
2. It's insert-only
3. It's highly concurrent

I think a proper lock free SkipList would be a better fit for LLVM though, and this code could then build upon it, which would solve it's existing scalability issue.

Truth is this isn't a huge deal (ignoring the HashString->xxHash change and assuming tcmalloc, it's saving 1+ seconds, out of 4.5-5ish).  It might lead itself to a custom allocator, though, which may make linking with tcmalloc unnecessary (something like BumpPtrAllocator, but threadsafe, or maybe just thread local).  Again, a trick you can get away with since this is insert-only.

So to break it down:

1. the HashString->xxHash change could be made separately if there was a version of StringMap that took the hash as a parameter, rather than running the hash function itself.  That would reduce the # of calls from 2 to 1, and would allow the use of a function that is better suited to lldb (I ran into resistance changing llvm::HashString since clang depends on it, and I couldn't prove it wouldn't hurt.
2. 64-bit hashes for fewer collisions - StringMap uses 32-bit hashes, which makes sense for smaller hash tables.  This may or may not matter much.
3. no lock for reading or updating value - GetMangledCounterpart, SetMangledCounterparts, and half of GetConstCStringAndSetMangledCountrpart no longer compute a hash, and don't need a lock.  You can actually do this with StringMap by using a struct as the value side that has std::atomic in it
4. much finer granularity - I tried increasing the # of buckets in the old ConstString but could never improve performance (though that was without tcmalloc, maybe it'd work now)
5. lockfree insertion

and possibly in the future:

6. BumpPtrAllocator for struct Entry - maybe eliminate the need for tcmalloc (well, assuming the demangler used an arena allocator, something else I gave up rather than trying to convince libcxxabi to change, then having to port that back to llvm).

I haven't measured how much each of these changes matter.  This change is small enough and in a file that changes infrequently, so we can keep this as a private patch if we have to.


Repository:
  rL LLVM

https://reviews.llvm.org/D32832