[clang-tidy] Speed up readability-container-contains#175121
[clang-tidy] Speed up readability-container-contains#175121localspook merged 2 commits intollvm:mainfrom
readability-container-contains#175121Conversation
|
@llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) ChangesThis is currently our most expensive check as measured by hyperfine \
--shell=none \
--prepare='cmake --build build/release --target clang-tidy' \
'./build/release/bin/clang-tidy --checks=-*,readability-container-contains all_headers.cpp -header-filter=.* -system-headers -- -std=c++23 -fno-delayed-template-parsing'First, the status quo: Time (mean ± σ): 5.243 s ± 0.158 s [User: 4.964 s, System: 0.248 s]
Range (min … max): 5.133 s … 5.612 s 10 runsThis PR improves that in two independent commits. The first commit changes the default traversal mode from Time (mean ± σ): 4.554 s ± 0.100 s [User: 4.273 s, System: 0.248 s]
Range (min … max): 4.442 s … 4.785 s 10 runsThe second commit merges all 12 of the check's Time (mean ± σ): 3.480 s ± 0.121 s [User: 3.264 s, System: 0.186 s]
Range (min … max): 3.400 s … 3.798 s 10 runsFull diff: https://github.com/llvm/llvm-project/pull/175121.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index efcf13d63b4ff..3b4113c4e2ed0 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -14,6 +14,7 @@
using namespace clang::ast_matchers;
namespace clang::tidy::readability {
+
void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
const auto Literal0 = integerLiteral(equals(0));
const auto Literal1 = integerLiteral(equals(1));
@@ -47,57 +48,36 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
const auto StringNpos = anyOf(declRefExpr(to(varDecl(hasName("npos")))),
memberExpr(member(hasName("npos"))));
- auto AddSimpleMatcher = [&](const auto &Matcher) {
- Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource, Matcher), this);
- };
-
- // Find membership tests which use `count()`.
- Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),
- hasSourceExpression(CountCall))
- .bind("positiveComparison"),
- this);
- AddSimpleMatcher(
- binaryOperation(hasOperatorName("!="), hasOperands(CountCall, Literal0))
- .bind("positiveComparison"));
- AddSimpleMatcher(
- binaryOperation(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0))
- .bind("positiveComparison"));
- AddSimpleMatcher(
- binaryOperation(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall))
- .bind("positiveComparison"));
- AddSimpleMatcher(binaryOperation(hasLHS(CountCall), hasOperatorName(">="),
- hasRHS(Literal1))
- .bind("positiveComparison"));
- AddSimpleMatcher(binaryOperation(hasLHS(Literal1), hasOperatorName("<="),
- hasRHS(CountCall))
- .bind("positiveComparison"));
-
- // Find inverted membership tests which use `count()`.
- AddSimpleMatcher(
- binaryOperation(hasOperatorName("=="), hasOperands(CountCall, Literal0))
- .bind("negativeComparison"));
- AddSimpleMatcher(binaryOperation(hasLHS(CountCall), hasOperatorName("<="),
- hasRHS(Literal0))
- .bind("negativeComparison"));
- AddSimpleMatcher(binaryOperation(hasLHS(Literal0), hasOperatorName(">="),
- hasRHS(CountCall))
- .bind("negativeComparison"));
- AddSimpleMatcher(
- binaryOperation(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1))
- .bind("negativeComparison"));
- AddSimpleMatcher(
- binaryOperation(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall))
- .bind("negativeComparison"));
-
- // Find membership tests based on `find() == end()` or `find() == npos`.
- AddSimpleMatcher(
- binaryOperation(hasOperatorName("!="),
- hasOperands(FindCall, anyOf(EndCall, StringNpos)))
- .bind("positiveComparison"));
- AddSimpleMatcher(
- binaryOperation(hasOperatorName("=="),
- hasOperands(FindCall, anyOf(EndCall, StringNpos)))
- .bind("negativeComparison"));
+ Finder->addMatcher(
+ traverse(TK_AsIs,
+ implicitCastExpr(hasImplicitDestinationType(booleanType()),
+ hasSourceExpression(CountCall))
+ .bind("positiveComparison")),
+ this);
+
+ const auto PositiveComparison =
+ anyOf(allOf(hasOperatorName("!="), hasOperands(CountCall, Literal0)),
+ allOf(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0)),
+ allOf(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall)),
+ allOf(hasLHS(CountCall), hasOperatorName(">="), hasRHS(Literal1)),
+ allOf(hasLHS(Literal1), hasOperatorName("<="), hasRHS(CountCall)),
+ allOf(hasOperatorName("!="),
+ hasOperands(FindCall, anyOf(EndCall, StringNpos))));
+
+ const auto NegativeComparison =
+ anyOf(allOf(hasOperatorName("=="), hasOperands(CountCall, Literal0)),
+ allOf(hasLHS(CountCall), hasOperatorName("<="), hasRHS(Literal0)),
+ allOf(hasLHS(Literal0), hasOperatorName(">="), hasRHS(CountCall)),
+ allOf(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1)),
+ allOf(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall)),
+ allOf(hasOperatorName("=="),
+ hasOperands(FindCall, anyOf(EndCall, StringNpos))));
+
+ Finder->addMatcher(
+ binaryOperation(
+ anyOf(allOf(PositiveComparison, expr().bind("positiveComparison")),
+ allOf(NegativeComparison, expr().bind("negativeComparison")))),
+ this);
}
void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
index 8e058f20427fd..8d043910fc2ba 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
@@ -29,7 +29,7 @@ class ContainerContainsCheck : public ClangTidyCheck {
return LO.CPlusPlus;
}
std::optional<TraversalKind> getCheckTraversalKind() const override {
- return TK_AsIs;
+ return TK_IgnoreUnlessSpelledInSource;
}
};
|
|
@llvm/pr-subscribers-clang-tidy Author: Victor Chernyakin (localspook) ChangesThis is currently our most expensive check as measured by hyperfine \
--shell=none \
--prepare='cmake --build build/release --target clang-tidy' \
'./build/release/bin/clang-tidy --checks=-*,readability-container-contains all_headers.cpp -header-filter=.* -system-headers -- -std=c++23 -fno-delayed-template-parsing'First, the status quo: Time (mean ± σ): 5.243 s ± 0.158 s [User: 4.964 s, System: 0.248 s]
Range (min … max): 5.133 s … 5.612 s 10 runsThis PR improves that in two independent commits. The first commit changes the default traversal mode from Time (mean ± σ): 4.554 s ± 0.100 s [User: 4.273 s, System: 0.248 s]
Range (min … max): 4.442 s … 4.785 s 10 runsThe second commit merges all 12 of the check's Time (mean ± σ): 3.480 s ± 0.121 s [User: 3.264 s, System: 0.186 s]
Range (min … max): 3.400 s … 3.798 s 10 runsFull diff: https://github.com/llvm/llvm-project/pull/175121.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index efcf13d63b4ff..3b4113c4e2ed0 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -14,6 +14,7 @@
using namespace clang::ast_matchers;
namespace clang::tidy::readability {
+
void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
const auto Literal0 = integerLiteral(equals(0));
const auto Literal1 = integerLiteral(equals(1));
@@ -47,57 +48,36 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
const auto StringNpos = anyOf(declRefExpr(to(varDecl(hasName("npos")))),
memberExpr(member(hasName("npos"))));
- auto AddSimpleMatcher = [&](const auto &Matcher) {
- Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource, Matcher), this);
- };
-
- // Find membership tests which use `count()`.
- Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),
- hasSourceExpression(CountCall))
- .bind("positiveComparison"),
- this);
- AddSimpleMatcher(
- binaryOperation(hasOperatorName("!="), hasOperands(CountCall, Literal0))
- .bind("positiveComparison"));
- AddSimpleMatcher(
- binaryOperation(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0))
- .bind("positiveComparison"));
- AddSimpleMatcher(
- binaryOperation(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall))
- .bind("positiveComparison"));
- AddSimpleMatcher(binaryOperation(hasLHS(CountCall), hasOperatorName(">="),
- hasRHS(Literal1))
- .bind("positiveComparison"));
- AddSimpleMatcher(binaryOperation(hasLHS(Literal1), hasOperatorName("<="),
- hasRHS(CountCall))
- .bind("positiveComparison"));
-
- // Find inverted membership tests which use `count()`.
- AddSimpleMatcher(
- binaryOperation(hasOperatorName("=="), hasOperands(CountCall, Literal0))
- .bind("negativeComparison"));
- AddSimpleMatcher(binaryOperation(hasLHS(CountCall), hasOperatorName("<="),
- hasRHS(Literal0))
- .bind("negativeComparison"));
- AddSimpleMatcher(binaryOperation(hasLHS(Literal0), hasOperatorName(">="),
- hasRHS(CountCall))
- .bind("negativeComparison"));
- AddSimpleMatcher(
- binaryOperation(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1))
- .bind("negativeComparison"));
- AddSimpleMatcher(
- binaryOperation(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall))
- .bind("negativeComparison"));
-
- // Find membership tests based on `find() == end()` or `find() == npos`.
- AddSimpleMatcher(
- binaryOperation(hasOperatorName("!="),
- hasOperands(FindCall, anyOf(EndCall, StringNpos)))
- .bind("positiveComparison"));
- AddSimpleMatcher(
- binaryOperation(hasOperatorName("=="),
- hasOperands(FindCall, anyOf(EndCall, StringNpos)))
- .bind("negativeComparison"));
+ Finder->addMatcher(
+ traverse(TK_AsIs,
+ implicitCastExpr(hasImplicitDestinationType(booleanType()),
+ hasSourceExpression(CountCall))
+ .bind("positiveComparison")),
+ this);
+
+ const auto PositiveComparison =
+ anyOf(allOf(hasOperatorName("!="), hasOperands(CountCall, Literal0)),
+ allOf(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0)),
+ allOf(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall)),
+ allOf(hasLHS(CountCall), hasOperatorName(">="), hasRHS(Literal1)),
+ allOf(hasLHS(Literal1), hasOperatorName("<="), hasRHS(CountCall)),
+ allOf(hasOperatorName("!="),
+ hasOperands(FindCall, anyOf(EndCall, StringNpos))));
+
+ const auto NegativeComparison =
+ anyOf(allOf(hasOperatorName("=="), hasOperands(CountCall, Literal0)),
+ allOf(hasLHS(CountCall), hasOperatorName("<="), hasRHS(Literal0)),
+ allOf(hasLHS(Literal0), hasOperatorName(">="), hasRHS(CountCall)),
+ allOf(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1)),
+ allOf(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall)),
+ allOf(hasOperatorName("=="),
+ hasOperands(FindCall, anyOf(EndCall, StringNpos))));
+
+ Finder->addMatcher(
+ binaryOperation(
+ anyOf(allOf(PositiveComparison, expr().bind("positiveComparison")),
+ allOf(NegativeComparison, expr().bind("negativeComparison")))),
+ this);
}
void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
index 8e058f20427fd..8d043910fc2ba 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
@@ -29,7 +29,7 @@ class ContainerContainsCheck : public ClangTidyCheck {
return LO.CPlusPlus;
}
std::optional<TraversalKind> getCheckTraversalKind() const override {
- return TK_AsIs;
+ return TK_IgnoreUnlessSpelledInSource;
}
};
|
|
To give better perspective, what is speedup according to --enable-check-profile? If I understand correctly, running |
|
Before: ---User Time--- --System Time-- --User+System-- ---Wall Time--- --- Name ---
2.1875 (100.0%) 0.0938 (100.0%) 2.2812 (100.0%) 2.2609 (100.0%) readability-container-containsAfter: ---User Time--- --System Time-- --User+System-- ---Wall Time--- --- Name ---
0.2188 (100.0%) 0.0781 (100.0%) 0.2969 (100.0%) 0.3131 (100.0%) readability-container-contains |
|
Should pull request be marked as |
|
This feels like a gray area: is a performance change a "functional" change? LLVM docs define NFC as "pure refactoring/cleanup", so it seems this isn't NFC, but it would be nice if it was more explicit. More discussion here: https://groups.google.com/g/llvm-dev/c/AXCgjDwcWnQ Update: Discord discussion yields conflicting opinions! |
zeyi2
left a comment
zeyi2
left a comment
There was a problem hiding this comment.
The code part LGTM, thank you!
Regarding the commit message: personally I don't think this is a NFC change, so I'm fine with the current PR title/message :)
|
We changed AS-IS to IgnoreUnless.. but there are no tests with template functions: template <typename T, U>
void foo(map<T, U> m) {
m.size() = ...
}Can we add them? (I think we should add them before your change and then see if everything still pass) The speedup is impressive in terms of check speed! I think we should look at --enable-check-profile data rather then full clang-tidy time |
|
The check currently doesn't work with templated containers: https://godbolt.org/z/9Ts9d9EcG llvm-project/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp Lines 129 to 137 in d593bcd |
|
The tests currently seems broken; take a look at https://godbolt.org/z/6bKG3Y8Y3. The last warning is line 453, and in actual tests we should have 487 line |
|
And the check doesn't work if we use actual map instead of fake one: And If you instantiate the function, it works with the template. |
This seems to be because of the unfortunate fact that |
And |
|
Yes, really. I should postpone all reviews till the end of vacation... But, with instantiation templates still works https://godbolt.org/z/hMP6E85q7. |
This PR breaks that case, true. But I hope we can agree that the current behaviour is still not ideal, right? We would like the check to work whether you instantiate the template or not. That would require another PR implementing heuristic name resolution (and I would be happy to write it!).
I think it would be less work in the end to leave this PR as-is, go implement the heuristic, then come back here: does that sound like a plan? |
I think it's not perfectly ideal, but good enough for 99.9..% cases. I think we have dozens of other checks that operate with same logic involving templates. (To find exact number need grep
This sounds good to me. I don't like merging with just |
zwuis
left a comment
zwuis
left a comment
There was a problem hiding this comment.
LGTM.
I'm neutral about the commit title (with or without NFC).
And If you instantiate the function, it works with the template.
This PR breaks that case, true. ...
I don't get it. How the case is broken? Changing traversal mode? Did I miss anything?
We changed AS-IS to IgnoreUnless.. ...
See lambda AddSimpleMatcher deleted by this PR, which uses TK_IgnoreUnlessSpelledInSource. Traversal mode of each matcher doesn't change.
Oh, actually I missed that we already had
As I understand, we didn't break anything with current patch. However, the check had FN before: e.g. https://godbolt.org/z/459M3EaTe: even if |
|
Oh yeah, looks like I was confused: I think I saw that the check flags the “conversion to bool” pattern even in templates, and assumed that it works 100% even in templates, but that’s not the case. I agree now, this change preserves behavior. |
This is currently one of our most expensive checks according to `--enable-check-profile`. I measured using [the usual setup](llvm#174357 (comment)): ```sh hyperfine \ --shell=none \ --prepare='cmake --build build/release --target clang-tidy' \ './build/release/bin/clang-tidy --checks=-*,readability-container-contains all_headers.cpp -header-filter=.* -system-headers -- -std=c++23 -fno-delayed-template-parsing' ``` First, the status quo: ```txt Time (mean ± σ): 5.243 s ± 0.158 s [User: 4.964 s, System: 0.248 s] Range (min … max): 5.133 s … 5.612 s 10 runs ``` This PR improves that in two independent commits. The first commit changes the default traversal mode from `TK_AsIs` to `TK_IgnoreUnlessSpelledInSource`. Result: ```txt Time (mean ± σ): 4.554 s ± 0.100 s [User: 4.273 s, System: 0.248 s] Range (min … max): 4.442 s … 4.785 s 10 runs ``` The second commit merges all 12 of the check's `BinaryOperation` matchers into one, because I found that each additional registered `BinaryOperation` matcher has pretty extreme overhead. Result: ```txt Time (mean ± σ): 3.480 s ± 0.121 s [User: 3.264 s, System: 0.186 s] Range (min … max): 3.400 s … 3.798 s 10 runs ```
This is currently one of our most expensive checks according to `--enable-check-profile`. I measured using [the usual setup](llvm#174357 (comment)): ```sh hyperfine \ --shell=none \ --prepare='cmake --build build/release --target clang-tidy' \ './build/release/bin/clang-tidy --checks=-*,readability-container-contains all_headers.cpp -header-filter=.* -system-headers -- -std=c++23 -fno-delayed-template-parsing' ``` First, the status quo: ```txt Time (mean ± σ): 5.243 s ± 0.158 s [User: 4.964 s, System: 0.248 s] Range (min … max): 5.133 s … 5.612 s 10 runs ``` This PR improves that in two independent commits. The first commit changes the default traversal mode from `TK_AsIs` to `TK_IgnoreUnlessSpelledInSource`. Result: ```txt Time (mean ± σ): 4.554 s ± 0.100 s [User: 4.273 s, System: 0.248 s] Range (min … max): 4.442 s … 4.785 s 10 runs ``` The second commit merges all 12 of the check's `BinaryOperation` matchers into one, because I found that each additional registered `BinaryOperation` matcher has pretty extreme overhead. Result: ```txt Time (mean ± σ): 3.480 s ± 0.121 s [User: 3.264 s, System: 0.186 s] Range (min … max): 3.400 s … 3.798 s 10 runs ```
This is currently one of our most expensive checks according to `--enable-check-profile`. I measured using [the usual setup](llvm#174357 (comment)): ```sh hyperfine \ --shell=none \ --prepare='cmake --build build/release --target clang-tidy' \ './build/release/bin/clang-tidy --checks=-*,readability-container-contains all_headers.cpp -header-filter=.* -system-headers -- -std=c++23 -fno-delayed-template-parsing' ``` First, the status quo: ```txt Time (mean ± σ): 5.243 s ± 0.158 s [User: 4.964 s, System: 0.248 s] Range (min … max): 5.133 s … 5.612 s 10 runs ``` This PR improves that in two independent commits. The first commit changes the default traversal mode from `TK_AsIs` to `TK_IgnoreUnlessSpelledInSource`. Result: ```txt Time (mean ± σ): 4.554 s ± 0.100 s [User: 4.273 s, System: 0.248 s] Range (min … max): 4.442 s … 4.785 s 10 runs ``` The second commit merges all 12 of the check's `BinaryOperation` matchers into one, because I found that each additional registered `BinaryOperation` matcher has pretty extreme overhead. Result: ```txt Time (mean ± σ): 3.480 s ± 0.121 s [User: 3.264 s, System: 0.186 s] Range (min … max): 3.400 s … 3.798 s 10 runs ```
6 participants
This is currently one of our most expensive checks according to
--enable-check-profile. I measured using the usual setup:hyperfine \ --shell=none \ --prepare='cmake --build build/release --target clang-tidy' \ './build/release/bin/clang-tidy --checks=-*,readability-container-contains all_headers.cpp -header-filter=.* -system-headers -- -std=c++23 -fno-delayed-template-parsing'First, the status quo:
This PR improves that in two independent commits. The first commit changes the default traversal mode from
TK_AsIstoTK_IgnoreUnlessSpelledInSource. Result:The second commit merges all 12 of the check's
BinaryOperationmatchers into one, because I found that each additional registeredBinaryOperationmatcher has pretty extreme overhead. Result: