[Lldb-commits] [PATCH] D32820: Parallelize demangling

[Scott Smith via Phabricator via lldb-commits]

scott.smith marked an inline comment as done.
scott.smith added a comment.

In https://reviews.llvm.org/D32820#747309, @clayborg wrote:

> What are the measured improvements here? We can't slow things down on any platforms. I know MacOS didn't respond well to making demangling run in parallel. I want to see some numbers here. And please don't quote numbers with tcmalloc or any special allocator unless you have a patch in  LLDB already to make this a build option.


Without tcmalloc, on Ubuntu 14.04, 40 core VM:  13%
With tcmalloc, on Ubuntu 14.04, 40 core VM: 24%  (built using cmake ... -DCMAKE_EXE_LINKER_FLAGS=-ltcmalloc_minimal, which amazingly only works when building with clang, not gcc...)

I don't have access to a Mac, and of course YMMV depending on the application.

so yeah, it's a bigger improvement with tcmalloc.  Interestingly, I tried adding back the demangler improvements I had queued up (which reduced memory allocations), and they didn't matter much, which makes me think this is contention allocating const strings.  I could be wrong though.

By far the biggest performance improvement I have queued up is loading the shared libraries in parallel (https://reviews.llvm.org/D32597), but that's waiting on pulling parallel_for_each from LLD into LLVM (I think).

If you're really leery of this change, I could just make the structural changes to allow parallelization, and then keep a small patch internally to enable it.  Or enabling it could be platform dependent.  Or ... ?



================
Comment at: source/Symbol/Symtab.cpp:233-239
+  // Don't let trampolines get into the lookup by name map
+  // If we ever need the trampoline symbols to be searchable by name
+  // we can remove this and then possibly add a new bool to any of the
+  // Symtab functions that lookup symbols by name to indicate if they
+  // want trampolines.
+  if (symbol.IsTrampoline())
+    return;
----------------
clayborg wrote:
> Not being able to search for symbols by name when they are trampolines? If you lookup symbols by name I would expect things not to fail and I would expect that I would get all the answers, not just ones that are omitted for performance reasons. I would also not expect to have to specify extra flags. Please remove
This is just moved code, not new code.  You can use the phabricator history tool above to diff baseline against #97908, and see that the only change I made was continue->return, due to changing it from a loop to a lambda (now a separate function).

This is why I pubished D32708 separately - I wanted to separate the functional change from the structural change.


Repository:
  rL LLVM

https://reviews.llvm.org/D32820