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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: clangd/index/Index.h:188
   /// candidate list. For example, "(X x, Y y) const" is a function signature.
+  /// This field is only meaningful when the symbol is indexed for completion.
   llvm::StringRef Signature;
----------------
NIT: Maybe change to `only set when the symbol...`?  "Meaningful" might create confusion.


================
Comment at: clangd/index/SymbolCollector.cpp:543
+
+  if (!(S.Flags & Symbol::IndexedForCodeCompletion))
+    return Insert(S);
----------------
hokein wrote:
> 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.
Totally, the main file symbol docs are not necessary. OTOH, the savings for the main files should be almost negligible, since there are not that many main files open at a time and comments is just a small fraction of the information we store for each file (preabmles, source code contents, etc.)


================
Comment at: clangd/index/SymbolCollector.cpp:538
+
+  auto Insert = [this](const Symbol& S) {
+    Symbols.insert(S);
----------------
NIT: consider inlining the lamda, the code inside it is simple enough.
Up to you, though.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56314