[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D52078#1234667, @ilya-biryukov wrote:

> ~1% increase in memory usage seems totally fine. Actually surprised it's that small.


Tested on a larger file: it's ~5% for `ClangdServer.cpp`. I think it's still worth it for the speedup.



================
Comment at: clangd/index/FileIndex.cpp:84
+    Merged.insert(Macro);
+  for (const auto &Sym : Collector.takeSymbols())
+    Merged.insert(Sym);
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > Why make an extra copy of the symbols? Why not add macros to the same builder used in collector?
> > `index::indexTopLevelDecls` will re-`initialize` the collector. This is safe with the current implementation, but I can imagine it goes wrong when we do more cleanup in the initialization.
> Sorry, missed the comment about it.
> And if we do this after `indexTopLevelDecls`, than we'll be calling after the `finish` call. **Sigh**...
> 
> Doing an extra copy of the symbols here is wasteful and makes the code more complicated than it should be.
> 
> We have alternatives that could fix this:
> 1. Move `IndexMacro` logic into `index::indexTopLevelDecls`
> 2. Move macro collection to `SymbolCollector::finish()`
> 3. Extract a function that creates a symbol for macro definition, so we don't need to call `handleMacroOccurence` and add can add extra macro symbols after `SymbolCollector` finishes.
> 
> 
> (1) seems to be the cleanest option, WDYT?
(1) sounds good. D52098


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078