[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/index/SymbolCollector.cpp:543
+
+  if (!(S.Flags & Symbol::IndexedForCodeCompletion))
+    return Insert(S);
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Most of the fields updated at the bottom aren't useful. However, I feel the documentation is actually important, since Sema only has doc comments for the **current** file and the rest are currently expected to be provided by the index.
> > > 
> > > I'm not sure if we already have the code to query the doc comments via index for member completions. If not, it's an oversight.
> > > In any case, I suggest we **always** store the comments in **dynamic** index. Not storing the comments in the static index is fine, since any data for member completions should be provided by the dynamic index (we see a member in completion ⇒ sema has processed the headers ⇒ the dynamic index should know about those members)
> > This is a good point.
> > 
> > For class member completions, we rely solely on Sema completions (no query being queried). I'm not sure it is practical to query the index for member completions.
> > - this means for **every** code completion, we query the index, it may slow down completions
> > - `fuzzyFind` is not supported for class members in our internal index service (due to the large number of them)
> > 
> > So it turns two possibilities:
> > 
> > 1) always store comments (`SymbolCollector` doesn't know whether it is used in static index or dynamic index)
> > 2) or drop them for now, this wouldn't break anything, and add it back when we actually use them for class completions
> > 
> > I slightly prefer 2) at the moment. WDYT?
> Yeah, instead of using `fuzzyFind()`, we'll call  `lookup()` to get details of the symbols we've discovered.
> It's true that this is going to add some latency, but I hope we have the latency budget to handle this (these queries should be fast, e.g. we're doing this for signature help and I haven't seen any noticeable latency there from the index query, most of the running time is parsing C++).
> 
> I also like option 2, but unfortunately we already rely on this to get the comments in signature help, so this change would actually introduce regressions there (less used than code completion, but still not nice to break it)
> Would the third option of having a config option (e.g. `SymbolCollector::Options::StoreAllComments`) work?  `clangd-indexer` and auto-indexer would set the option to false, `indexSymbols` would set the option to true. We'll both get the optimizations and avoid introducing any regressions. The plumbing should be no more than a few lines of code.
Sounds fair. I totally missed `signature help`, it is unfortunate that our test case doesn't find this regression (added one!)

> Would the third option of having a config option (e.g. SymbolCollector::Options::StoreAllComments) work?  clangd-indexer and auto-indexer would set the option to false, indexSymbols would set the option to true. We'll both get the optimizations and avoid introducing any regressions. The plumbing should be no more than a few lines of code.

This is a reasonable solution, but I prefer to do it in a separated patch. Now I make this patch always store docs, which should not introduce any regressions. There is another optimization opportunity here -- unlike header symbols, docs from main-file symbols can be dropped from the index, we can retrieve them from Sema.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56314/new/

https://reviews.llvm.org/D56314