[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments
gribozavr added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:728 - class RawCommentAndCacheFlags { + class CommentAndOrigin { public: `RawCommentAndOrigin`? Comment at: clang/include/clang/AST/ASTContext.h:751 - /// Mapping from declarations to comments attached to any + /// Mapping from canonical declarations to comments attached to any /// redeclaration. 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." Comment at: clang/lib/AST/ASTContext.cpp:372 D = adjustDeclToTemplate(D); + const Decl* CanonicalDecl = D->getCanonicalDecl(); jkorous wrote: > gribozavr wrote: > > jkorous wrote: > > > arphaman wrote: > > > > Why are we now checking for the canonical declaration instead of `D` as > > > > before? > > > 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? > > > 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. > I see. I made a wrong assumption - that for any two redeclarations the > redeclaration chain always starts from the same declaration. 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" Comment at: clang/lib/AST/ASTContext.cpp:373 - // Check whether we have cached a comment for this declaration already. - { -llvm::DenseMap::iterator Pos = -RedeclComments.find(D); + auto CacheLookup = [&](const Decl *SpecificD) -> const RawComment * { +llvm::DenseMap::iterator Pos = `GetCommentFromCache`? Comment at: clang/lib/AST/ASTContext.cpp:405 + // Check whether we have cached a comment for this specific declaration. + if (auto * CachedComment = CacheLookup(D)) +return CachedComment; No space after `*`. Comment at: clang/lib/AST/ASTContext.cpp:415 + + // In case this is the canonical Decl we're done. + auto CanonicalD = D->getCanonicalDecl(); 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" Comment at: clang/lib/AST/ASTContext.cpp:418 + if (!CanonicalD) +return nullptr; When is it possible to not have a canonical decl? Comment at: clang/lib/AST/ASTContext.cpp:421 + // Check whether we have cached a comment for canonical declaration of this declaration. + if (auto * CachedComment = CacheLookup(CanonicalD)) +return CachedComment; No space after `*`. Comment at: clang/lib/AST/ASTContext.cpp:425 + // 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()) { 80 columns. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60432/new/
[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments
jkorous updated this revision to Diff 194609. jkorous added a comment. rename RawCommentAndCacheFlags -> CommentAndOrigin CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60432/new/ https://reviews.llvm.org/D60432 Files: clang/include/clang/AST/ASTContext.h clang/lib/AST/ASTContext.cpp Index: clang/lib/AST/ASTContext.cpp === --- clang/lib/AST/ASTContext.cpp +++ clang/lib/AST/ASTContext.cpp @@ -370,70 +370,69 @@ const Decl **OriginalDecl) const { D = adjustDeclToTemplate(D); - // Check whether we have cached a comment for this declaration already. - { -llvm::DenseMap::iterator Pos = -RedeclComments.find(D); + auto CacheLookup = [&](const Decl *SpecificD) -> const RawComment * { +llvm::DenseMap::iterator Pos = +RedeclComments.find(SpecificD); if (Pos != RedeclComments.end()) { - const RawCommentAndCacheFlags = Pos->second; - if (Raw.getKind() != RawCommentAndCacheFlags::NoCommentInDecl) { -if (OriginalDecl) - *OriginalDecl = Raw.getOriginalDecl(); -return Raw.getRaw(); - } + const CommentAndOrigin = Pos->second; + if (OriginalDecl) +*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::iterator Pos = -RedeclComments.find(I); -if (Pos != RedeclComments.end()) { - const RawCommentAndCacheFlags = 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 { +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; + // Call order swapped to work around ICE in VS2015 RTM (Release Win32) + // https://connect.microsoft.com/VisualStudio/feedback/details/1741530 + 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; - if (OriginalDecl) -*OriginalDecl = OriginalDeclForRC; + // Try to find comment for this specific declaration. + if (llvm::Optional OptCommentAndFlags = GetCommentNoCache(D)) { +const CommentAndOrigin& CommentAndFlags = OptCommentAndFlags.getValue(); +RedeclComments[D] = CommentAndFlags; +return CommentAndFlags.getRaw(); + } + + // 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 = RedeclComments[I]; -if (R.getKind() == RawCommentAndCacheFlags::NoCommentInDecl) - R = Raw; +if (llvm::Optional OptCommentAndFlags = GetCommentNoCache(I)) { + const CommentAndOrigin& CommentAndFlags = OptCommentAndFlags.getValue(); + RedeclComments[CanonicalD] =
[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments
jkorous updated this revision to Diff 194604. jkorous added a comment. 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 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60432/new/ https://reviews.llvm.org/D60432 Files: clang/include/clang/AST/ASTContext.h clang/lib/AST/ASTContext.cpp Index: clang/lib/AST/ASTContext.cpp === --- clang/lib/AST/ASTContext.cpp +++ clang/lib/AST/ASTContext.cpp @@ -370,70 +370,69 @@ const Decl **OriginalDecl) const { D = adjustDeclToTemplate(D); - // Check whether we have cached a comment for this declaration already. - { + auto CacheLookup = [&](const Decl *SpecificD) -> const RawComment * { llvm::DenseMap::iterator Pos = -RedeclComments.find(D); +RedeclComments.find(SpecificD); if (Pos != RedeclComments.end()) { - const RawCommentAndCacheFlags = Pos->second; - if (Raw.getKind() != RawCommentAndCacheFlags::NoCommentInDecl) { -if (OriginalDecl) - *OriginalDecl = Raw.getOriginalDecl(); -return Raw.getRaw(); - } + const RawCommentAndCacheFlags = Pos->second; + if (OriginalDecl) +*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::iterator Pos = -RedeclComments.find(I); -if (Pos != RedeclComments.end()) { - const RawCommentAndCacheFlags = 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 { +if (const RawComment *RC = getRawCommentForDeclNoCache(SpecificD)) { + // If we find a comment, it should be a documentation comment. + assert(RC->isDocumentation() || LangOpts.CommentOpts.ParseAllComments); + + RawCommentAndCacheFlags NewCachedComment; + // Call order swapped to work around ICE in VS2015 RTM (Release Win32) + // https://connect.microsoft.com/VisualStudio/feedback/details/1741530 + 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; - if (OriginalDecl) -*OriginalDecl = OriginalDeclForRC; + // Try to find comment for this specific declaration. + if (llvm::Optional OptCommentAndFlags = GetCommentNoCache(D)) { +const RawCommentAndCacheFlags& CommentAndFlags = OptCommentAndFlags.getValue(); +RedeclComments[D] = CommentAndFlags; +return CommentAndFlags.getRaw(); + } + + // 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
[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments
jkorous marked an inline comment as done. jkorous added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:372 D = adjustDeclToTemplate(D); + const Decl* CanonicalDecl = D->getCanonicalDecl(); gribozavr wrote: > jkorous wrote: > > arphaman wrote: > > > Why are we now checking for the canonical declaration instead of `D` as > > > before? > > 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? > > 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. I see. I made a wrong assumption - that for any two redeclarations the redeclaration chain always starts from the same declaration. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60432/new/ https://reviews.llvm.org/D60432 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments
gribozavr added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:756 /// lazily. mutable llvm::DenseMap RedeclComments; The name of this variable (and `RawCommentAndCacheFlags`) don't make sense after the change. Comment at: clang/lib/AST/ASTContext.cpp:372 D = adjustDeclToTemplate(D); + const Decl* CanonicalDecl = D->getCanonicalDecl(); jkorous wrote: > arphaman wrote: > > Why are we now checking for the canonical declaration instead of `D` as > > before? > 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? > 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60432/new/ https://reviews.llvm.org/D60432 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments
jkorous marked an inline comment as done. jkorous added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:372 D = adjustDeclToTemplate(D); + const Decl* CanonicalDecl = D->getCanonicalDecl(); arphaman wrote: > Why are we now checking for the canonical declaration instead of `D` as > before? 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? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60432/new/ https://reviews.llvm.org/D60432 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments
arphaman added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:372 D = adjustDeclToTemplate(D); + const Decl* CanonicalDecl = D->getCanonicalDecl(); Why are we now checking for the canonical declaration instead of `D` as before? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60432/new/ https://reviews.llvm.org/D60432 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments
jkorous created this revision. jkorous added reviewers: gribozavr, arphaman, dexonsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. - 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`. Repository: rC Clang https://reviews.llvm.org/D60432 Files: clang/include/clang/AST/ASTContext.h clang/lib/AST/ASTContext.cpp Index: clang/lib/AST/ASTContext.cpp === --- clang/lib/AST/ASTContext.cpp +++ clang/lib/AST/ASTContext.cpp @@ -369,71 +369,43 @@ const Decl *D, const Decl **OriginalDecl) const { D = adjustDeclToTemplate(D); + const Decl* CanonicalDecl = D->getCanonicalDecl(); // Check whether we have cached a comment for this declaration already. { llvm::DenseMap::iterator Pos = -RedeclComments.find(D); +RedeclComments.find(CanonicalDecl); if (Pos != RedeclComments.end()) { const RawCommentAndCacheFlags = Pos->second; - if (Raw.getKind() != RawCommentAndCacheFlags::NoCommentInDecl) { -if (OriginalDecl) - *OriginalDecl = Raw.getOriginalDecl(); -return Raw.getRaw(); - } + if (OriginalDecl) +*OriginalDecl = Raw.getOriginalDecl(); + return Raw.getRaw(); } } - // Search for comments attached to declarations in the redeclaration chain. - const RawComment *RC = nullptr; - const Decl *OriginalDeclForRC = nullptr; + // We don't have comment for D in cache - search for any comment attached to + // declarations in the redeclaration chain. for (auto I : D->redecls()) { -llvm::DenseMap::iterator Pos = -RedeclComments.find(I); -if (Pos != RedeclComments.end()) { - const RawCommentAndCacheFlags = 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; -} - } - - // If we found a comment, it should be a documentation comment. - assert(!RC || RC->isDocumentation() || LangOpts.CommentOpts.ParseAllComments); +if (const RawComment *RC = getRawCommentForDeclNoCache(I)) { + // If we find a comment, it should be a documentation comment. + assert(RC->isDocumentation() || LangOpts.CommentOpts.ParseAllComments); - if (OriginalDecl) -*OriginalDecl = OriginalDeclForRC; + if (OriginalDecl) +*OriginalDecl = I; - // Update cache for every declaration in the redeclaration chain. - RawCommentAndCacheFlags Raw; - Raw.setRaw(RC); - Raw.setKind(RawCommentAndCacheFlags::FromRedecl); - Raw.setOriginalDecl(OriginalDeclForRC); + RawCommentAndCacheFlags NewCachedComment; + // Call order swapped to work around ICE in VS2015 RTM (Release Win32) + // https://connect.microsoft.com/VisualStudio/feedback/details/1741530 + NewCachedComment.setRaw(RC); + NewCachedComment.setOriginalDecl(I); + RedeclComments[CanonicalDecl] = NewCachedComment; - for (auto I : D->redecls()) { -RawCommentAndCacheFlags = RedeclComments[I]; -if (R.getKind() == RawCommentAndCacheFlags::NoCommentInDecl) - R = Raw; + return RC; +} } - return RC; + // We didn't find any comment attached to any redeclaration of D. + return nullptr; } static void addRedeclaredMethods(const ObjCMethodDecl *ObjCMethod, Index: clang/include/clang/AST/ASTContext.h === --- clang/include/clang/AST/ASTContext.h +++