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

[hokein]

Summary

This would save us some memory and disk space:

• Dex usage (261 MB vs 266 MB)
• Disk (75 MB vs 76 MB)

It would save more when we index the main file symbol D55185.

[nridge]

Might we want to keep some of this information for workspace/symbol? I mean, surely not "documentation", but perhaps "signature" and "return type"?

For example, when cquery responds to workspace/symbol, it puts into SymbolInformation.name a "detailed name" that includes the return type and signature for functions, allowing clients to display these in their "Open Element" dropdown. This is in turn useful for e.g. selecting the correct one among multiple overloads of a function to jump to.

[ilya-biryukov]

@hokein, do you need reviewers for this? I'm happy to volunteer.

In D56314#1347511, @nridge wrote:

Might we want to keep some of this information for workspace/symbol? I mean, surely not "documentation", but perhaps "signature" and "return type"?

There's nothing stopping us from reintroducing this information if we start doing the same. I don't foresee difficulties with this. It would be easier to figure out the bits we actually need when we implement this functionality.
But I generally prefer to move fast and fix things as you go, even if that means going back and forth, others might disagree.

[hokein]

In D56314#1347964, @ilya-biryukov wrote:

@hokein, do you need reviewers for this? I'm happy to volunteer.

Thanks.

In D56314#1347511, @nridge wrote:

Might we want to keep some of this information for workspace/symbol? I mean, surely not "documentation", but perhaps "signature" and "return type"?

There's nothing stopping us from reintroducing this information if we start doing the same. I don't foresee difficulties with this. It would be easier to figure out the bits we actually need when we implement this functionality.
But I generally prefer to move fast and fix things as you go, even if that means going back and forth, others might disagree.

+1, we don't use the signature and return type in the workspace/symbol at the moment. We could revisit it when we actually need them.

[ilya-biryukov]

clangd/index/Index.h

232 ↗

(On Diff #180233)

This comment would be most useful beside the mentioned fields themselves. Maybe add it there too? Possibly with a shorter form, since there's no need to mention the field names there.

clangd/index/SymbolCollector.cpp

543 ↗

(On Diff #180233)

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)

[hokein]

clangd/index/SymbolCollector.cpp

543 ↗

(On Diff #180233)

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?

[ilya-biryukov]

clangd/index/SymbolCollector.cpp

543 ↗

(On Diff #180233)

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.

[hokein]

Address comments, store docs.

[hokein]

clangd/index/SymbolCollector.cpp

543 ↗

(On Diff #180233)

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.

[ilya-biryukov]

LGTM

clangd/index/Index.h

188 ↗

(On Diff #180808)

NIT: Maybe change to only set when the symbol...? "Meaningful" might create confusion.

clangd/index/SymbolCollector.cpp

538 ↗

(On Diff #180808)

NIT: consider inlining the lamda, the code inside it is simple enough.
Up to you, though.

543 ↗

(On Diff #180233)

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.)

[hokein]

address review comments

[ilya-biryukov]

LGTM again :-) I bet the savings are less now that we're always storing the comments in the static index, so the numbers in the description might be outdated.

[hokein]

In D56314#1351091, @ilya-biryukov wrote:

LGTM again :-) I bet the savings are less now that we're always storing the comments in the static index, so the numbers in the description might be outdated.

Yes, fixed.

clang-tools-extra/trunk/clangd/index/Index.h

struct Symbol {
   SymbolLocation CanonicalDeclaration;
   // The number of translation units that reference this symbol from their main
   // file. This number is only meaningful if aggregated in an index.
   unsigned References = 0;
   /// Where this symbol came from. Usually an index provides a constant value.
   SymbolOrigin Origin = SymbolOrigin::Unknown;
   /// A brief description of the symbol that can be appended in the completion
   /// candidate list. For example, "(X x, Y y) const" is a function signature.
+  /// Only set when the symbol is indexed for completion.
   llvm::StringRef Signature;
   /// What to insert when completing this symbol, after the symbol name.
   /// This is in LSP snippet syntax (e.g. "({$0})" for a no-args function).
   /// (When snippets are disabled, the symbol name alone is used).
+  /// Only set when the symbol is indexed for completion.
   llvm::StringRef CompletionSnippetSuffix;
   /// Documentation including comment for the symbol declaration.
   llvm::StringRef Documentation;
   /// Type when this symbol is used in an expression. (Short display form).
   /// e.g. return type of a function, or type of a variable.
+  /// Only set when the symbol is indexed for completion.
   llvm::StringRef ReturnType;
 
   /// Raw representation of the OpaqueType of the symbol, used for scoring
   /// purposes.
+  /// Only set when the symbol is indexed for completion.
   llvm::StringRef Type;
 
   struct IncludeHeaderWithReferences {
     IncludeHeaderWithReferences() = default;
 
     IncludeHeaderWithReferences(llvm::StringRef IncludeHeader,
                                 unsigned References)
         : IncludeHeader(IncludeHeader), References(References) {}
struct IncludeHeaderWithReferences {
     /// The number of translation units that reference this symbol and include
     /// this header. This number is only meaningful if aggregated in an index.
     unsigned References = 0;
   };
   /// One Symbol can potentially be incuded via different headers.
   ///   - If we haven't seen a definition, this covers all declarations.
   ///   - If we have seen a definition, this covers declarations visible from
   ///   any definition.
+  /// Only set when the symbol is indexed for completion.
   llvm::SmallVector<IncludeHeaderWithReferences, 1> IncludeHeaders;
 
   enum SymbolFlag : uint8_t {
     None = 0,
     /// Whether or not this symbol is meant to be used for the code completion.
     /// See also isIndexedForCodeCompletion().
+    /// Note that we don't store completion information (signature, snippet,
+    /// type, inclues) if the symbol is not indexed for code completion.
     IndexedForCodeCompletion = 1 << 0,
     /// Indicates if the symbol is deprecated.
     Deprecated = 1 << 1,
     // Symbol is an implementation detail.
     ImplementationDetail = 1 << 2,
   };
 
   SymbolFlag Flags = SymbolFlag::None;

clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp

const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
   std::string FileURI;
   auto Loc = findNameLoc(&ND);
   // FIXME: use the result to filter out symbols.
   shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
   if (auto DeclLoc =
           getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
+  S.Origin = Opts.Origin;
+  if (ND.getAvailability() == AR_Deprecated)
+    S.Flags |= Symbol::Deprecated;
+
   // Add completion info.
   // FIXME: we may want to choose a different redecl, or combine from several.
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
   // We use the primary template, as clang does during code completion.
   CodeCompletionResult SymbolCompletion(&getTemplateOrThis(ND), 0);
   const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
       *ASTCtx, *PP, CodeCompletionContext::CCC_Symbol, *CompletionAllocator,
       *CompletionTUInfo,
       /*IncludeBriefComments*/ false);
-  std::string Signature;
-  std::string SnippetSuffix;
-  getSignature(*CCS, &Signature, &SnippetSuffix);
   std::string Documentation =
       formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion,
                                               /*CommentsFromHeaders=*/true));
+  // For symbols not indexed for completion (class members), we also store their
+  // docs in the index, because Sema doesn't load the docs from the preamble, we
+  // rely on the index to get the docs.
+  // FIXME: this can be optimized by only storing the docs in dynamic index --
+  // dynamic index should index these symbols when Sema completes a member
+  // completion.
+  S.Documentation = Documentation;
+  if (!(S.Flags & Symbol::IndexedForCodeCompletion)) {
+    Symbols.insert(S);
+    return Symbols.find(S.ID);
+  }
+
+  std::string Signature;
+  std::string SnippetSuffix;
+  getSignature(*CCS, &Signature, &SnippetSuffix);
+  S.Signature = Signature;
+  S.CompletionSnippetSuffix = SnippetSuffix;
   std::string ReturnType = getReturnType(*CCS);
+  S.ReturnType = ReturnType;
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
     // Use the expansion location to get the #include header since this is
     // where the symbol is exposed.
     if (auto Header = getIncludeHeader(
             QName, SM, SM.getExpansionLoc(ND.getLocation()), Opts))
       Include = std::move(*Header);
   }
-  S.Signature = Signature;
-  S.CompletionSnippetSuffix = SnippetSuffix;
-  S.Documentation = Documentation;
-  S.ReturnType = ReturnType;
   if (!Include.empty())
     S.IncludeHeaders.emplace_back(Include, 1);
 
   llvm::Optional<OpaqueType> TypeStorage;
   if (S.Flags & Symbol::IndexedForCodeCompletion) {
     TypeStorage = OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion);
     if (TypeStorage)
       S.Type = TypeStorage->raw();
   }
 
-  S.Origin = Opts.Origin;
-  if (ND.getAvailability() == AR_Deprecated)
-    S.Flags |= Symbol::Deprecated;
   Symbols.insert(S);
   return Symbols.find(S.ID);
 }
 
 void SymbolCollector::addDefinition(const NamedDecl &ND,
                                     const Symbol &DeclSym) {
   if (DeclSym.Definition)
     return;

clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

class Foo {
       static int x;
     };
   )";
   const std::string Main = R"(
     void Foo::g() {}
     void Foo::ssf() {}
   )";
   runSymbolCollector(Header, Main);
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("Foo"), QName("Foo::f"),
-                                   QName("Foo::g"), QName("Foo::sf"),
-                                   QName("Foo::ssf"), QName("Foo::x")));
+  EXPECT_THAT(
+      Symbols,
+      UnorderedElementsAre(
+          QName("Foo"),
+          AllOf(QName("Foo::f"), ReturnType(""), ForCodeCompletion(false)),
+          AllOf(QName("Foo::g"), ReturnType(""), ForCodeCompletion(false)),
+          AllOf(QName("Foo::sf"), ReturnType(""), ForCodeCompletion(false)),
+          AllOf(QName("Foo::ssf"), ReturnType(""), ForCodeCompletion(false)),
+          AllOf(QName("Foo::x"), ReturnType(""), ForCodeCompletion(false))));
 }
 
 TEST_F(SymbolCollectorTest, Scopes) {
   const std::string Header = R"(
     namespace na {
     class Foo {};
     namespace nb {
     class Bar {};