<div dir="ltr">Sent <a href="http://reviews.llvm.org/D5826">http://reviews.llvm.org/D5826</a> as a follow change.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 16, 2014 at 4:13 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Sorry for not chiming in earlier. A few nits.<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 15, 2014 at 4:58 PM, Samuel Benzaquen <span dir="ltr"><<a href="mailto:sbenza@google.com" target="_blank">sbenza@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: sbenza<br>
Date: Wed Oct 15 09:58:46 2014<br>
New Revision: 219792<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=219792&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=219792&view=rev</a><br>
Log:<br>
Speed up hasName() matcher.<br>
<br>
Summary:<br>
Speed up hasName() matcher by skipping the expensive generation of the<br>
fully qualified name unless we need it.<br>
In the common case of matching an unqualified name, we don't need to<br>
generate the full name. We might not even need to copy any string at<br>
all.<br>
This change speeds up our clang-tidy benchmark by ~10%<br>
<br>
Reviewers: klimek<br>
<br>
Subscribers: klimek, cfe-commits<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D5776" target="_blank">http://reviews.llvm.org/D5776</a><br>
<br>
Modified:<br>
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h<br>
cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h<br>
cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp<br>
<br>
Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=219792&r1=219791&r2=219792&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=219792&r1=219791&r2=219792&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)<br>
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Wed Oct 15 09:58:46 2014<br>
@@ -1591,16 +1591,8 @@ inline internal::Matcher<Stmt> sizeOfExp<br>
/// \code<br>
/// namespace a { namespace b { class X; } }<br>
/// \endcode<br>
-AST_MATCHER_P(NamedDecl, hasName, std::string, Name) {<br>
- assert(!Name.empty());<br>
- const std::string FullNameString = "::" + Node.getQualifiedNameAsString();<br>
- const StringRef FullName = FullNameString;<br>
- const StringRef Pattern = Name;<br>
- if (Pattern.startswith("::")) {<br>
- return FullName == Pattern;<br>
- } else {<br>
- return FullName.endswith(("::" + Pattern).str());<br>
- }<br>
+inline internal::Matcher<NamedDecl> hasName(StringRef Name) {<br>
+ return internal::Matcher<NamedDecl>(new internal::HasNameMatcher(Name));<br>
}<br>
<br>
/// \brief Matches NamedDecl nodes whose fully qualified names contain<br>
<br>
Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=219792&r1=219791&r2=219792&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=219792&r1=219791&r2=219792&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)<br>
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Wed Oct 15 09:58:46 2014<br>
@@ -574,6 +574,33 @@ private:<br>
std::string Name;<br>
};<br>
<br>
+/// \brief Matches named declarations with a specific name.<br>
+///<br>
+/// See \c hasName() in ASTMatchers.h for details.<br>
+class HasNameMatcher : public SingleNodeMatcherInterface<NamedDecl> {<br>
+ public:<br>
+ explicit HasNameMatcher(StringRef Name);<br>
+<br>
+ bool matchesNode(const NamedDecl &Node) const override;<br>
+<br>
+ private:<br>
+ /// \brief Unqualified match routine.<br>
+ ///<br>
+ /// It is much faster than the full match, but it only works for unqualified<br>
+ /// matches.<br>
+ bool matchesNodeUnqualified(const NamedDecl &Node) const;<br>
+<br>
+ /// \brief Full match routine<br>
+ ///<br>
+ /// It generates the fully qualified name of the declaration (which is<br>
+ /// expensive) before trying to match.<br>
+ /// It is slower but simple and works on all cases.<br>
+ bool matchesNodeFull(const NamedDecl &Node) const;<br>
+<br>
+ const bool UseUnqualifiedMatch;<br>
+ const std::string Name;<br>
+};<br>
+<br>
/// \brief Matches declarations for QualType and CallExpr.<br>
///<br>
/// Type argument DeclMatcherT is required by PolymorphicMatcherWithParam1 but<br>
<br>
Modified: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=219792&r1=219791&r2=219792&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=219792&r1=219791&r2=219792&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp (original)<br>
+++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp Wed Oct 15 09:58:46 2014<br>
@@ -13,6 +13,7 @@<br>
<br>
#include "clang/ASTMatchers/ASTMatchers.h"<br>
#include "clang/ASTMatchers/ASTMatchersInternal.h"<br>
+#include "llvm/ADT/SmallString.h"<br>
#include "llvm/Support/ManagedStatic.h"<br>
<br>
namespace clang {<br>
@@ -221,6 +222,51 @@ bool AnyOfVariadicOperator(const ast_typ<br>
return false;<br>
}<br>
<br>
+HasNameMatcher::HasNameMatcher(StringRef NameRef)<br>
+ : UseUnqualifiedMatch(NameRef.find("::") == NameRef.npos), Name(NameRef) {<br>
+ assert(!Name.empty());<br>
+}<br>
+<br>
+bool HasNameMatcher::matchesNodeUnqualified(const NamedDecl &Node) const {<br>
+ assert(UseUnqualifiedMatch);<br>
+ if (Node.getIdentifier()) {<br>
+ // Simple name.<br>
+ return Name == Node.getName();<br>
+ } else if (Node.getDeclName()) {<br></blockquote><div><br></div><div><a href="http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return" target="_blank">http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ // Name needs to be constructed.<br>
+ llvm::SmallString<128> NodeName;<br>
+ llvm::raw_svector_ostream OS(NodeName);<br>
+ Node.printName(OS);<br>
+ return Name == OS.str();<br>
+ } else {<br></blockquote><div><br></div><div><a href="http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return" target="_blank">http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ return false;<br>
+ }<br>
+}<br>
+<br>
+bool HasNameMatcher::matchesNodeFull(const NamedDecl &Node) const {<br>
+ llvm::SmallString<128> NodeName = StringRef("::");<br>
+ llvm::raw_svector_ostream OS(NodeName);<br>
+ Node.printQualifiedName(OS);<br>
+ const StringRef FullName = OS.str();<br>
+ const StringRef Pattern = Name;<br>
+ if (Pattern.startswith("::")) {<br>
+ return FullName == Pattern;<br>
+ } else {<br></blockquote><div><br></div><div><a href="http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return" target="_blank">http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ return FullName.endswith(("::" + Pattern).str());<br></blockquote><div><br></div><div>If this shows on the profile, we can also avoid creating a temporary string here:</div><div><br></div><div>return FullName.endswith(Pattern) && FullName.drop_back(Pattern.size()).endswith("::");</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ }<br>
+}<br>
+<br>
+bool HasNameMatcher::matchesNode(const NamedDecl &Node) const {<br>
+ // FIXME: There is still room for improvement, but it would require copying a<br>
+ // lot of the logic from NamedDecl::printQualifiedName(). The benchmarks do<br>
+ // not show like that extra complexity is needed right now.<br>
+ if (UseUnqualifiedMatch) {<br>
+ assert(matchesNodeUnqualified(Node) == matchesNodeFull(Node));<br>
+ return matchesNodeUnqualified(Node);<br>
+ }<br>
+ return matchesNodeFull(Node);<br>
+}<br>
+<br>
} // end namespace internal<br>
} // end namespace ast_matchers<br>
} // end namespace clang<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br><br clear="all"><div><br></div>
</div></div>
</blockquote></div><br></div>