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

[Sam McCall via Phabricator via cfe-commits]

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;
     CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
----------------
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117529/new/

https://reviews.llvm.org/D117529