[PATCH] D117529: [clangd][NFC] Cache ClangTidy check globs to speed up createChecks

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 18 16:55:34 PST 2022

sammccall added a comment.

Happy to take a look at this, but is there a particular motive for optimizing this?
Looking at some profiles this appears to be something like 0.1-0.5ms in fairly complex configurations.

Comment at: clang-tools-extra/clangd/ParsedAST.cpp:289
+// necessary checks efficiently.
+class CachedFactories {
+  using FactoryFunc = std::function<std::unique_ptr<tidy::ClangTidyCheck>(
if it's possible, it seems like this class really wants to be a function

Comment at: clang-tools-extra/clangd/ParsedAST.cpp:293
+  llvm::SmallVector<std::pair<std::string, FactoryFunc>, 0> AllChecks;
+  std::mutex Guard;
+  llvm::StringMap<llvm::BitVector> CachedGlobs;
I wonder if a global map is overkill (and the locking maybe undesirable).

In practice, this is being called in a loop on a single thread (the AST worker thread), with no other calls on that thread.
The idea with caching is that the `checks` globs are usually the same, that is especially true for parses of the main file.

So maybe we could just use a single-entry `thread_local` cache, with no map and no locking?

Comment at: clang-tools-extra/clangd/ParsedAST.cpp:296
+  llvm::BitVector &getCachedValue(StringRef CheckGlobString) {
+    std::lock_guard<std::mutex> LG(Guard);
why cache a bitmap instead of the vector<factory> directly?

Comment at: clang-tools-extra/clangd/ParsedAST.cpp:468
     trace::Span Tracer("ClangTidyInit");
-    tidy::ClangTidyCheckFactories CTFactories;
-    for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
-      E.instantiate()->addCheckFactories(CTFactories);
+    static CachedFactories Factory;
we need to be more careful with static state than this. This adds global destructors and we need to suppress them. (e.g. by using `new` without delete)

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list