[PATCH] D45478: [clangd] Merge symbols in global-sym-builder on the go

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:53
 
+/// Combines occurrences of the same symbols across translation units.
+class SymbolMerger {
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > Seems reasonably likely we would actually have contention here? merging per-thread (combiner) and then globally at the end (reducer) might be the way to go (might be significantly faster). But not sure how big the impact is.
> I haven't seen any noticeable performance degradation after this patch.
> There should be contention, but it doesn't seem to matter much, as parsing still takes way more time. 
> 
> Still makes sense to rewrite the code in order to avoid contention, will do that.
Looking at the traces, the threads spend ~6% of `mergeSymbols` time between `Lock(Mut)` and `for(const Sym...`.
Improving it might be a good idea, but from my point of view it's not too bad to commit it as is.

Doing it properly is a little complicated in the current setup, since we don't have knobs to create per-thread structs and merge them later.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45478