[clang][ASTContext] Simplify caching for declaration-related comments

[jkorous]

Summary

• I assume we can cache comments for canonical declarations only, not for every redeclaration.
• Caching that we didn't find comments seems to be unnecessary since we try to search for comment again again next time if we find this data in cache.
  • We might implement proper cache invalidation in the future and get back to using this information.
• Origin of comment (directly from declaration / from some redeclaration) seems to not be used anywhere.

I plan to do some performance testing before committing but like to have some feedback first.

BTW there's another cache for comments in the ASTContext - ParsedComments. We could try to experiment with different caching approaches to see how it affects performance - maybe caching mapping from canonical declarations to raw comments and separately caching mapping from raw comments to full comments.

[arphaman]

clang/lib/AST/ASTContext.cpp

372

Why are we now checking for the canonical declaration instead of D as before?

[jkorous]

clang/lib/AST/ASTContext.cpp

372

Before this we were caching comment for every redeclaration separately but for every redeclaration the comment was the same.

As I understand it - for a given declaration we found the first comment in the redeclaration chain and then saved it to the cache for every redeclaration (the same comment).
Later, during lookup, we iterated the redeclaration chain again and did a lookup for every redeclaration (see L392 in the original implementation).

Unless I missed something, it's suboptimal in both memory consumption and runtime overhead.

That's the reason why I want to cache the comment for the redeclaration group as a whole. The only thing I am not entirely sure about is what key to use to represent the whole redeclaration chain - maybe the first declaration in the redeclaration chain would be better then the canonical declaration...

WDYT?

[gribozavr]

clang/include/clang/AST/ASTContext.h

756

The name of this variable (and RawCommentAndCacheFlags) don't make sense after the change.

clang/lib/AST/ASTContext.cpp

372

As I understand it - for a given declaration we found the first comment in the redeclaration chain and then saved it to the cache for every redeclaration (the same comment).

Only if there was only one comment in the redeclaration chain. If a redeclaration had a different comment, this function would return that comment.

[jkorous]

clang/lib/AST/ASTContext.cpp

372

I see. I made a wrong assumption - that for any two redeclarations the redeclaration chain always starts from the same declaration.

[jkorous]

Second attempt on reducing the cache size and number of operations.

I try in this order

1. cache lookup for the specific provided declaration
2. try to find comment attached to the specific provided declaration without using cache (and cache it using the specific declaration as a key)
3. cache lookup using the canonical declaration (which would return comment from any redeclaration - see the next step)
4. try to find comment attached to any redeclaration (and cache it using the canonical declaration as a key)
5. give up, return nullptr

[jkorous]

rename RawCommentAndCacheFlags -> CommentAndOrigin

[gribozavr]

clang/include/clang/AST/ASTContext.h

728

RawCommentAndOrigin?

751

Please remove "canonical", it is not correct according to the implementation.

Please also update the patch description that currently says "I assume we can cache comments for canonical declarations only, not for every redeclaration."

clang/lib/AST/ASTContext.cpp

372

I still don't think this patch is a no-op.

Here is the intended behavior:

/// First
void foo(); // (1), canonical decl

void foo(); // (2)

/// Third
void foo() {} // (3)

void foo(); // (4)

getRawCommentForAnyRedecl(1) => "First"
getRawCommentForAnyRedecl(2) => "First"
getRawCommentForAnyRedecl(3) => "Third"
getRawCommentForAnyRedecl(4) => "First"

373

GetCommentFromCache?

405

No space after *.

415

I don't understand the comment -- I read it as "if we got here, and the decl was canonical, and we haven't found anything already, we should return nullptr". The code does something different.

Also, just to be clear, here is the intended behavior:

void foo(); // (1), canonical decl

/// Second
void foo(); // (2)

getRawCommentForAnyRedecl(1) => "Second"
getRawCommentForAnyRedecl(2) => "Second"

418

When is it possible to not have a canonical decl?

421

No space after *.

425

80 columns.

clang/include/clang/AST/ASTContext.h

public:
   }
 
   /// All comments in this translation unit.
   RawCommentList Comments;
 
   /// True if comments are already loaded from ExternalASTSource.
   mutable bool CommentsLoaded = false;
 
-  class RawCommentAndCacheFlags {
+  class CommentAndOrigin {
   public:
-    enum Kind {
-      /// We searched for a comment attached to the particular declaration, but
-      /// didn't find any.
-      ///
-      /// getRaw() == 0.
-      NoCommentInDecl = 0,
-
-      /// We have found a comment attached to this particular declaration.
-      ///
-      /// getRaw() != 0.
-      FromDecl,
-
-      /// This declaration does not have an attached comment, and we have
-      /// searched the redeclaration chain.
-      ///
-      /// If getRaw() == 0, the whole redeclaration chain does not have any
-      /// comments.
-      ///
-      /// If getRaw() != 0, it is a comment propagated from other
-      /// redeclaration.
-      FromRedecl
-    };
-
-    Kind getKind() const LLVM_READONLY {
-      return Data.getInt();
-    }
-
-    void setKind(Kind K) {
-      Data.setInt(K);
-    }
-
     const RawComment *getRaw() const LLVM_READONLY {
-      return Data.getPointer();
+      return Comment;
     }
 
     void setRaw(const RawComment *RC) {
-      Data.setPointer(RC);
+      Comment = RC;
     }
 
     const Decl *getOriginalDecl() const LLVM_READONLY {
       return OriginalDecl;
     }
 
     void setOriginalDecl(const Decl *Orig) {
       OriginalDecl = Orig;
     }
 
   private:
-    llvm::PointerIntPair<const RawComment *, 2, Kind> Data;
+    const RawComment * Comment;
     const Decl *OriginalDecl;
   };
 
-  /// Mapping from declarations to comments attached to any
+  /// Mapping from canonical declarations to comments attached to any
   /// redeclaration.
   ///
   /// Raw comments are owned by Comments list.  This mapping is populated
   /// lazily.
-  mutable llvm::DenseMap<const Decl *, RawCommentAndCacheFlags> RedeclComments;
+  mutable llvm::DenseMap<const Decl *, CommentAndOrigin> 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;

clang/lib/AST/ASTContext.cpp

static const Decl *adjustDeclToTemplate(const Decl *D) {
   // FIXME: Adjust alias templates?
   return D;
 }
 
 const RawComment *ASTContext::getRawCommentForAnyRedecl(
                                                 const Decl *D,
                                                 const Decl **OriginalDecl) 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);
+  auto CacheLookup = [&](const Decl *SpecificD) -> const RawComment * {
+    llvm::DenseMap<const Decl *, CommentAndOrigin>::iterator Pos =
+        RedeclComments.find(SpecificD);
     if (Pos != RedeclComments.end()) {
-      const RawCommentAndCacheFlags &Raw = Pos->second;
-      if (Raw.getKind() != RawCommentAndCacheFlags::NoCommentInDecl) {
-        if (OriginalDecl)
-          *OriginalDecl = Raw.getOriginalDecl();
-        return Raw.getRaw();
-      }
-    }
+      const CommentAndOrigin &CachedComment = Pos->second;
+        *OriginalDecl = CachedComment.getOriginalDecl();
+      return CachedComment.getRaw();
     }
+    return nullptr;
+  };
 
-  // Search for comments attached to declarations in the redeclaration chain.
-  const RawComment *RC = nullptr;
-  const Decl *OriginalDeclForRC = nullptr;
-  for (auto I : D->redecls()) {
-    llvm::DenseMap<const Decl *, RawCommentAndCacheFlags>::iterator Pos =
-        RedeclComments.find(I);
-    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);
-      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
-        Raw.setKind(RawCommentAndCacheFlags::NoCommentInDecl);
-      Raw.setOriginalDecl(I);
-      RedeclComments[I] = Raw;
-      if (RC)
-        break;
-    }
+  auto GetCommentNoCache = [&](const Decl *SpecificD) -> const llvm::Optional<CommentAndOrigin> {
+    if (const RawComment *RC = getRawCommentForDeclNoCache(SpecificD)) {
+      // If we find a comment, it should be a documentation comment.
+      assert(RC->isDocumentation() || LangOpts.CommentOpts.ParseAllComments);
+
+      CommentAndOrigin NewCachedComment;
+      NewCachedComment.setRaw(RC);
+      NewCachedComment.setOriginalDecl(SpecificD);
+
+      if (OriginalDecl)
+        *OriginalDecl = SpecificD;
+
+      return NewCachedComment;
     }
+    return llvm::None;
+  };
 
-  // If we found a comment, it should be a documentation comment.
-  assert(!RC || RC->isDocumentation() || LangOpts.CommentOpts.ParseAllComments);
+  // Check whether we have cached a comment for this specific declaration.
+  if (auto * CachedComment = CacheLookup(D))
+    return CachedComment;
+
+  // Try to find comment for this specific declaration.
+  if (llvm::Optional<CommentAndOrigin> OptCommentAndFlags = GetCommentNoCache(D)) {
+    const CommentAndOrigin& CommentAndFlags = OptCommentAndFlags.getValue();
+    RedeclComments[D] = CommentAndFlags;
+    return CommentAndFlags.getRaw();
+  }
 
-  if (OriginalDecl)
-    *OriginalDecl = OriginalDeclForRC;
+  // In case this is the canonical Decl we're done.
+  auto CanonicalD = D->getCanonicalDecl();
+  if (!CanonicalD)
+    return nullptr;
 
-  // Update cache for every declaration in the redeclaration chain.
-  RawCommentAndCacheFlags Raw;
-  Raw.setRaw(RC);
-  Raw.setKind(RawCommentAndCacheFlags::FromRedecl);
-  Raw.setOriginalDecl(OriginalDeclForRC);
+  // Check whether we have cached a comment for canonical declaration of this declaration.
+  if (auto * CachedComment = CacheLookup(CanonicalD))
+    return CachedComment;
 
+  // We don't have comment for neither D, nor it's canonical declaration in cache and D doesn't have any comment attached to itself.
+  // Search for any comment attached to redeclarations of D and cache it for canonical declaration of D.
   for (auto I : D->redecls()) {
-    RawCommentAndCacheFlags &R = RedeclComments[I];
-    if (R.getKind() == RawCommentAndCacheFlags::NoCommentInDecl)
-      R = Raw;
+    if (llvm::Optional<CommentAndOrigin> OptCommentAndFlags = GetCommentNoCache(I)) {
+      const CommentAndOrigin& CommentAndFlags = OptCommentAndFlags.getValue();
+      RedeclComments[CanonicalD] = CommentAndFlags;
+      return CommentAndFlags.getRaw();
+    }
   }
 
-  return RC;
+  // We didn't find any comment attached to any redeclaration of D.
+  return nullptr;
 }
 
 static void addRedeclaredMethods(const ObjCMethodDecl *ObjCMethod,
                    SmallVectorImpl<const NamedDecl *> &Redeclared) {
   const DeclContext *DC = ObjCMethod->getDeclContext();
   if (const auto *IMD = dyn_cast<ObjCImplDecl>(DC)) {
     const ObjCInterfaceDecl *ID = IMD->getClassInterface();
     if (!ID)