r219792 - Speed up hasName() matcher.

[Samuel Benzaquen]

Sent http://reviews.llvm.org/D5826 as a follow change.

On Thu, Oct 16, 2014 at 4:13 AM, Alexander Kornienko <alexfh at google.com>
wrote:

> 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/53713190/attachment.html>