[clang][ASTContext] Don't load external comments from Sema::ActOnDocumentableDecls

[jkorous]

Summary

Loading of external comments is expensive for large AST files and not necessary for all use-cases.

The way I understand the intention in ActOnDocumentableDecls is that we are just checking if some of existing comments should be attached to newly created Decls. I assume we don't need to load external comments since those Decls are "local".

SemaDecl.cpp:12390

// There is at least one comment that not attached to a decl.
// Maybe it should be attached to one of these decls?

[gribozavr]

The way I understand the intention in ActOnDocumentableDecls is that we are just checking if some of existing comments should be attached to newly created Decls. I assume we don't need to load external comments since those Decls are "local".

The point of ActOnDocumentableDecls is not to just check if comments should be attached. That can be done later, lazily. The point is to emit warnings in doc comments, which requires parsing the comment.

I feel like this change is making the APIs significantly worse, especially the high-level getCommentForDecl. This change is exposing an obscure low-level knob that many users don't know what to do with.

I think the root of the performance problem is that ActOnDocumentableDecls calls getCommentForDecl at all. Yes, it is convenient, however, it performs a lot of extra work -- like searching for comments on related decls, searching for comments on redecls etc., when all we want is to check whether these specific decls have any comments attached directly to them, and if yes, parse the comments. I think it would be better if you could add a function that does exactly that, and call it from ActOnDocumentableDecls.

[jkorous]

I agree and will try to decrease the impact on interfaces.

On the functional side - does this look like a reasonable idea?

[gribozavr]

On the functional side - does this look like a reasonable idea?

Yes, totally.

[jkorous]

Abandonned in favor of https://reviews.llvm.org/D61103

clang/include/clang/AST/ASTContext.h

public:
   mutable llvm::DenseMap<const Decl *, RawCommentAndCacheFlags> RedeclComments;
 
   /// Mapping from declarations to parsed comments attached to any
   /// redeclaration.
   mutable llvm::DenseMap<const Decl *, comments::FullComment *> ParsedComments;
 
   /// Return the documentation comment attached to a given declaration,
   /// without looking into cache.
-  RawComment *getRawCommentForDeclNoCache(const Decl *D) const;
+  RawComment *getRawCommentForDeclNoCache(const Decl *D,
+                                          bool LoadExternal) const;
 
 public:
   RawCommentList &getRawCommentList() {
     return Comments;
   }
 
   void addComment(const RawComment &RC) {
     assert(LangOpts.RetainCommentsFromSystemHeaders ||
            !SourceMgr.isInSystemHeader(RC.getSourceRange().getBegin()));
     Comments.addComment(RC, LangOpts.CommentOpts, BumpAlloc);
   }
 
   /// Return the documentation comment attached to a given declaration.
   /// Returns nullptr if no comment is attached.
   ///
   /// \param OriginalDecl if not nullptr, is set to declaration AST node that
   /// had the comment, if the comment we found comes from a redeclaration.
-  const RawComment *
-  getRawCommentForAnyRedecl(const Decl *D,
-                            const Decl **OriginalDecl = nullptr) const;
+  const RawComment *getRawCommentForAnyRedecl(const Decl *D,
+                                              const Decl **OriginalDecl,
+                                              bool LoadExternal) const;
 
   /// Return parsed documentation comment attached to a given declaration.
   /// Returns nullptr if no comment is attached.
   ///
   /// \param PP the Preprocessor used with this TU.  Could be nullptr if
   /// preprocessor is not available.
   comments::FullComment *getCommentForDecl(const Decl *D,
-                                           const Preprocessor *PP) const;
+                                           const Preprocessor *PP,
+                                           bool LoadExternal) const;
 
   /// Return parsed documentation comment attached to a given declaration.
   /// Returns nullptr if no comment is attached. Does not look at any
   /// redeclarations of the declaration.
   comments::FullComment *getLocalCommentForDeclUncached(const Decl *D) const;
 
   comments::FullComment *cloneFullComment(comments::FullComment *FC,
                                          const Decl *D) const;

clang/lib/AST/ASTContext.cpp

 #include <utility>
 
 using namespace clang;
 
 enum FloatingRank {
   Float16Rank, HalfRank, FloatRank, DoubleRank, LongDoubleRank, Float128Rank
 };
 
-RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
+RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D,
+                                                    bool LoadExternal) const {
   assert(D);
 
   // If we already tried to load comments but there are none,
   // we won't find anything.
   if (CommentsLoaded && Comments.getComments().empty())
     return nullptr;
 
   // User can not attach documentation to implicit declarations.
else {
     }
   }
 
   // If the declaration doesn't map directly to a location in a file, we
   // can't find the comment.
   if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
     return nullptr;
 
-  if (!CommentsLoaded && ExternalSource) {
+  if (LoadExternal && !CommentsLoaded && ExternalSource) {
     ExternalSource->ReadComments();
 
 #ifndef NDEBUG
     ArrayRef<RawComment *> RawComments = Comments.getComments();
     assert(std::is_sorted(RawComments.begin(), RawComments.end(),
                           BeforeThanCompare<RawComment>(SourceMgr)));
 #endif
 
if (const EnumDecl *MemberDecl = ED->getInstantiatedFromMemberEnum())
       return MemberDecl;
 
     return D;
   }
   // FIXME: Adjust alias templates?
   return D;
 }
 
-const RawComment *ASTContext::getRawCommentForAnyRedecl(
-                                                const Decl *D,
-                                                const Decl **OriginalDecl) const {
+const RawComment *
+ASTContext::getRawCommentForAnyRedecl(const Decl *D, const Decl **OriginalDecl,
+                                      bool LoadExternal) const {
   D = adjustDeclToTemplate(D);
 
   // Check whether we have cached a comment for this declaration already.
   {
     llvm::DenseMap<const Decl *, RawCommentAndCacheFlags>::iterator Pos =
         RedeclComments.find(D);
     if (Pos != RedeclComments.end()) {
       const RawCommentAndCacheFlags &Raw = Pos->second;
for (auto I : D->redecls()) {
     if (Pos != RedeclComments.end()) {
       const RawCommentAndCacheFlags &Raw = Pos->second;
       if (Raw.getKind() != RawCommentAndCacheFlags::NoCommentInDecl) {
         RC = Raw.getRaw();
         OriginalDeclForRC = Raw.getOriginalDecl();
         break;
       }
     } else {
-      RC = getRawCommentForDeclNoCache(I);
+      RC = getRawCommentForDeclNoCache(I, LoadExternal);
       OriginalDeclForRC = I;
       RawCommentAndCacheFlags Raw;
       if (RC) {
         // Call order swapped to work around ICE in VS2015 RTM (Release Win32)
         // https://connect.microsoft.com/VisualStudio/feedback/details/1741530
         Raw.setKind(RawCommentAndCacheFlags::FromDecl);
         Raw.setRaw(RC);
       } else
if (!ThisDeclInfo->TemplateParameters)
     ThisDeclInfo->TemplateParameters = FC->getDeclInfo()->TemplateParameters;
   comments::FullComment *CFC =
     new (*this) comments::FullComment(FC->getBlocks(),
                                       ThisDeclInfo);
   return CFC;
 }
 
 comments::FullComment *ASTContext::getLocalCommentForDeclUncached(const Decl *D) const {
-  const RawComment *RC = getRawCommentForDeclNoCache(D);
+  const RawComment *RC = getRawCommentForDeclNoCache(D, /*LoadExternal=*/true);
   return RC ? RC->parse(*this, nullptr, D) : nullptr;
 }
 
-comments::FullComment *ASTContext::getCommentForDecl(
-                                              const Decl *D,
-                                              const Preprocessor *PP) const {
+comments::FullComment *ASTContext::getCommentForDecl(const Decl *D,
+                                                     const Preprocessor *PP,
+                                                     bool LoadExternal) const {
   if (D->isInvalidDecl())
     return nullptr;
   D = adjustDeclToTemplate(D);
 
   const Decl *Canonical = D->getCanonicalDecl();
   llvm::DenseMap<const Decl *, comments::FullComment *>::iterator Pos =
       ParsedComments.find(Canonical);
 
   if (Pos != ParsedComments.end()) {
     if (Canonical != D) {
       comments::FullComment *FC = Pos->second;
       comments::FullComment *CFC = cloneFullComment(FC, D);
       return CFC;
     }
     return Pos->second;
   }
 
   const Decl *OriginalDecl;
 
-  const RawComment *RC = getRawCommentForAnyRedecl(D, &OriginalDecl);
+  const RawComment *RC =
+      getRawCommentForAnyRedecl(D, &OriginalDecl, LoadExternal);
   if (!RC) {
     if (isa<ObjCMethodDecl>(D) || isa<FunctionDecl>(D)) {
       SmallVector<const NamedDecl*, 8> Overridden;
       const auto *OMD = dyn_cast<ObjCMethodDecl>(D);
       if (OMD && OMD->isPropertyAccessor())
         if (const ObjCPropertyDecl *PDecl = OMD->findPropertyDecl())
-          if (comments::FullComment *FC = getCommentForDecl(PDecl, PP))
+          if (comments::FullComment *FC =
+                  getCommentForDecl(PDecl, PP, LoadExternal))
             return cloneFullComment(FC, D);
       if (OMD)
         addRedeclaredMethods(OMD, Overridden);
       getOverriddenMethods(dyn_cast<NamedDecl>(D), Overridden);
       for (unsigned i = 0, e = Overridden.size(); i < e; i++)
-        if (comments::FullComment *FC = getCommentForDecl(Overridden[i], PP))
+        if (comments::FullComment *FC =
+                getCommentForDecl(Overridden[i], PP, LoadExternal))
           return cloneFullComment(FC, D);
     }
     else if (const auto *TD = dyn_cast<TypedefNameDecl>(D)) {
       // Attach any tag type's documentation to its typedef if latter
       // does not have one of its own.
       QualType QT = TD->getUnderlyingType();
       if (const auto *TT = QT->getAs<TagType>())
         if (const Decl *TD = TT->getDecl())
-          if (comments::FullComment *FC = getCommentForDecl(TD, PP))
+          if (comments::FullComment *FC =
+                  getCommentForDecl(TD, PP, LoadExternal))
             return cloneFullComment(FC, D);
     }
     else if (const auto *IC = dyn_cast<ObjCInterfaceDecl>(D)) {
       while (IC->getSuperClass()) {
         IC = IC->getSuperClass();
-        if (comments::FullComment *FC = getCommentForDecl(IC, PP))
+        if (comments::FullComment *FC = getCommentForDecl(IC, PP, LoadExternal))
           return cloneFullComment(FC, D);
       }
     }
     else if (const auto *CD = dyn_cast<ObjCCategoryDecl>(D)) {
       if (const ObjCInterfaceDecl *IC = CD->getClassInterface())
-        if (comments::FullComment *FC = getCommentForDecl(IC, PP))
+        if (comments::FullComment *FC = getCommentForDecl(IC, PP, LoadExternal))
           return cloneFullComment(FC, D);
     }
     else if (const auto *RD = dyn_cast<CXXRecordDecl>(D)) {
       if (!(RD = RD->getDefinition()))
         return nullptr;
       // Check non-virtual bases.
       for (const auto &I : RD->bases()) {
         if (I.isVirtual() || (I.getAccessSpecifier() != AS_public))
           continue;
         QualType Ty = I.getType();
         if (Ty.isNull())
           continue;
         if (const CXXRecordDecl *NonVirtualBase = Ty->getAsCXXRecordDecl()) {
           if (!(NonVirtualBase= NonVirtualBase->getDefinition()))
             continue;
 
-          if (comments::FullComment *FC = getCommentForDecl((NonVirtualBase), PP))
+          if (comments::FullComment *FC =
+                  getCommentForDecl((NonVirtualBase), PP, LoadExternal))
             return cloneFullComment(FC, D);
         }
       }
       // Check virtual bases.
       for (const auto &I : RD->vbases()) {
         if (I.getAccessSpecifier() != AS_public)
           continue;
         QualType Ty = I.getType();
         if (Ty.isNull())
           continue;
         if (const CXXRecordDecl *VirtualBase = Ty->getAsCXXRecordDecl()) {
           if (!(VirtualBase= VirtualBase->getDefinition()))
             continue;
-          if (comments::FullComment *FC = getCommentForDecl((VirtualBase), PP))
+          if (comments::FullComment *FC =
+                  getCommentForDecl((VirtualBase), PP, LoadExternal))
             return cloneFullComment(FC, D);
         }
       }
     }
     return nullptr;
   }
 
   // If the RawComment was attached to other redeclaration of this Decl, we
   // should parse the comment in context of that other Decl.  This is important
   // because comments can contain references to parameter names which can be
   // different across redeclarations.
   if (D != OriginalDecl)
-    return getCommentForDecl(OriginalDecl, PP);
+    return getCommentForDecl(OriginalDecl, PP, LoadExternal);
 
   comments::FullComment *FC = RC->parse(*this, PP, D);
   ParsedComments[Canonical] = FC;
   return FC;
 }
 
 void
 ASTContext::CanonicalTemplateTemplateParm::Profile(llvm::FoldingSetNodeID &ID,

clang/lib/Sema/SemaCodeComplete.cpp

Result.AddTypedTextChunk(
       Result.getAllocator().CopyString(ND->getNameAsString()));
   return Result.TakeString();
 }
 
 const RawComment *clang::getCompletionComment(const ASTContext &Ctx,
                                               const NamedDecl *ND) {
   if (!ND)
     return nullptr;
-  if (auto *RC = Ctx.getRawCommentForAnyRedecl(ND))
+  if (auto *RC = Ctx.getRawCommentForAnyRedecl(ND, /*OriginalDecl=*/nullptr,
+                                               /*LoadExternal=*/true))
     return RC;
 
   // Try to find comment from a property for ObjC methods.
   const auto *M = dyn_cast<ObjCMethodDecl>(ND);
   if (!M)
     return nullptr;
   const ObjCPropertyDecl *PDecl = M->findPropertyDecl();
   if (!PDecl)
     return nullptr;
 
-  return Ctx.getRawCommentForAnyRedecl(PDecl);
+  return Ctx.getRawCommentForAnyRedecl(PDecl, /*OriginalDecl=*/nullptr,
+                                       /*LoadExternal=*/true);
 }
 
 const RawComment *clang::getPatternCompletionComment(const ASTContext &Ctx,
                                                      const NamedDecl *ND) {
   const auto *M = dyn_cast_or_null<ObjCMethodDecl>(ND);
   if (!M || !M->isPropertyAccessor())
     return nullptr;
 
   // Provide code completion comment for self.GetterName where
   // GetterName is the getter method for a property with name
   // different from the property name (declared via a property
   // getter attribute.
   const ObjCPropertyDecl *PDecl = M->findPropertyDecl();
   if (!PDecl)
     return nullptr;
   if (PDecl->getGetterName() == M->getSelector() &&
       PDecl->getIdentifier() != M->getIdentifier()) {
-    if (auto *RC = Ctx.getRawCommentForAnyRedecl(M))
+    if (auto *RC = Ctx.getRawCommentForAnyRedecl(M, /*OriginalDecl=*/nullptr,
+                                                 /*LoadExternal=*/true))
       return RC;
-    if (auto *RC = Ctx.getRawCommentForAnyRedecl(PDecl))
+    if (auto *RC = Ctx.getRawCommentForAnyRedecl(
+            PDecl, /*OriginalDecl=*/nullptr, /*LoadExternal=*/true))
       return RC;
   }
   return nullptr;
 }
 
 const RawComment *clang::getParameterComment(
     const ASTContext &Ctx,
     const CodeCompleteConsumer::OverloadCandidate &Result, unsigned ArgIndex) {
   auto FDecl = Result.getFunction();
   if (!FDecl)
     return nullptr;
   if (ArgIndex < FDecl->getNumParams())
-    return Ctx.getRawCommentForAnyRedecl(FDecl->getParamDecl(ArgIndex));
+    return Ctx.getRawCommentForAnyRedecl(FDecl->getParamDecl(ArgIndex),
+                                         /*OriginalDecl=*/nullptr,
+                                         /*LoadExternal=*/true);
   return nullptr;
 }
 
 /// Add function overload parameter chunks to the given code completion
 /// string.
 static void AddOverloadParameterChunks(ASTContext &Context,
                                        const PrintingPolicy &Policy,
                                        const FunctionDecl *Function,

clang/lib/Sema/SemaDecl.cpp

if (!Comments.empty() &&
     // There is at least one comment that not attached to a decl.
     // Maybe it should be attached to one of these decls?
     //
     // Note that this way we pick up not only comments that precede the
     // declaration, but also comments that *follow* the declaration -- thanks to
     // the lookahead in the lexer: we've consumed the semicolon and looked
     // ahead through comments.
     for (unsigned i = 0, e = Group.size(); i != e; ++i)
-      Context.getCommentForDecl(Group[i], &PP);
+      Context.getCommentForDecl(Group[i], &PP, /*LoadExternal=*/false);
   }
 }
 
 /// ActOnParamDeclarator - Called from Parser::ParseFunctionDeclarator()
 /// to introduce parameters into function prototype scope.
 Decl *Sema::ActOnParamDeclarator(Scope *S, Declarator &D) {
   const DeclSpec &DS = D.getDeclSpec();
 

clang/tools/libclang/CIndex.cpp

 }
 
 CXSourceRange clang_Cursor_getCommentRange(CXCursor C) {
   if (!clang_isDeclaration(C.kind))
     return clang_getNullRange();
 
   const Decl *D = getCursorDecl(C);
   ASTContext &Context = getCursorContext(C);
-  const RawComment *RC = Context.getRawCommentForAnyRedecl(D);
+  const RawComment *RC = Context.getRawCommentForAnyRedecl(
+      D, /*OriginalDecl=*/nullptr, /*LoadExternal=*/true);
   if (!RC)
     return clang_getNullRange();
 
   return cxloc::translateSourceRange(Context, RC->getSourceRange());
 }
 
 CXString clang_Cursor_getRawCommentText(CXCursor C) {
   if (!clang_isDeclaration(C.kind))
     return cxstring::createNull();
 
   const Decl *D = getCursorDecl(C);
   ASTContext &Context = getCursorContext(C);
-  const RawComment *RC = Context.getRawCommentForAnyRedecl(D);
+  const RawComment *RC = Context.getRawCommentForAnyRedecl(
+      D, /*OriginalDecl=*/nullptr, /*LoadExternal=*/true);
   StringRef RawText = RC ? RC->getRawText(Context.getSourceManager()) :
                            StringRef();
 
   // Don't duplicate the string because RawText points directly into source
   // code.
   return cxstring::createRef(RawText);
 }
 
 CXString clang_Cursor_getBriefCommentText(CXCursor C) {
   if (!clang_isDeclaration(C.kind))
     return cxstring::createNull();
 
   const Decl *D = getCursorDecl(C);
   const ASTContext &Context = getCursorContext(C);
-  const RawComment *RC = Context.getRawCommentForAnyRedecl(D);
+  const RawComment *RC = Context.getRawCommentForAnyRedecl(
+      D, /*OriginalDecl=*/nullptr, /*LoadExternal=*/true);
 
   if (RC) {
     StringRef BriefText = RC->getBriefText(Context);
 
     // Don't duplicate the string because RawComment ensures that this memory
     // will not go away.
     return cxstring::createRef(BriefText);
   }

clang/tools/libclang/CXComment.cpp

 CXComment clang_Cursor_getParsedComment(CXCursor C) {
   using namespace clang::cxcursor;
 
   if (!clang_isDeclaration(C.kind))
     return createCXComment(nullptr, nullptr);
 
   const Decl *D = getCursorDecl(C);
   const ASTContext &Context = getCursorContext(C);
-  const FullComment *FC = Context.getCommentForDecl(D, /*PP=*/nullptr);
+  const FullComment *FC =
+      Context.getCommentForDecl(D, /*PP=*/nullptr, /*LoadExternal=*/true);
 
   return createCXComment(FC, getCursorTU(C));
 }
 
 enum CXCommentKind clang_Comment_getKind(CXComment CXC) {
   const Comment *C = getASTNode(CXC);
   if (!C)
     return CXComment_Null;