[clang] 9f86524 - [ASTMatcher] Fix a performance regression: memorize the child match.

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2020-06-30T15:43:51+02:00
New Revision: 9f865246a81759cdcb056c883e6f92fe6693b3d3

URL: https://github.com/llvm/llvm-project/commit/9f865246a81759cdcb056c883e6f92fe6693b3d3
DIFF: https://github.com/llvm/llvm-project/commit/9f865246a81759cdcb056c883e6f92fe6693b3d3.diff

LOG: [ASTMatcher] Fix a performance regression: memorize the child match.

D80025 introduced a performance regression: in some cases, it makes
clang-tidy readability-container-size-empty ~80x slower (running on an internal
huge TU, before that patch 12s vs after 950s).

after this patch, we go back to 12s.

Differential Revision: https://reviews.llvm.org/D82771

Added: 
    

Modified: 
    clang/lib/ASTMatchers/ASTMatchFinder.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index b4189069db83..563f3fc9a634 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -45,7 +45,9 @@ static const unsigned MaxMemoizationEntries = 10000;
 
 enum class MatchType {
   Ancestors,
-  Descendants
+
+  Descendants,
+  Child,
 };
 
 // We use memoization to avoid running the same matcher on the same
@@ -452,8 +454,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
                                   BoundNodesTreeBuilder *Builder, int MaxDepth,
                                   TraversalKind Traversal, BindKind Bind) {
     // For AST-nodes that don't have an identity, we can't memoize.
-    // When doing a single-level match, we don't need to memoize
-    if (!Node.getMemoizationData() || !Builder->isComparable() || MaxDepth == 1)
+    if (!Node.getMemoizationData() || !Builder->isComparable())
       return matchesRecursively(Node, Matcher, Builder, MaxDepth, Traversal,
                                 Bind);
 
@@ -463,7 +464,8 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
     // Note that we key on the bindings *before* the match.
     Key.BoundNodes = *Builder;
     Key.Traversal = Ctx.getParentMapContext().getTraversalKind();
-    Key.Type = MatchType::Descendants;
+    // Memoize result even doing a single-level match, it might be expensive.
+    Key.Type = MaxDepth == 1 ? MatchType::Child : MatchType::Descendants;
     MemoizationMap::iterator I = ResultCache.find(Key);
     if (I != ResultCache.end()) {
       *Builder = I->second.Nodes;
@@ -700,8 +702,10 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
                                             BoundNodesTreeBuilder *Builder,
                                             AncestorMatchMode MatchMode) {
     // For AST-nodes that don't have an identity, we can't memoize.
-    // When doing a single-level match, we don't need to memoize
-    if (!Builder->isComparable() || MatchMode == AncestorMatchMode::AMM_ParentOnly)
+    // When doing a single-level match, we don't need to memoize because
+    // ParentMap (in ASTContext) already memoizes the result.
+    if (!Builder->isComparable() ||
+        MatchMode == AncestorMatchMode::AMM_ParentOnly)
       return matchesAncestorOfRecursively(Node, Ctx, Matcher, Builder,
                                           MatchMode);