[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, mostly LG. some nits around comments and request for a knob :)



================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:80
+
+    auto Task = [FIndex(FIndex), Path(Path.str()), Version(Version.str()),
+                 ASTCtx(std::move(ASTCtx)),
----------------
let's leave a comment here saying that `FIndex` outlives the `UpdateIndexCallbacks`.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:83
+                 CanonIncludes(CanonIncludes)]() mutable {
+      FIndex->updatePreamble(Path, Version, ASTCtx.getASTContext(),
+                             ASTCtx.getPreprocessor(), *CanonIncludes);
----------------
can you add a `trace::Span Tracer("PreambleIndexing");` here


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:87
 
-    if (FIndex)
-      FIndex->updatePreamble(Path, Version, Ctx, PP, CanonIncludes);
+    if (Tasks) {
+      Tasks->runAsync("Preamble indexing for:" + Path + Version,
----------------
can you also introduce a `bool AsyncPreambleIndexing = false;` into `ClangdServer::Options` struct and pass it into `UpdateIndexCallbacks` and make this conditional on that? after testing it for couple weeks, we can make it the default.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:211
     IndexTasks.emplace();
+
   // Pass a callback into `WorkScheduler` to extract symbols from a newly
----------------
nit: let's revert this change


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:105
   void AfterExecute(CompilerInstance &CI) override {
-    if (ParsedCallback) {
-      trace::Span Tracer("Running PreambleCallback");
-      ParsedCallback(CI.getASTContext(), CI.getPreprocessor(), CanonIncludes);
+    // Set PrecompilePreambleConsumer/PCHGenerator to null.
+    if (CI.getASTReader()) {
----------------
the comments are still just talking about what the code is already doing, and not "why".

```
// ASTContext and CompilerInstance can keep references to ASTConsumer (PCHGenerator in case of preamble builds).
// These references will become dangling after preamble build finishes, even if they didn't we don't want operations on the preamble to mutate PCH afterwards.
// So clear those references explicitly here.
```


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:696
+      auto Ctx = CapturedInfo.takeLife();
+      // Set the FileManager VFS to consuming FS.
+      auto StatCacheFS = Result->StatCache->getConsumingFS(VFS);
----------------
again comment is talking about "what" the code does rather than "why", what about:
```
// Stat cache is thread safe only when there are no producers. Hence change the VFS underneath to a consuming fs.
```


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:698
+      auto StatCacheFS = Result->StatCache->getConsumingFS(VFS);
+      Ctx->getFileManager().setVirtualFileSystem(StatCacheFS);
+      // While extending the life of FileMgr and VFS, StatCache should also be
----------------
nit: std::move(StatCacheFS) or just inline the initializer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148088