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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.
Herald added a subscriber: jkorous.

In https://reviews.llvm.org/D45478#1064027, @sammccall wrote:

> Is this patch still relevant after haojian's string deduplication?


Apparently it does. It has an advantage of distributing the work more evenly between the program runs.
Currently the tool reports progress based on the number of files it parsed. But then it takes a lot of time to actually merge all the symbols at the end and the progress is not reported during that time.

The perfect behavior would be to remove duplicates, not just intern them :-) That shouldn't be too hard and it probably even makes sense to have a ToolExecutor that does that.



================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:53
 
+/// Combines occurrences of the same symbols across translation units.
+class SymbolMerger {
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45478