r219792 - Speed up hasName() matcher.

[Alexander Kornienko]

Sorry for not chiming in earlier. A few nits.

On Wed, Oct 15, 2014 at 4:58 PM, Samuel Benzaquen <sbenza at google.com> wrote:

> Author: sbenza
> Date: Wed Oct 15 09:58:46 2014
> New Revision: 219792
>
> URL: http://llvm.org/viewvc/llvm-project?rev=219792&view=rev
> Log:
> Speed up hasName() matcher.
>
> Summary:
> Speed up hasName() matcher by skipping the expensive generation of the
> fully qualified name unless we need it.
> In the common case of matching an unqualified name, we don't need to
> generate the full name. We might not even need to copy any string at
> all.
> This change speeds up our clang-tidy benchmark by ~10%
>
> Reviewers: klimek
>
> Subscribers: klimek, cfe-commits
>
> Differential Revision: http://reviews.llvm.org/D5776
>
> Modified:
>     cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
>     cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>     cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
>
> Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=219792&r1=219791&r2=219792&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
> +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Wed Oct 15 09:58:46
> 2014
> @@ -1591,16 +1591,8 @@ inline internal::Matcher<Stmt> sizeOfExp
>  /// \code
>  ///   namespace a { namespace b { class X; } }
>  /// \endcode
> -AST_MATCHER_P(NamedDecl, hasName, std::string, Name) {
> -  assert(!Name.empty());
> -  const std::string FullNameString = "::" +
> Node.getQualifiedNameAsString();
> -  const StringRef FullName = FullNameString;
> -  const StringRef Pattern = Name;
> -  if (Pattern.startswith("::")) {
> -    return FullName == Pattern;
> -  } else {
> -    return FullName.endswith(("::" + Pattern).str());
> -  }
> +inline internal::Matcher<NamedDecl> hasName(StringRef Name) {
> +  return internal::Matcher<NamedDecl>(new internal::HasNameMatcher(Name));
>  }
>
>  /// \brief Matches NamedDecl nodes whose fully qualified names contain
>
> Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=219792&r1=219791&r2=219792&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
> +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Wed Oct 15
> 09:58:46 2014
> @@ -574,6 +574,33 @@ private:
>    std::string Name;
>  };
>
> +/// \brief Matches named declarations with a specific name.
> +///
> +/// See \c hasName() in ASTMatchers.h for details.
> +class HasNameMatcher : public SingleNodeMatcherInterface<NamedDecl> {
> + public:
> +  explicit HasNameMatcher(StringRef Name);
> +
> +  bool matchesNode(const NamedDecl &Node) const override;
> +
> + private:
> +  /// \brief Unqualified match routine.
> +  ///
> +  /// It is much faster than the full match, but it only works for
> unqualified
> +  /// matches.
> +  bool matchesNodeUnqualified(const NamedDecl &Node) const;
> +
> +  /// \brief Full match routine
> +  ///
> +  /// It generates the fully qualified name of the declaration (which is
> +  /// expensive) before trying to match.
> +  /// It is slower but simple and works on all cases.
> +  bool matchesNodeFull(const NamedDecl &Node) const;
> +
> +  const bool UseUnqualifiedMatch;
> +  const std::string Name;
> +};
> +
>  /// \brief Matches declarations for QualType and CallExpr.
>  ///
>  /// Type argument DeclMatcherT is required by
> PolymorphicMatcherWithParam1 but
>
> Modified: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=219792&r1=219791&r2=219792&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp (original)
> +++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp Wed Oct 15 09:58:46
> 2014
> @@ -13,6 +13,7 @@
>
>  #include "clang/ASTMatchers/ASTMatchers.h"
>  #include "clang/ASTMatchers/ASTMatchersInternal.h"
> +#include "llvm/ADT/SmallString.h"
>  #include "llvm/Support/ManagedStatic.h"
>
>  namespace clang {
> @@ -221,6 +222,51 @@ bool AnyOfVariadicOperator(const ast_typ
>    return false;
>  }
>
> +HasNameMatcher::HasNameMatcher(StringRef NameRef)
> +    : UseUnqualifiedMatch(NameRef.find("::") == NameRef.npos),
> Name(NameRef) {
> +  assert(!Name.empty());
> +}
> +
> +bool HasNameMatcher::matchesNodeUnqualified(const NamedDecl &Node) const {
> +  assert(UseUnqualifiedMatch);
> +  if (Node.getIdentifier()) {
> +    // Simple name.
> +    return Name == Node.getName();
> +  } else if (Node.getDeclName()) {
>

http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


> +    // Name needs to be constructed.
> +    llvm::SmallString<128> NodeName;
> +    llvm::raw_svector_ostream OS(NodeName);
> +    Node.printName(OS);
> +    return Name == OS.str();
> +  } else {
>

http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


> +    return false;
> +  }
> +}
> +
> +bool HasNameMatcher::matchesNodeFull(const NamedDecl &Node) const {
> +  llvm::SmallString<128> NodeName = StringRef("::");
> +  llvm::raw_svector_ostream OS(NodeName);
> +  Node.printQualifiedName(OS);
> +  const StringRef FullName = OS.str();
> +  const StringRef Pattern = Name;
> +  if (Pattern.startswith("::")) {
> +    return FullName == Pattern;
> +  } else {
>

http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


> +    return FullName.endswith(("::" + Pattern).str());
>

If this shows on the profile, we can also avoid creating a temporary string
here:

return FullName.endswith(Pattern) &&
FullName.drop_back(Pattern.size()).endswith("::");


> +  }
> +}
> +
> +bool HasNameMatcher::matchesNode(const NamedDecl &Node) const {
> +  // FIXME: There is still room for improvement, but it would require
> copying a
> +  // lot of the logic from NamedDecl::printQualifiedName(). The
> benchmarks do
> +  // not show like that extra complexity is needed right now.
> +  if (UseUnqualifiedMatch) {
> +    assert(matchesNodeUnqualified(Node) == matchesNodeFull(Node));
> +    return matchesNodeUnqualified(Node);
> +  }
> +  return matchesNodeFull(Node);
> +}
> +
>  } // end namespace internal
>  } // end namespace ast_matchers
>  } // end namespace clang
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141016/6ae8cbd6/attachment.html>