This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy][NFC] Reimplement SimplifyBooleanExpr with RecursiveASTVisitors
ClosedPublic

Authored by njames93 on May 5 2022, 10:53 AM.

Details

Summary

Reimplement the matching logic using Visitors instead of matchers.

Benchmarks from running the check over SemaCodeComplete.cpp
Before 0.20s, After 0.04s

Diff Detail

Event Timeline

njames93 created this revision.May 5 2022, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 10:53 AM
njames93 requested review of this revision.May 5 2022, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 10:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
njames93 updated this revision to Diff 427418.May 5 2022, 12:35 PM

Remove redundant traversal of CompoundStmt in a replace method.

njames93 updated this revision to Diff 427430.May 5 2022, 1:39 PM

Transform final matcher, Benchmark -> 0.82s.

njames93 updated this revision to Diff 427436.May 5 2022, 1:51 PM

Remove SimplifyBoolExprMatchers header

njames93 updated this revision to Diff 427529.May 5 2022, 10:22 PM

Remove unnecessary tests in ReadabilityTidyModule now that the SimplifyBooleanExprMatchers header has been removed.

njames93 updated this revision to Diff 428041.May 9 2022, 5:18 AM

Clean up some code.

njames93 retitled this revision from [clang-tidy][NFC] Reimplement most of SimplifyBooleanExpr with RecursiveASTVisitors to [clang-tidy][NFC] Reimplement SimplifyBooleanExpr with RecursiveASTVisitors.May 9 2022, 5:57 AM
njames93 updated this revision to Diff 429659.May 16 2022, 2:34 AM

Tweak logic for detecting nested if.

njames93 edited the summary of this revision. (Show Details)May 16 2022, 3:12 AM

Since latest update speed up gone from 0.87s to 0.04s for SemaCodeComplete.

aaron.ballman accepted this revision.May 16 2022, 5:26 AM

LGTM, thank you for this!

This revision is now accepted and ready to land.May 16 2022, 5:26 AM

So those times were a little unfair as now we don't use ParentMapContext, however the cost of building that is included in the running time for the check.
In a use case where you have other enabled checks with do make use of that, the cost for building would be moved to other checks.
In order to level the playing field slightly, I've build the ParentMapContext ahead of time in both implementations for a fairer comparison in typical use cases.
This yields 0.20s for the original impl and 0.04s for this implementation.
Not quite as large a gap but still a 5x improvement.

njames93 edited the summary of this revision. (Show Details)May 16 2022, 6:41 AM

Just for my own edification, how did you know/suspect that a pure visitor would be faster than matchers?

Just for my own edification, how did you know/suspect that a pure visitor would be faster than matchers?

Mainly cause the fact we are creating 2 matcher expressions that differ by a bool value for each pattern, a visitor can easily handle both cases in one go.
There's also overhead with ASTMatchers that for simple cases sometimes may not be worth it.
It's not all good news though, there's a cost associated with running another ASTTraversal, however as we already have a visitor in this check we don't pay for that cost again.