[Lldb-commits] [PATCH] D32832: Make ConstString creation and access lockfree
Scott Smith via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Wed May 3 15:34:21 PDT 2017
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.
More information about the lldb-commits