[clangd] Remove accessors for top-level decls from preamble

[ilya-biryukov]

Summary

They cause lots of deserialization and are not actually used.
The ParsedAST accessor that previously returned those was renamed from
getTopLevelDecls to getLocalTopLevelDecls in order to avoid
confusion.

This change should considerably improve the speed of findDefinition
and other features that try to find AST nodes, corresponding to the
locations in the source code.

[sammccall]

Wow, nice!
Just unsure about the test helper. (We can rewrite it in another way if needed)

clangd/ClangdUnit.h

91–93

nit: remove comma

unittests/clangd/TestTU.cpp

74–75

isn't this incorrect? Or should be "findDeclInMainFile" or similar?

I'd think this would conflict with your other patch, which uses this to test the boost of things that come from the header.

[ioeric]

Nice! This looks good to me. Just some nits. I'll let Sam stamp.

clangd/ClangdUnit.h

84

IIUC, ASTContext in a ParsedAST may not contain information from preambles and thus may give an incomplete AST. If so, I think we should make this more explicit in the class level to make sure this is not misused (e.g. in case the incomplete context is used to build dynamic index).

121–124

nit: also rename this and add comment?

[ilya-biryukov]

• Rename the field in addition to the getter
• Address review comments

[ilya-biryukov]

clangd/ClangdUnit.h

84

Added a small doc comment about it. Let me know if you feel we need to elaborate more on this.
I don't think anything changed wrt to this behavior in this patch, apart from the fact that we used to deserialize more stuff most of the time before so this wan't as visible.

unittests/clangd/TestTU.cpp

74–75

Thanks for spotting that, I would also expect this to fail in the new patch.
I'll look into rewriting this helper.

[ilya-biryukov]

• Rewrote findDecl, it will deserialize decls if needed now

clangd/ClangdUnit.h

 namespace tooling {
 struct CompileCommand;
 }
 
 namespace clangd {
 
 // Stores Preamble and associated data.
 struct PreambleData {
-  PreambleData(PrecompiledPreamble Preamble,
-               std::vector<serialization::DeclID> TopLevelDeclIDs,
-               std::vector<Diag> Diags, std::vector<Inclusion> Inclusions);
+  PreambleData(PrecompiledPreamble Preamble, std::vector<Diag> Diags,
+               std::vector<Inclusion> Inclusions);
 
   PrecompiledPreamble Preamble;
-  std::vector<serialization::DeclID> TopLevelDeclIDs;
   std::vector<Diag> Diags;
   // Processes like code completions and go-to-definitions will need #include
   // information, and their compile action skips preamble range.
   std::vector<Inclusion> Inclusions;
 };
 
 /// Information required to run clang, e.g. to parse AST or do code completion.
 struct ParseInputs {
Build(std::unique_ptr<clang::CompilerInvocation> CI,
         std::shared_ptr<PCHContainerOperations> PCHs,
         IntrusiveRefCntPtr<vfs::FileSystem> VFS);
 
   ParsedAST(ParsedAST &&Other);
   ParsedAST &operator=(ParsedAST &&Other);
 
   ~ParsedAST();
 
+  /// Note that the returned ast will not contain decls from the preamble that
+  /// were not deserialized during parsing. Clients should expect only decls
+  /// from the main file to be in the AST.
   ASTContext &getASTContext();
   const ASTContext &getASTContext() const;
 
   Preprocessor &getPreprocessor();
   std::shared_ptr<Preprocessor> getPreprocessorPtr();
   const Preprocessor &getPreprocessor() const;
 
-  /// This function returns all top-level decls, including those that come
-  /// from Preamble. Decls, coming from Preamble, have to be deserialized, so
-  /// this call might be expensive.
-  ArrayRef<const Decl *> getTopLevelDecls();
+  /// This function returns top-level decls present in the main file of the AST.
+  /// The result does not include the decls that come from the preamble.
+  ArrayRef<const Decl *> getLocalTopLevelDecls();
 
   const std::vector<Diag> &getDiagnostics() const;
 
   /// Returns the esitmated size of the AST and the accessory structures, in
   /// bytes. Does not include the size of the preamble.
   std::size_t getUsedBytes() const;
   const std::vector<Inclusion> &getInclusions() const;
 
 private:
   ParsedAST(std::shared_ptr<const PreambleData> Preamble,
             std::unique_ptr<CompilerInstance> Clang,
             std::unique_ptr<FrontendAction> Action,
-            std::vector<const Decl *> TopLevelDecls, std::vector<Diag> Diags,
-            std::vector<Inclusion> Inclusions);
-
-private:
-  void ensurePreambleDeclsDeserialized();
+            std::vector<const Decl *> LocalTopLevelDecls,
+            std::vector<Diag> Diags, std::vector<Inclusion> Inclusions);
 
   // In-memory preambles must outlive the AST, it is important that this member
   // goes before Clang and Action.
   std::shared_ptr<const PreambleData> Preamble;
   // We store an "incomplete" FrontendAction (i.e. no EndSourceFile was called
   // on it) and CompilerInstance used to run it. That way we don't have to do
   // complex memory management of all Clang structures on our own. (They are
   // stored in CompilerInstance and cleaned up by
   // FrontendAction.EndSourceFile).
   std::unique_ptr<CompilerInstance> Clang;
   std::unique_ptr<FrontendAction> Action;
 
   // Data, stored after parsing.
   std::vector<Diag> Diags;
-  std::vector<const Decl *> TopLevelDecls;
-  bool PreambleDeclsDeserialized;
+  // Top-level decls inside the current file. Not that this does not include
+  // top-level decls from the preamble.
+  std::vector<const Decl *> LocalTopLevelDecls;
   std::vector<Inclusion> Inclusions;
 };
 
 using PreambleParsedCallback = std::function<void(
     PathRef Path, ASTContext &, std::shared_ptr<clang::Preprocessor>)>;
 
 /// Manages resources, required by clangd. Allows to rebuild file with new
 /// contents, and provides AST and Preamble for it.

clangd/ClangdUnit.cpp

private:
   std::vector<const Decl *> TopLevelDecls;
 };
 
 class CppFilePreambleCallbacks : public PreambleCallbacks {
 public:
   CppFilePreambleCallbacks(PathRef File, PreambleParsedCallback ParsedCallback)
       : File(File), ParsedCallback(ParsedCallback) {}
 
-  std::vector<serialization::DeclID> takeTopLevelDeclIDs() {
-    return std::move(TopLevelDeclIDs);
-  }
-
   std::vector<Inclusion> takeInclusions() { return std::move(Inclusions); }
 
-  void AfterPCHEmitted(ASTWriter &Writer) override {
-    TopLevelDeclIDs.reserve(TopLevelDecls.size());
-    for (Decl *D : TopLevelDecls) {
-      // Invalid top-level decls may not have been serialized.
-      if (D->isInvalidDecl())
-        continue;
-      TopLevelDeclIDs.push_back(Writer.getDeclID(D));
-    }
-  }
-
   void AfterExecute(CompilerInstance &CI) override {
     if (!ParsedCallback)
       return;
     trace::Span Tracer("Running PreambleCallback");
     ParsedCallback(File, CI.getASTContext(), CI.getPreprocessorPtr());
   }
 
-  void HandleTopLevelDecl(DeclGroupRef DG) override {
-    for (Decl *D : DG) {
-      if (isa<ObjCMethodDecl>(D))
-        continue;
-      TopLevelDecls.push_back(D);
-    }
-  }
-
   void BeforeExecute(CompilerInstance &CI) override {
     SourceMgr = &CI.getSourceManager();
   }
 
   std::unique_ptr<PPCallbacks> createPPCallbacks() override {
     assert(SourceMgr && "SourceMgr must be set at this point");
     return collectInclusionsInMainFileCallback(
         *SourceMgr,
         [this](Inclusion Inc) { Inclusions.push_back(std::move(Inc)); });
   }
 
 private:
   PathRef File;
   PreambleParsedCallback ParsedCallback;
-  std::vector<Decl *> TopLevelDecls;
-  std::vector<serialization::DeclID> TopLevelDeclIDs;
   std::vector<Inclusion> Inclusions;
   SourceManager *SourceMgr = nullptr;
 };
 
 } // namespace
 
 void clangd::dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
   AST.getASTContext().getTranslationUnitDecl()->dump(OS, true);
ParsedAST::Build(std::unique_ptr<clang::CompilerInvocation> CI,
   ASTDiags.EndSourceFile();
 
   std::vector<const Decl *> ParsedDecls = Action->takeTopLevelDecls();
   return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
                    std::move(ParsedDecls), ASTDiags.take(),
                    std::move(Inclusions));
 }
 
-void ParsedAST::ensurePreambleDeclsDeserialized() {
-  if (PreambleDeclsDeserialized || !Preamble)
-    return;
-
-  std::vector<const Decl *> Resolved;
-  Resolved.reserve(Preamble->TopLevelDeclIDs.size());
-
-  ExternalASTSource &Source = *getASTContext().getExternalSource();
-  for (serialization::DeclID TopLevelDecl : Preamble->TopLevelDeclIDs) {
-    // Resolve the declaration ID to an actual declaration, possibly
-    // deserializing the declaration in the process.
-    if (Decl *D = Source.GetExternalDecl(TopLevelDecl))
-      Resolved.push_back(D);
-  }
-
-  TopLevelDecls.reserve(TopLevelDecls.size() +
-                        Preamble->TopLevelDeclIDs.size());
-  TopLevelDecls.insert(TopLevelDecls.begin(), Resolved.begin(), Resolved.end());
-
-  PreambleDeclsDeserialized = true;
-}
-
 ParsedAST::ParsedAST(ParsedAST &&Other) = default;
 
 ParsedAST &ParsedAST::operator=(ParsedAST &&Other) = default;
 
 ParsedAST::~ParsedAST() {
   if (Action) {
     Action->EndSourceFile();
   }

 std::shared_ptr<Preprocessor> ParsedAST::getPreprocessorPtr() {
   return Clang->getPreprocessorPtr();
 }
 
 const Preprocessor &ParsedAST::getPreprocessor() const {
   return Clang->getPreprocessor();
 }
 
-ArrayRef<const Decl *> ParsedAST::getTopLevelDecls() {
-  ensurePreambleDeclsDeserialized();
-  return TopLevelDecls;
+ArrayRef<const Decl *> ParsedAST::getLocalTopLevelDecls() {
+  return LocalTopLevelDecls;
 }
 
 const std::vector<Diag> &ParsedAST::getDiagnostics() const { return Diags; }
 
 std::size_t ParsedAST::getUsedBytes() const {
   auto &AST = getASTContext();
   // FIXME(ibiryukov): we do not account for the dynamically allocated part of
   // Message and Fixes inside each diagnostic.
   return AST.getASTAllocatedMemory() + AST.getSideTableAllocatedMemory() +
-         ::getUsedBytes(TopLevelDecls) + ::getUsedBytes(Diags);
+         ::getUsedBytes(LocalTopLevelDecls) + ::getUsedBytes(Diags);
 }
 
 const std::vector<Inclusion> &ParsedAST::getInclusions() const {
   return Inclusions;
 }
 
 PreambleData::PreambleData(PrecompiledPreamble Preamble,
-                           std::vector<serialization::DeclID> TopLevelDeclIDs,
                            std::vector<Diag> Diags,
                            std::vector<Inclusion> Inclusions)
-    : Preamble(std::move(Preamble)),
-      TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)),
+    : Preamble(std::move(Preamble)), Diags(std::move(Diags)),
       Inclusions(std::move(Inclusions)) {}
 
 ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
                      std::unique_ptr<CompilerInstance> Clang,
                      std::unique_ptr<FrontendAction> Action,
-                     std::vector<const Decl *> TopLevelDecls,
+                     std::vector<const Decl *> LocalTopLevelDecls,
                      std::vector<Diag> Diags, std::vector<Inclusion> Inclusions)
     : Preamble(std::move(Preamble)), Clang(std::move(Clang)),
       Action(std::move(Action)), Diags(std::move(Diags)),
-      TopLevelDecls(std::move(TopLevelDecls)), PreambleDeclsDeserialized(false),
+      LocalTopLevelDecls(std::move(LocalTopLevelDecls)),
       Inclusions(std::move(Inclusions)) {
   assert(this->Clang);
   assert(this->Action);
 }
 
 CppFile::CppFile(PathRef FileName, bool StorePreamblesInMemory,
                  std::shared_ptr<PCHContainerOperations> PCHs,
                  PreambleParsedCallback PreambleCallback)
CppFile::rebuildPreamble(CompilerInvocation &CI,
   // bodies.
   CI.getFrontendOpts().SkipFunctionBodies = false;
 
   if (BuiltPreamble) {
     log("Built preamble of size " + Twine(BuiltPreamble->getSize()) +
         " for file " + Twine(FileName));
 
     return std::make_shared<PreambleData>(
-        std::move(*BuiltPreamble),
-        SerializedDeclsCollector.takeTopLevelDeclIDs(),
-        PreambleDiagnostics.take(), SerializedDeclsCollector.takeInclusions());
+        std::move(*BuiltPreamble), PreambleDiagnostics.take(),
+        SerializedDeclsCollector.takeInclusions());
   } else {
     log("Could not build a preamble for file " + Twine(FileName));
     return nullptr;
   }
 }
 
 SourceLocation clangd::getBeginningOfIdentifier(ParsedAST &Unit,
                                                 const Position &Pos,

clangd/XRefs.cpp

 
 IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
   auto DeclMacrosFinder = DeclarationAndMacrosFinder(
       llvm::errs(), Pos, AST.getASTContext(), AST.getPreprocessor());
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = true;
-  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
+  indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(),
                      DeclMacrosFinder, IndexOpts);
 
   return {DeclMacrosFinder.takeDecls(), DeclMacrosFinder.takeMacroInfos()};
 }
 
 llvm::Optional<std::string>
 getAbsoluteFilePath(const FileEntry *F, const SourceManager &SourceMgr) {
   SmallString<64> FilePath = F->tryGetRealPathName();
std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
 
   DocumentHighlightsFinder DocHighlightsFinder(
       llvm::errs(), AST.getASTContext(), AST.getPreprocessor(), SelectedDecls);
 
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = true;
-  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
+  indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(),
                      DocHighlightsFinder, IndexOpts);
 
   return DocHighlightsFinder.takeHighlights();
 }
 
 static PrintingPolicy PrintingPolicyForDecls(PrintingPolicy Base) {
   PrintingPolicy Policy(Base);
 

unittests/clangd/TestTU.cpp

-//===--- TestTU.cpp - Scratch source files for testing ------------*-
-//C++-*-===//
+//===--- TestTU.cpp - Scratch source files for testing --------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 #include "TestTU.h"
const Symbol &findSymbol(const SymbolSlab &Slab, llvm::StringRef QName) {
   if (!Result) {
     ADD_FAILURE() << "No symbol named " << QName << " in "
                   << ::testing::PrintToString(Slab);
     assert(false && "No symbol with QName");
   }
   return *Result;
 }
 
 const NamedDecl &findDecl(ParsedAST &AST, llvm::StringRef QName) {
-  const NamedDecl *Result = nullptr;
-  for (const Decl *D : AST.getTopLevelDecls()) {
-    auto *ND = dyn_cast<NamedDecl>(D);
-    if (!ND || ND->getNameAsString() != QName)
-      continue;
-    if (Result) {
-      ADD_FAILURE() << "Multiple Decls named " << QName;
-      assert(false && "QName is not unique");
-    }
-    Result = ND;
-  }
-  if (!Result) {
-    ADD_FAILURE() << "No Decl named " << QName << " in AST";
-    assert(false && "No Decl with QName");
+  llvm::SmallVector<llvm::StringRef, 4> Components;
+  QName.split(Components, "::");
+
+  auto &Ctx = AST.getASTContext();
+  auto LookupDecl = [&Ctx](const DeclContext &Scope,
+                           llvm::StringRef Name) -> const NamedDecl & {
+    auto LookupRes = Scope.lookup(DeclarationName(&Ctx.Idents.get(Name)));
+    assert(!LookupRes.empty() && "Lookup failed");
+    assert(LookupRes.size() == 1 && "Lookup returned multiple results");
+    return *LookupRes.front();
+  };
+
+  const DeclContext *Scope = Ctx.getTranslationUnitDecl();
+  for (auto NameIt = Components.begin(), End = Components.end() - 1;
+       NameIt != End; ++NameIt) {
+    Scope = &cast<DeclContext>(LookupDecl(*Scope, *NameIt));
   }
-  return *Result;
+  return LookupDecl(*Scope, Components.back());
 }
 
 } // namespace clangd
 } // namespace clang