[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

Tue May 23 12:09:02 PDT 2023

kuganv added inline comments.

================
Comment at: clang-tools-extra/clangd/Preamble.cpp:106-113
+    CI.setSema(nullptr);
+    CI.setASTConsumer(nullptr);
+    if (CI.getASTReader()) {
+      CI.getASTReader()->setDeserializationListener(nullptr);
+      /* This just sets consumer to null when DeserializationListener is null */
+      CI.getASTReader()->StartTranslationUnit(nullptr);
     }
----------------
kadircet wrote:
> kuganv wrote:
> > kadircet wrote:
> > > kuganv wrote:
> > > > kadircet wrote:
> > > > > why are we triggering destructors for all of these objects eagerly? if this is deliberate to "fix" some issue, could you mention that in comments?
> > > > > why are we triggering destructors for all of these objects eagerly? if this is deliberate to "fix" some issue, could you mention that in comments?
> > > > 
> > > > Thanks a lot for the review.
> > > > If we don't destruct and set it to null, CapturedASTCtx will also have to keep instances such as ASTConsumer including other related callbacks such PreambleCallbacks. This was making the CapturedASTCtx interface and implementation complex.
> > > > If we don't destruct and set it to null, CapturedASTCtx will also have to keep instances such as ASTConsumer 
> > > 
> > > Sorry if I am being dense but I can't actually see any references to those objects in the structs we preseve. AFAICT, Sema, ASTConsumer and ASTReader are members of CompilerInstance, which we don't keep around. Can you point me towards any references to the rest of the objects you're setting to null in case I am missing any?
> > > 
> > > `ASTMutationListener` seems to be the only one that's relevant. It's indeed exposed through `ASTContext` and our indexing operations can trigger various callbacks to fire. Can you set only that one to nullptr and leave a comment explaining the situation?
> > > > If we don't destruct and set it to null, CapturedASTCtx will also have to keep instances such as ASTConsumer 
> > > 
> > > Sorry if I am being dense but I can't actually see any references to those objects in the structs we preseve. AFAICT, Sema, ASTConsumer and ASTReader are members of CompilerInstance, which we don't keep around. Can you point me towards any references to the rest of the objects you're setting to null in case I am missing any?
> > > 
> > > `ASTMutationListener` seems to be the only one that's relevant. It's indeed exposed through `ASTContext` and our indexing operations can trigger various callbacks to fire. Can you set only that one to nullptr and leave a comment explaining the situation?
> > 
> > Thanks for the review.  I will revise based on the feedback. 
> > 
> > In buildPreamble, we create CppFilePreambleCallbacks in stack to be used in PrecompiledPreamble::Build which becomes part of PrecompilePreambleConsumer. I think this this need to be reset. Probably this has to be done in a different way. If I dont reset, I am seeing crash in some cases. See:
> > 
> > ```
> > 85c0c79f in clang::ASTReader::DecodeIdentifierInfo(unsigned int) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReader.cpp:8695:32
> >     #1 0x558285c75266 in clang::ASTReader::readIdentifier(clang::serialization::ModuleFile&, llvm::SmallVector<unsigned long, 64u> const&, unsigned int&) /home/kugan/local/llvm-project/clang/include/clang/Seriali
> > zation/ASTReader.h:2126:12
> >     #2 0x558285c75266 in clang::ASTRecordReader::readIdentifier() /home/kugan/local/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:211:20
> >     #3 0x558285c75266 in clang::serialization::BasicReaderBase<clang::ASTRecordReader>::readDeclarationName() /home/kugan/local/llvm-project/build/tools/clang/include/clang/AST/AbstractBasicReader.inc:780:58
> >     #4 0x558285ce10bc in clang::ASTDeclReader::VisitNamedDecl(clang::NamedDecl*) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:694:26
> >     #5 0x558285ce10bc in clang::ASTDeclReader::VisitNamespaceDecl(clang::NamespaceDecl*) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:1785:3
> >     #6 0x558285cbc6e8 in clang::ASTDeclReader::Visit(clang::Decl*) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:543:37
> >     #7 0x558285d5a133 in clang::ASTReader::ReadDeclRecord(unsigned int) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:4052:10
> >     #8 0x558285c73677 in clang::ASTReader::GetDecl(unsigned int) /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReader.cpp:7607:5
> >     #9 0x558285c73677 in clang::ASTReader::GetLocalDecl(clang::serialization::ModuleFile&, unsigned int) /home/kugan/local/llvm-project/clang/include/clang/Serialization/ASTReader.h:1913:12
> >     #10 0x558285bfd741 in clang::ASTReader::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&)::$_9::operator()(clang::serializ
> > ation::ModuleFile*, llvm::ArrayRef<llvm::support::detail::packed_endian_specific_integral<unsigned int, (llvm::support::endianness)2, 1ul, 1ul>>) const /home/kugan/local/llvm-project/clang/lib/Serialization/ASTRe
> > ader.cpp:7687:21
> >     #11 0x558285bfd741 in clang::ASTReader::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) /home/kugan/local/llvm-project/c
> > lang/lib/Serialization/ASTReader.cpp:7697:7
> >     #12 0x55827e942de7 in clang::ExternalASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::SmallVectorImpl<clang::Decl*>&) /home/kugan/local/llvm-project/clang/include/clang/AST/ExternalASTSour
> > ce.h:185:5
> >     #13 0x55827e942de7 in clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const /home/kugan/local/llvm-project/clang/lib/AST/DeclBase.cpp:1421:11
> >     #14 0x55827e94436b in clang::DeclContext::decls_begin() const /home/kugan/local/llvm-project/clang/lib/AST/DeclBase.cpp:1477:5
> >     #15 0x5582822787ab in clang::DeclContext::decls() const /home/kugan/local/llvm-project/clang/include/clang/AST/DeclBase.h:2188:48
> >     #16 0x5582822787ab in clang::clangd::indexHeaderSymbols(llvm::StringRef, clang::ASTContext&, clang::Preprocessor&, clang::clangd::CanonicalIncludes const&) /home/kugan/local/llvm-project/clang-tools-extra/cla
> > ngd/index/FileIndex.cpp:233:37
> >     #17 0x558282281be9 in clang::clangd::FileIndex::updatePreamble(llvm::StringRef, llvm::StringRef, clang::ASTContext&, clang::Preprocessor&, clang::clangd::CanonicalIncludes const&) /home/kugan/local/llvm-proje
> > ct/clang-tools-extra/clangd/index/FileIndex.cpp:464:7
> > ```
> > 
> > 
> > 
> ah, this one is unfortunately a little bit more involved, and triggers only in the cases where we have modules enabled (which is incidentally working in clangd today, but effect is wider as -std=c++20 implicitly turns on -fmodules).
> 
> tl;dr; astcontext is keeping references to pchgenerator in module-enabled builds.
> 
> TBH the safest option here seems like clearing external source in astcontext completely. As we can't really risk callbacks to PCHGenerator, even if we fixed all the life-time concerns (ast consumer keeps references to frontendaction alive, so both the callbacks we pass here and the action itself needs to outlive the consumer).
> 
> But that's likely going to break preamble-indexing completely for codebases that use modules (we'll likely only index top-level decls from the current modules, but I am not sure). The alternative suggested here strikes some middle ground (there's still going to be behavior change, previously all the modules accessed during indexing would effect the final serialized preamble, they won't anymore) but I am afraid it's too fragile. The dependency between these clang-internals were so subtle that it took ~hours for me to analyze, despite me specifically looking for them, hence I don't think anyone that's unaware can preserve these invariants (i.e. make sure we don't have references to astconsumer in other places.).
> 
> I am not sure how these work for you today, but can you check if just setting external source on the astcontext to null is good enough?
Thanks for checking.  Unfortunately, just setting external source on the astcontext to null is not working. Setting PrecompilePreambleConsumer/PCHGenerator  and ASTMutationListener  to null seem to be sufficient.

I checked the Preamble index size with and without the patch for some files and they are the same. Updating to the revised patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148088